On Fri, Feb 19, 2021 at 12:19:20PM +0100, Robert Richter wrote: > 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. No, this API solves the above mentioned problem (what makes so special about pci_free_irq_vectors() that it should be present?) Why do we have pcim_iomap*() variations and not the rest? The PCIm API is horrible in the sense of being used properly. Yes, I know how it works and I was trying to help with that, the problem is that people didn't and don't get how it works and stream of patches like the ones that add pci_free_irq_vectors() are coming. > 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. True. And here is the part that most annoying right now. Btw, I never saw you fought against these small clean ups here and there, that *add* explicit calls to freeing resources. Also I haven't noticed anybody trying to correct documentation. This series is a step to a right direction. > But this adds another > bunch of useless functions. Wrong. This is quite useful to have balanced APIs. How many patches you have seen related to the PCIm imbalanced APIs? I could tell from my experience, I saw plenty and each time I'm trying to explain how it works people don't easily get. > > 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. It's open source, why should we do that and not what we are proposing here? Propose your ideas and we will discuss the patches, right? > > 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? Why it's not correctly freed? The function is idempotent. > 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(). No, it's not okay from API namespace / semantics perspective. > In the end, let's make sure everything is released in pci_dev if it is > managed and document this. Feel free to submit a patch! -- With Best Regards, Andy Shevchenko
