Re: [PATCH 04/16] PCI/P2PDMA: Refactor pci_p2pdma_map_type() to take pagmap and device

[John Hubbard]

On 5/3/21 11:56 AM, Logan Gunthorpe wrote:
...
> > IMHO, it is better to have all of the pci_*() functions dealing with pci_dev
> > instead of dev, but it is also true that this is a larger change, so I
> > won't press the point too hard right now.
>
> As a general rule, I'd agree with you. However, it's not good to blindly
> follow the rule when there might be good reasons to do it differently.
>
> In this case, the caller doesn't have PCI devices. The nvme fabrics code
> has a number of block devices and an RDMA device. It doesn't even know
> if these devices are backed by PCI devices and it doesn't have a direct
> path to obtain the pci_dev.
>
> Each struct device, might be turned into a pci_dev using the static
> function find_parent_pci_dev(). If any device is not a PCI device then
> we reject the P2PDMA transaction as not supported. Pushing the
> find_parent_pci_dev() logic into the callers is, IMO, just asking the
> callers to replicate a bunch of logic it shouldn't even be aware of
> creating messier code as a result.

I guess my main concern here is that there are these pci*() functions
that somehow want to pass around struct device. If a layer is carefully
named throughout with pci in the function names, then something is still
misaligned.  This can happen over time, of course. But the really best
patchsets try to avoid or mitigate the effect, by keeping names and
functionality carefully aligned.

Anyway, I've bugged you enough, I should just wait and see what the next
round looks like, at this point. :)

thanks,
--
John Hubbard
NVIDIA