| Message ID | 20210216160249.749799-1-zhengdejin5@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce pcim_alloc_irq_vectors() | expand |
On 17.02.21 00:02:45, Dejin Zheng wrote: > Introduce pcim_alloc_irq_vectors(), a device-managed version of > pci_alloc_irq_vectors(), In some i2c drivers, If pcim_enable_device() > has been called before, then pci_alloc_irq_vectors() is actually a > device-managed function. It is used as a device-managed function, So > replace it with pcim_alloc_irq_vectors(). > > Changelog > --------- > v2 -> v3: > - Add some commit comments for replace some codes in > pcim_release() by pci_free_irq_vectors(). > - Simplify the error handling path in i2c designware > driver. > v1 -> v2: > - Use pci_free_irq_vectors() to replace some code in > pcim_release(). > - Modify some commit messages. > > Dejin Zheng (4): > PCI: Introduce pcim_alloc_irq_vectors() > Documentation: devres: Add pcim_alloc_irq_vectors() This is already taken care of, see pcim_release(): if (dev->msi_enabled) pci_disable_msi(dev); if (dev->msix_enabled) pci_disable_msix(dev); Activated when used with pcim_enable_device(). This series is not required. -Robert > i2c: designware: Use the correct name of device-managed function > i2c: thunderx: Use the correct name of device-managed function > > .../driver-api/driver-model/devres.rst | 1 + > drivers/i2c/busses/i2c-designware-pcidrv.c | 15 +++------ > drivers/i2c/busses/i2c-thunderx-pcidrv.c | 2 +- > drivers/pci/pci.c | 33 ++++++++++++++++--- > include/linux/pci.h | 3 ++ > 5 files changed, 38 insertions(+), 16 deletions(-) > > -- > 2.25.0 >
On Thu, Feb 18, 2021 at 10:36:28AM +0100, Robert Richter wrote: > On 17.02.21 00:02:45, Dejin Zheng wrote: > > Introduce pcim_alloc_irq_vectors(), a device-managed version of > > pci_alloc_irq_vectors(), In some i2c drivers, If pcim_enable_device() > > has been called before, then pci_alloc_irq_vectors() is actually a > > device-managed function. It is used as a device-managed function, So > > replace it with pcim_alloc_irq_vectors(). > > > > Changelog > > --------- > > v2 -> v3: > > - Add some commit comments for replace some codes in > > pcim_release() by pci_free_irq_vectors(). > > - Simplify the error handling path in i2c designware > > driver. > > v1 -> v2: > > - Use pci_free_irq_vectors() to replace some code in > > pcim_release(). > > - Modify some commit messages. > > > > Dejin Zheng (4): > > PCI: Introduce pcim_alloc_irq_vectors() > > Documentation: devres: Add pcim_alloc_irq_vectors() > > This is already taken care of, see pcim_release(): > > if (dev->msi_enabled) > pci_disable_msi(dev); > if (dev->msix_enabled) > pci_disable_msix(dev); > > Activated when used with pcim_enable_device(). > > This series is not required. The problem this series solves is an imbalanced API. 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. 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.
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
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!