| Message ID | 20210215181550.714101-2-zhengdejin5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce pcim_alloc_irq_vectors() | expand |
Hi Dejin, Thank you for all the work here! The subject and the commit message could be improved to include a little more details about why do you want to do it, and what problems does it aims to solve. > Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of > pci_alloc_irq_vectors(). You can probably drop the "explicit" word from the sentence above. > +/** > + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors() > + * > + * It depends on calling pcim_enable_device() to make irq resources manageable. > + */ It would be "IRQ" in the sentence above. Also see [1] for more details about how to make a patch ready to be accepted. Also, this comment looks like it's intended to be compliant with the kernel-doc format, and if so, then you should describe each argument as the bare minimum, so that the entire comment would become this function documentation making it also a little more useful. See [2] for an example of how to use kernel-doc. > +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, > + unsigned int max_vecs, unsigned int flags) > +{ > + struct pci_devres *dr; > + > + /*Ensure that the pcim_enable_device() function has been called*/ The comment above has to be correctly aligned and it's also missing spaces around the sentence to be properly formatted, see [3]. > + dr = find_pci_dr(dev); > + if (!dr || !dr->enabled) > + return -EINVAL; > + > + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags); > +} Question: wouldn't you need to call pci_free_irq_vectors() somewhere, possibly to pcim_release() callback? Although, I am not sure where the right place would be. I am asking, as the documentation (see [4]) suggests that one would have to release allocated IRQ vectors (relevant exceprt): >> To automatically use MSI or MSI-X interrupt vectors, use the following >> function: >> >> int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, >> unsigned int max_vecs, unsigned int flags); >> >> which allocates up to max_vecs interrupt vectors for a PCI device. >> >> (...) >> >> Any allocated resources should be freed before removing the device using >> the following function: >> >> void pci_free_irq_vectors(struct pci_dev *dev); What do you think? 1. https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/ 2. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html 3. https://www.kernel.org/doc/html/latest/process/coding-style.html 4. https://www.kernel.org/doc/html/latest/PCI/msi-howto.html Krzysztof
On Mon, Feb 15, 2021 at 09:55:06PM +0100, Krzysztof Wilczyński wrote: > Question: wouldn't you need to call pci_free_irq_vectors() somewhere, > possibly to pcim_release() callback? Although, I am not sure where the > right place would be. > > I am asking, as the documentation (see [4]) suggests that one would have > to release allocated IRQ vectors (relevant exceprt): It's done in pcim_release() but not explicitly. if (dev->msi_enabled) pci_disable_msi(dev); if (dev->msix_enabled) pci_disable_msix(dev); Maybe above can be replaced by pci_free_irq_vectors() to be sure that any future change to PCI IRQ allocation APIs. Yes, I have checked and currently the above code is equivalent to pci_free_irq_vectors(). Dejin, please update your patch accordingly.
On Tue, Feb 16, 2021 at 12:12:39PM +0200, Andy Shevchenko wrote: > On Mon, Feb 15, 2021 at 09:55:06PM +0100, Krzysztof Wilczyński wrote: > > > Question: wouldn't you need to call pci_free_irq_vectors() somewhere, > > possibly to pcim_release() callback? Although, I am not sure where the > > right place would be. > > > > I am asking, as the documentation (see [4]) suggests that one would have > > to release allocated IRQ vectors (relevant exceprt): > > It's done in pcim_release() but not explicitly. > > if (dev->msi_enabled) > pci_disable_msi(dev); > if (dev->msix_enabled) > pci_disable_msix(dev); > > Maybe above can be replaced by pci_free_irq_vectors() to be sure that any > future change to PCI IRQ allocation APIs. > > Yes, I have checked and currently the above code is equivalent to > pci_free_irq_vectors(). > > Dejin, please update your patch accordingly. > Hi Andy and Krzysztof, I have modified it and sent patch v2. thank you very much! BR, Dejin > > -- > With Best Regards, > Andy Shevchenko > >
Hi Dejin and Andy, [...] > > > Question: wouldn't you need to call pci_free_irq_vectors() somewhere, > > > possibly to pcim_release() callback? Although, I am not sure where the > > > right place would be. > > > > > > I am asking, as the documentation (see [4]) suggests that one would have > > > to release allocated IRQ vectors (relevant exceprt): > > > > It's done in pcim_release() but not explicitly. > > > > if (dev->msi_enabled) > > pci_disable_msi(dev); > > if (dev->msix_enabled) > > pci_disable_msix(dev); > > > > Maybe above can be replaced by pci_free_irq_vectors() to be sure that any > > future change to PCI IRQ allocation APIs. > > > > Yes, I have checked and currently the above code is equivalent to > > pci_free_irq_vectors(). > > > > Dejin, please update your patch accordingly. > > > Hi Andy and Krzysztof, > > I have modified it and sent patch v2. thank you very much! Thank you Andy for double-checking! Much appreciated. Moving to pci_free_irq_vectors() directly looks great as we are also removing the surplus checks that pcim_release() is doing - since both the pci_disable_msi() and the pci_disable_msix() are doing internal validation to see whether MSI/MSI-X is currently enabled. Thank you again, Dejin and Andy. :) Krzysztof
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b67c4327d307..33244b512057 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2054,6 +2054,25 @@ void pcim_pin_device(struct pci_dev *pdev) } EXPORT_SYMBOL(pcim_pin_device); +/** + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors() + * + * It depends on calling pcim_enable_device() to make irq resources manageable. + */ +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, + unsigned int max_vecs, unsigned int flags) +{ + struct pci_devres *dr; + + /*Ensure that the pcim_enable_device() function has been called*/ + dr = find_pci_dr(dev); + if (!dr || !dr->enabled) + return -EINVAL; + + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags); +} +EXPORT_SYMBOL(pcim_alloc_irq_vectors); + /* * pcibios_add_device - provide arch specific hooks when adding device dev * @dev: the PCI device being added diff --git a/include/linux/pci.h b/include/linux/pci.h index 86c799c97b77..d75ba85ddfc5 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1818,6 +1818,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, NULL); } +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, + unsigned int max_vecs, unsigned int flags); + /* Include architecture-dependent settings and functions */ #include <asm/pci.h>
Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of pci_alloc_irq_vectors(). Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- drivers/pci/pci.c | 19 +++++++++++++++++++ include/linux/pci.h | 3 +++ 2 files changed, 22 insertions(+)