| Message ID | 20210218150458.798347-2-zhengdejin5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce pcim_alloc_irq_vectors() | expand |
On 18.02.21 23:04:55, Dejin Zheng wrote: > Introduce pcim_alloc_irq_vectors(), a device-managed version of > pci_alloc_irq_vectors(). Introducing this function can simplify > the error handling path in many drivers. > > And use pci_free_irq_vectors() to replace some code in pcim_release(), > they are equivalent, and no functional change. It is more explicit > that pcim_alloc_irq_vectors() is a device-managed function. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > v3 -> v4: > - No change > v2 -> v3: > - Add some commit comments for replace some codes in > pcim_release() by pci_free_irq_vectors(). > v1 -> v2: > - Use pci_free_irq_vectors() to replace some code in > pcim_release(). > - Modify some commit messages. > > drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++---- > include/linux/pci.h | 3 +++ > 2 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index b67c4327d307..db799d089c85 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1969,10 +1969,7 @@ static void pcim_release(struct device *gendev, void *res) > struct pci_devres *this = res; > int i; > > - if (dev->msi_enabled) > - pci_disable_msi(dev); > - if (dev->msix_enabled) > - pci_disable_msix(dev); > + pci_free_irq_vectors(dev); > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) > if (this->region_mask & (1 << i)) > @@ -2054,6 +2051,34 @@ void pcim_pin_device(struct pci_dev *pdev) > } > EXPORT_SYMBOL(pcim_pin_device); > > +/** > + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors() > + * @dev: PCI device to operate on > + * @min_vecs: minimum number of vectors required (must be >= 1) > + * @max_vecs: maximum (desired) number of vectors > + * @flags: flags or quirks for the allocation > + * > + * Return the number of vectors allocated, (which might be smaller than > + * @max_vecs) if successful, or a negative error code on error. If less > + * than @min_vecs interrupt vectors are available for @dev the function > + * will fail with -ENOSPC. > + * > + * 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; > + > + 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); If it is just about having a pcim-* counterpart why not just an inline function like the one below. > + > /* > * 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); > + static inline int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs, unsigned int max_vecs, unsigned int flags) { if (!pci_is_managed(dev, min_vecs, max_vecs, flags)) return -EINVAL; return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags); } All those stub functions with EXPORT_SYMBOLS etc. could be dropped then. With some macro magic added a list of functions could easily being created that are already managed but just need a pcim* counterpart. -Robert > /* Include architecture-dependent settings and functions */ > > #include <asm/pci.h> > -- > 2.25.0 >
On 19.02.21 15:40:11, Robert Richter wrote: > static inline int pcim_alloc_irq_vectors(struct pci_dev *dev, > unsigned int min_vecs, unsigned int max_vecs, unsigned int flags) > { > if (!pci_is_managed(dev, min_vecs, max_vecs, flags)) Obiously this is meant here: if (!pci_is_managed(dev)) > return -EINVAL; > > return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags); > }
On Fri, Feb 19, 2021 at 03:40:05PM +0100, Robert Richter wrote: > On 18.02.21 23:04:55, Dejin Zheng wrote: > > Introduce pcim_alloc_irq_vectors(), a device-managed version of > > pci_alloc_irq_vectors(). Introducing this function can simplify > > the error handling path in many drivers. > > > > And use pci_free_irq_vectors() to replace some code in pcim_release(), > > they are equivalent, and no functional change. It is more explicit > > that pcim_alloc_irq_vectors() is a device-managed function. ... > If it is just about having a pcim-* counterpart why not just an inline > function like the one below. It's a good suggestion, thanks! Still we need to amend pcim_release() to explicitly show that we call pci_free_irq_vectors().
On 19.02.21 16:48:57, Andy Shevchenko wrote: > On Fri, Feb 19, 2021 at 03:40:05PM +0100, Robert Richter wrote: > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > Introduce pcim_alloc_irq_vectors(), a device-managed version of > > > pci_alloc_irq_vectors(). Introducing this function can simplify > > > the error handling path in many drivers. > > > > > > And use pci_free_irq_vectors() to replace some code in pcim_release(), > > > they are equivalent, and no functional change. It is more explicit > > > that pcim_alloc_irq_vectors() is a device-managed function. > > ... > > > If it is just about having a pcim-* counterpart why not just an inline > > function like the one below. > > It's a good suggestion, thanks! > > Still we need to amend pcim_release() to explicitly show that we call > pci_free_irq_vectors(). Fair enough. Thanks, -Robert
Hi Robert, [...] > Obiously this is meant here: > > if (!pci_is_managed(dev)) [...] A question to improve my understanding for future reference. Was the previous approach of checking for "enabled" flag from struct pci_devres was not a good choice here? Krzysztof
On Fri, Feb 19, 2021 at 03:40:05PM +0100, Robert Richter wrote: > On 18.02.21 23:04:55, Dejin Zheng wrote: > > Introduce pcim_alloc_irq_vectors(), a device-managed version of > > pci_alloc_irq_vectors(). Introducing this function can simplify > > the error handling path in many drivers. > > > > And use pci_free_irq_vectors() to replace some code in pcim_release(), > > they are equivalent, and no functional change. It is more explicit > > that pcim_alloc_irq_vectors() is a device-managed function. > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > > --- > > v3 -> v4: > > - No change > > v2 -> v3: > > - Add some commit comments for replace some codes in > > pcim_release() by pci_free_irq_vectors(). > > v1 -> v2: > > - Use pci_free_irq_vectors() to replace some code in > > pcim_release(). > > - Modify some commit messages. > > > > drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++---- > > include/linux/pci.h | 3 +++ > > 2 files changed, 32 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index b67c4327d307..db799d089c85 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1969,10 +1969,7 @@ static void pcim_release(struct device *gendev, void *res) > > struct pci_devres *this = res; > > int i; > > > > - if (dev->msi_enabled) > > - pci_disable_msi(dev); > > - if (dev->msix_enabled) > > - pci_disable_msix(dev); > > + pci_free_irq_vectors(dev); > > > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) > > if (this->region_mask & (1 << i)) > > @@ -2054,6 +2051,34 @@ void pcim_pin_device(struct pci_dev *pdev) > > } > > EXPORT_SYMBOL(pcim_pin_device); > > > > +/** > > + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors() > > + * @dev: PCI device to operate on > > + * @min_vecs: minimum number of vectors required (must be >= 1) > > + * @max_vecs: maximum (desired) number of vectors > > + * @flags: flags or quirks for the allocation > > + * > > + * Return the number of vectors allocated, (which might be smaller than > > + * @max_vecs) if successful, or a negative error code on error. If less > > + * than @min_vecs interrupt vectors are available for @dev the function > > + * will fail with -ENOSPC. > > + * > > + * 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; > > + > > + 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); > > If it is just about having a pcim-* counterpart why not just an inline > function like the one below. > Robert and Andy, First of all, thank you very much for your suggestions and help. I think this is not just a pcim-* counterpart, I may not explain this place clearly. In addition to calling pci_alloc_irq_vectors(), the pcim_alloc_irq_vectors() function also checks whether the pci device is enabled and whether the pci device resource has been managed. If any one is wrong, it will return failure. Therefore, I think it should be used as a function. For novices, maybe I understand it incorrectly, so I look forward to your suggestions. > > + dr = find_pci_dr(dev); static struct pci_devres *find_pci_dr(struct pci_dev *pdev) { if (pci_is_managed(pdev)) return devres_find(&pdev->dev, pcim_release, NULL, NULL); return NULL; } here checks whether the pci device resource has been managed. > > + if (!dr || !dr->enabled) here checks whether the pci device is enabled. int pcim_enable_device(struct pci_dev *pdev) { struct pci_devres *dr; int rc; dr = get_pci_dr(pdev); if (unlikely(!dr)) return -ENOMEM; if (dr->enabled) return 0; rc = pci_enable_device(pdev); if (!rc) { pdev->is_managed = 1; dr->enabled = 1; } return rc; } BR, Dejin > > + > > /* > > * 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); > > + > > static inline int pcim_alloc_irq_vectors(struct pci_dev *dev, > unsigned int min_vecs, unsigned int max_vecs, unsigned int flags) > { > if (!pci_is_managed(dev, min_vecs, max_vecs, flags)) > return -EINVAL; > > return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags); > } > > All those stub functions with EXPORT_SYMBOLS etc. could be dropped > then. > > With some macro magic added a list of functions could easily being > created that are already managed but just need a pcim* counterpart. > > -Robert > > > /* Include architecture-dependent settings and functions */ > > > > #include <asm/pci.h> > > -- > > 2.25.0 > >
On 20.02.21 00:46:49, Dejin Zheng wrote: > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > + if (!dr || !dr->enabled) > here checks whether the pci device is enabled. What is the purpose of this? The device "is_managed" or not. -Robert
On 19.02.21 17:15:50, Krzysztof Wilczyński wrote: > Hi Robert, > > [...] > > Obiously this is meant here: > > > > if (!pci_is_managed(dev)) > [...] > > A question to improve my understanding for future reference. Was the > previous approach of checking for "enabled" flag from struct pci_devres > was not a good choice here? Initially this was meant to just show the idea. After careful review I don't see this additional check is required as once the pci dev is managed, it will be always released with pcim_release(). -Robert
On Mon, Feb 22, 2021 at 11:56:08AM +0100, Robert Richter wrote: > On 20.02.21 00:46:49, Dejin Zheng wrote: > > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > > > + if (!dr || !dr->enabled) > > here checks whether the pci device is enabled. > > What is the purpose of this? The device "is_managed" or not. > The device is managed or not by check whether "dr" is NULL. And check the "dr->enabled" is for the PCI device enable. I think it may not make sense to apply for irq vectors when PCI device is not enabled. PCI device enable by call pci_enable_device() function, this function initialize device before it's used by a driver. Ask low-level code to enable I/O and memory. Wake up the device if it was suspended. So I think it might be better to return to failure when it is found the PCI device is not enabled in the pcim_alloc_irq_vectors() function. It can facilitate developers to find problems as soon as possible. BR, Dejin > -Robert
On 22.02.21 23:14:15, Dejin Zheng wrote: > On Mon, Feb 22, 2021 at 11:56:08AM +0100, Robert Richter wrote: > > On 20.02.21 00:46:49, Dejin Zheng wrote: > > > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > > > > > + if (!dr || !dr->enabled) > > > here checks whether the pci device is enabled. > > > > What is the purpose of this? The device "is_managed" or not. > > > The device is managed or not by check whether "dr" is NULL. And > check the "dr->enabled" is for the PCI device enable. I think it > may not make sense to apply for irq vectors when PCI device is not > enabled. I don't see how a disabled device affects in any way the release of the irq vectors during device removal. dr is always non-null in case the device is managed, a check isn't needed for that. -Robert
On Tue, Feb 23, 2021 at 09:02:54AM +0100, Robert Richter wrote: > On 22.02.21 23:14:15, Dejin Zheng wrote: > > On Mon, Feb 22, 2021 at 11:56:08AM +0100, Robert Richter wrote: > > > On 20.02.21 00:46:49, Dejin Zheng wrote: > > > > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > > > > > > > + if (!dr || !dr->enabled) > > > > here checks whether the pci device is enabled. > > > > > > What is the purpose of this? The device "is_managed" or not. > > > > > The device is managed or not by check whether "dr" is NULL. And > > check the "dr->enabled" is for the PCI device enable. I think it > > may not make sense to apply for irq vectors when PCI device is not > > enabled. > > I don't see how a disabled device affects in any way the release of > the irq vectors during device removal. dr is always non-null in case > the device is managed, a check isn't needed for that. > Yes, the disabled device does not affect release irq vectors, But the disabled device affects apply for irq vectors, It is wrong to apply for the irq vectors when the device is not enabled. Add this check can facilitate developers to find problems as soon as possible. > -Robert
On 23.02.21 22:14:35, Dejin Zheng wrote: > On Tue, Feb 23, 2021 at 09:02:54AM +0100, Robert Richter wrote: > > On 22.02.21 23:14:15, Dejin Zheng wrote: > > > On Mon, Feb 22, 2021 at 11:56:08AM +0100, Robert Richter wrote: > > > > On 20.02.21 00:46:49, Dejin Zheng wrote: > > > > > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > > > > > > > > > + if (!dr || !dr->enabled) > > > > > here checks whether the pci device is enabled. > > > > > > > > What is the purpose of this? The device "is_managed" or not. > > > > > > > The device is managed or not by check whether "dr" is NULL. And > > > check the "dr->enabled" is for the PCI device enable. I think it > > > may not make sense to apply for irq vectors when PCI device is not > > > enabled. > > > > I don't see how a disabled device affects in any way the release of > > the irq vectors during device removal. dr is always non-null in case > > the device is managed, a check isn't needed for that. > > > Yes, the disabled device does not affect release irq vectors, But > the disabled device affects apply for irq vectors, It is wrong to apply > for the irq vectors when the device is not enabled. What is the scenario you have in mind here? What does happen then? The typical use case is to pcim_enable_device() it and then add the irq vectors. It is always enabled then. Even if the device could wrongly be disabled, it does not affect the device's release. Also, how is this related to pcim? There isn't a check in pci_alloc_irq_vectors() either for that case. > Add this check can > facilitate developers to find problems as soon as possible. No, there are many ways to shoot yourself in the foot. We cannot add checks here and there for this, esp. at runtime. If there is a valid reason that the device must always be enabled and we cannot assume this is the case, then we could add a WARN_ON(). But I doubt that. -Robert
On Thu, Feb 25, 2021 at 10:33:53AM +0100, Robert Richter wrote: > On 23.02.21 22:14:35, Dejin Zheng wrote: > > On Tue, Feb 23, 2021 at 09:02:54AM +0100, Robert Richter wrote: > > > On 22.02.21 23:14:15, Dejin Zheng wrote: > > > > On Mon, Feb 22, 2021 at 11:56:08AM +0100, Robert Richter wrote: > > > > > On 20.02.21 00:46:49, Dejin Zheng wrote: > > > > > > > On 18.02.21 23:04:55, Dejin Zheng wrote: > > > > > > > > > > > > > + if (!dr || !dr->enabled) > > > > > > here checks whether the pci device is enabled. > > > > > > > > > > What is the purpose of this? The device "is_managed" or not. > > > > > > > > > The device is managed or not by check whether "dr" is NULL. And > > > > check the "dr->enabled" is for the PCI device enable. I think it > > > > may not make sense to apply for irq vectors when PCI device is not > > > > enabled. > > > > > > I don't see how a disabled device affects in any way the release of > > > the irq vectors during device removal. dr is always non-null in case > > > the device is managed, a check isn't needed for that. > > > > > Yes, the disabled device does not affect release irq vectors, But > > the disabled device affects apply for irq vectors, It is wrong to apply > > for the irq vectors when the device is not enabled. > > What is the scenario you have in mind here? What does happen then? > The typical use case is to pcim_enable_device() it and then add the > irq vectors. It is always enabled then. > > Even if the device could wrongly be disabled, it does not affect the > device's release. > > Also, how is this related to pcim? There isn't a check in > pci_alloc_irq_vectors() either for that case. > > > Add this check can > > facilitate developers to find problems as soon as possible. > > No, there are many ways to shoot yourself in the foot. We cannot add > checks here and there for this, esp. at runtime. If there is a valid > reason that the device must always be enabled and we cannot assume > this is the case, then we could add a WARN_ON(). But I doubt that. > Robert, You are right, I will remove the enable check. Thanks! BR, Dejin > -Robert
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b67c4327d307..db799d089c85 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1969,10 +1969,7 @@ static void pcim_release(struct device *gendev, void *res) struct pci_devres *this = res; int i; - if (dev->msi_enabled) - pci_disable_msi(dev); - if (dev->msix_enabled) - pci_disable_msix(dev); + pci_free_irq_vectors(dev); for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) if (this->region_mask & (1 << i)) @@ -2054,6 +2051,34 @@ void pcim_pin_device(struct pci_dev *pdev) } EXPORT_SYMBOL(pcim_pin_device); +/** + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors() + * @dev: PCI device to operate on + * @min_vecs: minimum number of vectors required (must be >= 1) + * @max_vecs: maximum (desired) number of vectors + * @flags: flags or quirks for the allocation + * + * Return the number of vectors allocated, (which might be smaller than + * @max_vecs) if successful, or a negative error code on error. If less + * than @min_vecs interrupt vectors are available for @dev the function + * will fail with -ENOSPC. + * + * 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; + + 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 device-managed version of pci_alloc_irq_vectors(). Introducing this function can simplify the error handling path in many drivers. And use pci_free_irq_vectors() to replace some code in pcim_release(), they are equivalent, and no functional change. It is more explicit that pcim_alloc_irq_vectors() is a device-managed function. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- v3 -> v4: - No change v2 -> v3: - Add some commit comments for replace some codes in pcim_release() by pci_free_irq_vectors(). v1 -> v2: - Use pci_free_irq_vectors() to replace some code in pcim_release(). - Modify some commit messages. drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++---- include/linux/pci.h | 3 +++ 2 files changed, 32 insertions(+), 4 deletions(-)