On 2021-05-02 2:41 p.m., John Hubbard wrote: > On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >> 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
