[lkml]   [2021]   [May]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 03/16] PCI/P2PDMA: Attempt to set map_type if it has not been set
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,
>> 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,
>>> 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.

John Hubbard

 \ /
  Last update: 2021-05-03 20:23    [W:3.426 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site