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

[John Hubbard <jhubbard@nvidia.com>]

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