On 2021-05-02 2:41 p.m., John Hubbard wrote:

On 4/8/21 10:01 AM, Logan Gunthorpe wrote:

quoted

All callers of pci_p2pdma_map_type() have a struct dev_pgmap and a
struct device (of the client doing the DMA transfer). Thus move the
conversion to struct pci_devs for the provider and client into this
function.

Actually, this is the wrong direction to go! All of these pre-existing
pci_*() functions have a small problem already: they are dealing with
struct device, instead of struct pci_dev. And so refactoring should be
pushing the conversion to pci_dev *up* the calling stack, not lower as
the patch here proposes.

Also, there is no improvement in clarity by passing in (pgmap, dev)
instead of the previous (provider, client). Now you have to do more type
checking in the leaf function, which is another indication of a problem.

Let's go that direction, please? Just convert to pci_dev much higher in
the calling stack, and you'll find that everything fits together better.
And it's OK to pass in extra params if that turns out to be necessary,
after all.

No, I disagree with this and it seems a bit confused. This change is
allowing callers to call the function with what they have and doing more
checks inside the called function. This allows for *less* checks in the
leaf function, not more checks. (I mean, look at the patch itself, it
puts a bunch of checks in both call sites into the callee and makes the
code a lot cleaner -- it's removing more lines than it adds).

Similar argument can be made with the pci_p2pdma_distance_many() (which
I assume you are referring to). If the function took struct pci_dev
instead of struct device, every caller would need to do all checks and
conversions to struct pci_dev. That is not an improvement.

Logan