On 18.02.21 16:01:56, Andy Shevchenko wrote: > The problem this series solves is an imbalanced API.
This (added) API is bloated and incomplete. It adds functions without benefit, the only is to have a single pcim alloc function in addition to the pairing of alloc/free functions. I agree, it is hard to detect which parts are released if pcim_enable_device() is used. Additional, you need to go through pcim_release() to add other pcim_*() functions for everything else that is released there. Otherwise that new API is still incomplete. But this adds another bunch of useless functions. > Christoph IIRC was clear that if we want to use PCI IRQ allocation API the > caller must know what's going on. Hiding this behind the scenes is not good. > And this series unhides that. IMO, this is more a documentation issue. pcim_enable_device() must be better documented and list all enable/alloc functions that are going to be released out of the box later. Even better, make sure everything is managed and thus all of a pci_dev is released, no matter how it was setup (this could even already be the case). In addition you could implement a static code checker. > Also, you may go and clean up all pci_free_irq_vectors() when > pcim_enable_device() is called, but I guess you will get painful process and > rejection in a pile of cases. Why should something be rejected if it is not correctly freed? Even if pci_free_irq_vectors() is called, pcim_release() will not complain if it was already freed before. So using pci_free_irq_vectors() is ok even in conjunction with pcim_enable_device(). In the end, let's make sure everything is released in pci_dev if it is managed and document this. Thanks, -Robert
