Messages in this thread | | | Subject | Re: [PATCH 03/16] PCI/P2PDMA: Attempt to set map_type if it has not been set | From | John Hubbard <> | Date | Mon, 3 May 2021 11:22:34 -0700 |
| |
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
| |