Re: [PATCH 03/16] PCI/P2PDMA: Attempt to set map_type if it has not been set

[John Hubbard]

On 5/3/21 9:17 AM, Logan Gunthorpe wrote:
> > Returning a "bridge distance" from a "get map type" routine is jarring,
> > and I think it is because of a pre-existing problem: the above function
> > is severely misnamed. Let's try renaming it (and the other one) to
> > approximately:
> >
> >       upstream_bridge_map_type_warn()
> >       upstream_bridge_map_type()
> >
> > ...and that should fix that. Well, that, plus tweaking the kernel doc
> > comments, which are also confused. I think someone started off thinking
> > about distances through PCIe, but in the end, the routine boils down to
> > just a few situations that are not distances at all.
> >
> > Also, the above will read a little better if it is written like this:
> >
> > 	ret = xa_to_value(xa_load(&provider->p2pdma->map_types,
> > 				  map_types_idx(client)));
> >
> > 	if (ret == PCI_P2PDMA_MAP_UNKNOWN)
> > 		ret = upstream_bridge_map_type_warn(provider, client, NULL,
> > 						    GFP_ATOMIC);
> > 	
> > 	return ret;
> >
> >
> > >    }
>
> I agree that some of this has evolved in a way that some of the names
> are a bit odd now. Could definitely use a cleanup, but that's not really
> part of this series. When I have some time I can look at doing a cleanup
> series to help with some of this.

I'm OK with doing cleanup later. I just tend to call it out when I see it.

> > >    static int __pci_p2pdma_map_sg(struct pci_p2pdma_pagemap *p2p_pgmap,
> > > @@ -877,7 +884,6 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
> > >    	case PCI_P2PDMA_MAP_BUS_ADDR:
> > >    		return __pci_p2pdma_map_sg(p2p_pgmap, dev, sg, nents);
> > >    	default:
> > > -		WARN_ON_ONCE(1);
> >
> > Why? Or at least, why, in this patch? It looks like an accidental
> > leftover from something, seeing as how it is not directly related to the
> > patch, and is not mentioned at all.
>
> Before this patch, it was required that users of P2PDMA call
> pci_p2pdma_distance_many() in some form before calling
> pci_p2pdma_map_sg(). So, by convention, a usable map type had to already
> be in the cache. The warning was there to yell at anyone who wrote code
> that violated that convention.
>
> This patch removes that convention and allows users to map P2PDMA pages
> sight unseen and if the mapping type isn't in the cache, then it will
> determine the mapping type at dma mapping time. Thus, the warning can be
> removed and the function can fail normally if the mapping is unsupported.

Let's add some of those words to the commit description, perhaps, it's nice
to have. Obviously a minor point though.

thanks,
--
John Hubbard
NVIDIA