[PATCH v7 0/4] Introduce pcim_alloc_irq_vectors()

[PATCH v7 0/4] Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

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
---------
v6 -> v7:
	- rebase to PCI next branch
	- add a stub for pci_is_managed() when disable PCI for
	  fix build error in sparc architecture.
v5 -> v6:
	- rebase to 5.13-rc4
v4 -> v5:
	- Remove the check of enable device in pcim_alloc_irq_vectors()
	  and make it as a static line function.
	- Modify the subject name in patch 3 and patch 4.
v3 -> v4:
	- add some commit comments for patch 3
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()
  i2c: designware: Use pcim_alloc_irq_vectors() to allocate IRQ vectors
  i2c: thunderx: Use pcim_alloc_irq_vectors() to allocate IRQ vectors

 .../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                             |  5 +---
 include/linux/pci.h                           | 25 +++++++++++++++++++
 5 files changed, 32 insertions(+), 16 deletions(-)

-- 
2.30.1

[PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

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.

When CONFIG_PCI=n, there is no stub for pci_is_managed(), but
pcim_alloc_irq_vectors() will use it, so add one like other similar stubs.
Otherwise there can be build errors, as here by kernel test robot
reported:
include/linux/pci.h: In function 'pcim_alloc_irq_vectors':
>> include/linux/pci.h:1847:7: error: implicit declaration of function 'pci_is_managed' [-Werror=implicit-function-declaration]
    1847 |  if (!pci_is_managed(dev))
         |       ^~~~~~~~~~~~~~

Reported-by: kernel test robot <lkp@intel.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Robert Richter <rric@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v6 -> v7:
	- rebase to PCI next branch
	- add a stub for pci_is_managed() when disable PCI for
	  fix build error in sparc architecture.
v5 -> v6:
	- rebase to 5.13-rc4
v4 -> v5:
	- Remove the check of enable device in pcim_alloc_irq_vectors()
	  and make it as a static line function.
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   |  5 +----
 include/linux/pci.h | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 452351025a09..e3b3fc59bd35 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1989,10 +1989,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))
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59a57..5783262c4643 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1730,6 +1730,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
 
 static inline void pci_set_master(struct pci_dev *dev) { }
 static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
+static inline int pci_is_managed(struct pci_dev *pdev) { return 0; }
 static inline void pci_disable_device(struct pci_dev *dev) { }
 static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
 static inline int pci_assign_resource(struct pci_dev *dev, int i)
@@ -1825,6 +1826,30 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 					      NULL);
 }
 
+/**
+ * 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.
+ */
+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))
+		return -EINVAL;
+	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
+}
+
 /* Include architecture-dependent settings and functions */
 
 #include <asm/pci.h>
-- 
2.30.1

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.

...

> When CONFIG_PCI=n, there is no stub for pci_is_managed(), but
> pcim_alloc_irq_vectors() will use it, so add one like other similar stubs.
> Otherwise there can be build errors, as here by kernel test robot
> reported:
> include/linux/pci.h: In function 'pcim_alloc_irq_vectors':
> >> include/linux/pci.h:1847:7: error: implicit declaration of function 'pci_is_managed' [-Werror=implicit-function-declaration]
>     1847 |  if (!pci_is_managed(dev))
>          |       ^~~~~~~~~~~~~~

This is rather changelog related material. No need to pollute commit message
with this.

...

> Reported-by: kernel test robot <lkp@intel.com>

It's new functionality. Why this tag is here?
Use comments (similar location than changelog) to give a credit if you wish.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Robert Richter <rric@kernel.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> v6 -> v7:
> 	- rebase to PCI next branch
> 	- add a stub for pci_is_managed() when disable PCI for
> 	  fix build error in sparc architecture.
> v5 -> v6:
> 	- rebase to 5.13-rc4
> v4 -> v5:
> 	- Remove the check of enable device in pcim_alloc_irq_vectors()
> 	  and make it as a static line function.
> 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   |  5 +----
>  include/linux/pci.h | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 452351025a09..e3b3fc59bd35 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1989,10 +1989,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))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59a57..5783262c4643 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1730,6 +1730,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
>  
>  static inline void pci_set_master(struct pci_dev *dev) { }
>  static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> +static inline int pci_is_managed(struct pci_dev *pdev) { return 0; }
>  static inline void pci_disable_device(struct pci_dev *dev) { }
>  static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
>  static inline int pci_assign_resource(struct pci_dev *dev, int i)
> @@ -1825,6 +1826,30 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  					      NULL);
>  }
>  
> +/**
> + * 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.
> + */
> +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))
> +		return -EINVAL;
> +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> +}
> +
>  /* Include architecture-dependent settings and functions */
>  
>  #include <asm/pci.h>
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Bjorn Helgaas]

On Mon, Jun 07, 2021 at 07:12:34PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> 
> ...
> 
> > When CONFIG_PCI=n, there is no stub for pci_is_managed(), but
> > pcim_alloc_irq_vectors() will use it, so add one like other similar stubs.
> > Otherwise there can be build errors, as here by kernel test robot
> > reported:
> > include/linux/pci.h: In function 'pcim_alloc_irq_vectors':
> > >> include/linux/pci.h:1847:7: error: implicit declaration of function 'pci_is_managed' [-Werror=implicit-function-declaration]
> >     1847 |  if (!pci_is_managed(dev))
> >          |       ^~~~~~~~~~~~~~
> 
> This is rather changelog related material. No need to pollute commit message
> with this.
> 
> ...
> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> It's new functionality. Why this tag is here?
> Use comments (similar location than changelog) to give a credit if you wish.

Agreed, I'll tidy that up, so no need to repost for this.

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

On Mon, Jun 07, 2021 at 12:14:51PM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 07, 2021 at 07:12:34PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> > 
> > ...
> > 
> > > When CONFIG_PCI=n, there is no stub for pci_is_managed(), but
> > > pcim_alloc_irq_vectors() will use it, so add one like other similar stubs.
> > > Otherwise there can be build errors, as here by kernel test robot
> > > reported:
> > > include/linux/pci.h: In function 'pcim_alloc_irq_vectors':
> > > >> include/linux/pci.h:1847:7: error: implicit declaration of function 'pci_is_managed' [-Werror=implicit-function-declaration]
> > >     1847 |  if (!pci_is_managed(dev))
> > >          |       ^~~~~~~~~~~~~~
> > 
> > This is rather changelog related material. No need to pollute commit message
> > with this.
> > 
> > ...
> > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > It's new functionality. Why this tag is here?
> > Use comments (similar location than changelog) to give a credit if you wish.
> 
> Agreed, I'll tidy that up, so no need to repost for this.

Bjorn, Thank you very much, you are so nice!

BR,
Dejin

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

On Mon, Jun 07, 2021 at 07:12:34PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> 
> ...
> 
> > When CONFIG_PCI=n, there is no stub for pci_is_managed(), but
> > pcim_alloc_irq_vectors() will use it, so add one like other similar stubs.
> > Otherwise there can be build errors, as here by kernel test robot
> > reported:
> > include/linux/pci.h: In function 'pcim_alloc_irq_vectors':
> > >> include/linux/pci.h:1847:7: error: implicit declaration of function 'pci_is_managed' [-Werror=implicit-function-declaration]
> >     1847 |  if (!pci_is_managed(dev))
> >          |       ^~~~~~~~~~~~~~
> 
> This is rather changelog related material. No need to pollute commit message
> with this.
>
Okey.

> ...
> 
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> It's new functionality. Why this tag is here?
> Use comments (similar location than changelog) to give a credit if you wish.
>
Got it, Thanks!

> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Robert Richter <rric@kernel.org>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> > v6 -> v7:
> > 	- rebase to PCI next branch
> > 	- add a stub for pci_is_managed() when disable PCI for
> > 	  fix build error in sparc architecture.
> > v5 -> v6:
> > 	- rebase to 5.13-rc4
> > v4 -> v5:
> > 	- Remove the check of enable device in pcim_alloc_irq_vectors()
> > 	  and make it as a static line function.
> > 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   |  5 +----
> >  include/linux/pci.h | 25 +++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 452351025a09..e3b3fc59bd35 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1989,10 +1989,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))
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index c20211e59a57..5783262c4643 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1730,6 +1730,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
> >  
> >  static inline void pci_set_master(struct pci_dev *dev) { }
> >  static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> > +static inline int pci_is_managed(struct pci_dev *pdev) { return 0; }
> >  static inline void pci_disable_device(struct pci_dev *dev) { }
> >  static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
> >  static inline int pci_assign_resource(struct pci_dev *dev, int i)
> > @@ -1825,6 +1826,30 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> >  					      NULL);
> >  }
> >  
> > +/**
> > + * 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.
> > + */
> > +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))
> > +		return -EINVAL;
> > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > +}
> > +
> >  /* Include architecture-dependent settings and functions */
> >  
> >  #include <asm/pci.h>
> > -- 
> > 2.30.1
> > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Bjorn Helgaas]

On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 452351025a09..e3b3fc59bd35 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1989,10 +1989,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);

If I understand correctly, this hunk is a nice simplification, but
actually has nothing to do with making pcim_alloc_irq_vectors().  I
have it split to a separate patch in my local tree.  Or am I wrong
about that?

>  	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
>  		if (this->region_mask & (1 << i))
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e59a57..5783262c4643 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1730,6 +1730,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
>  
>  static inline void pci_set_master(struct pci_dev *dev) { }
>  static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> +static inline int pci_is_managed(struct pci_dev *pdev) { return 0; }
>  static inline void pci_disable_device(struct pci_dev *dev) { }
>  static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
>  static inline int pci_assign_resource(struct pci_dev *dev, int i)
> @@ -1825,6 +1826,30 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  					      NULL);
>  }
>  
> +/**
> + * 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.
> + */
> +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))
> +		return -EINVAL;
> +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);

This is great, but can you explain how pci_alloc_irq_vectors()
magically becomes a managed interface if we've already called
pcim_enable_device()?

I certainly believe it does; I'd just like to put a hint in the commit
log since my 5 minutes of grepping around didn't make it obvious to
me.

I see that pcim_enable_device() sets pdev->is_managed, but I didn't
find the connection between that and pci_alloc_irq_vectors().

> +}
> +
>  /* Include architecture-dependent settings and functions */
>  
>  #include <asm/pci.h>
> -- 
> 2.30.1
> 

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.

...

> > @@ -1989,10 +1989,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);
> 
> If I understand correctly, this hunk is a nice simplification, but
> actually has nothing to do with making pcim_alloc_irq_vectors().  I
> have it split to a separate patch in my local tree.  Or am I wrong
> about that?

It's a good simplification that had to be done when pci_free_irq_vectors()
appeared. But here is the fact that indirectly it's related to the pcim_*()
APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
pcim_release().

...

> > +/**
> > + * 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.
> > + */
> > +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))
> > +		return -EINVAL;
> > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> 
> This is great, but can you explain how pci_alloc_irq_vectors()
> magically becomes a managed interface if we've already called
> pcim_enable_device()?
> 
> I certainly believe it does; I'd just like to put a hint in the commit
> log since my 5 minutes of grepping around didn't make it obvious to
> me.
> 
> I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> find the connection between that and pci_alloc_irq_vectors().

One needs to read and understand the code, I agree. The explanation is spread
between pcim_release() and __pci_enable_msi/x_range().

The call chain is

msi_capability_init() / msix_capability_init()
  ...
  <- __pci_enable_msi/x_range()
    <- pci_alloc_irq_vectors_affinity()
      <- pci_alloc_irq_vectors()

where device msi_enabled / msix_enabled is set.

So, it may deserve to be explained in the commit message.

> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Bjorn Helgaas]

On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> 
> ...
> 
> > > @@ -1989,10 +1989,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);
> > 
> > If I understand correctly, this hunk is a nice simplification, but
> > actually has nothing to do with making pcim_alloc_irq_vectors().  I
> > have it split to a separate patch in my local tree.  Or am I wrong
> > about that?
> 
> It's a good simplification that had to be done when pci_free_irq_vectors()
> appeared.

Sorry to be pedantic.  You say the simplification "had to be done,"
but AFAICT there was no actual *requirement* for this simplification
to be done since pci_free_irq_vectors() is functionally identical to
the previous code.  I think we should do it because it's a little
simpler, but not because it *fixes* anything.

> But here is the fact that indirectly it's related to the pcim_*()
> APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> pcim_release().

Yes.  For posterity, my notes about the call chain (after applying
this patch):

  pci_alloc_irq_vectors
    pci_alloc_irq_vectors_affinity
      __pci_enable_msix_range                 # MSI-X path
        __pci_enable_msix
          msix_capability_init
            msix_setup_entries
              for (...)
                entry = alloc_msi_entry
                  kzalloc(msi_desc)           <--- alloc
                  kmemdup(msi_desc->affinity) <--- alloc
            dev->msix_enabled = 1             # MSI-X enabled
      __pci_enable_msi_range                  # MSI path
        msi_capability_init
          msi_setup_entry
            alloc_msi_entry                   <--- alloc
          dev->msi_enabled = 1                # MSI enabled

  pcim_release
    pci_free_irq_vectors
      pci_disable_msix                        # MSI-X
        if (!dev->msix_enabled)
          return
        pci_msix_shutdown
          dev->msix_enabled = 0               # MSI-X disabled
        free_msi_irqs
          list_for_each_entry_safe(..., msi_list, ...)
            free_msi_entry
              kfree(msi_desc->affinity)       <--- free
              kfree(msi_desc)                 <--- free
      pci_disable_msi                         # MSI
        if (!dev->msi_enabled)
          return
        pci_msi_shutdown
          dev->msi_enabled = 0                # MSI disabled
        free_msi_irqs                         <--- free

So I *think* (correct me if I'm wrong):

  - If a driver calls pcim_enable_device(), we will call
    pcim_release() when the last reference to the device is dropped.

  - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
    structures via msix_setup_entries() or msi_setup_entry().

  - pcim_release() will free those msi_desc and irq_affinity_desc
    structures.

  - Even before this series, pcim_release() frees msi_desc and
    irq_affinity_desc structures by calling pci_disable_msi() and
    pci_disable_msix().

  - Calling pci_free_irq_vectors() (or pci_disable_msi() or
    pci_disable_msix()) twice is unnecessary but probably harmless
    because they bail out early.

So this series actually does not fix any problems whatsoever.

It *does* remove unnecessary pci_free_irq_vectors() calls from
i2c-designware-pcidrv.c.

But because pci_alloc_irq_vectors() and related interfaces are
*already* managed as soon as a driver calls pcim_enable_device(),
we can simply remove the pci_free_irq_vectors() without doing anything
else.

I don't think we *should* do anything else.  There are many callers of
pcim_enable_device() that also call pci_alloc_irq_vectors(),
pci_enable_msix_range(), etc.  We don't have pcim_enable_msix_range(),
pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc.  I don't
think it's worth the churn of adding all those and changing all the
callers to use pcim_*() (as in patch 4/4 here).

Browsing the output of this:

  git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"

leads me to believe there are similar calls of pci_free_irq_vectors()
that could be removed here:

  mtip_pci_probe
  sp_pci_probe
  dw_edma_pcie_probe
  hisi_dma_probe
  ioat_pci_probe
  plx_dma_probe
  cci_pci_probe
  hibmc_pci_probe
  ...

and many more, but I got tired of looking.

> > > +/**
> > > + * 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.
> > > + */
> > > +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))
> > > +		return -EINVAL;
> > > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > 
> > This is great, but can you explain how pci_alloc_irq_vectors()
> > magically becomes a managed interface if we've already called
> > pcim_enable_device()?
> > 
> > I certainly believe it does; I'd just like to put a hint in the commit
> > log since my 5 minutes of grepping around didn't make it obvious to
> > me.
> > 
> > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > find the connection between that and pci_alloc_irq_vectors().
> 
> One needs to read and understand the code, I agree. The explanation is spread
> between pcim_release() and __pci_enable_msi/x_range().
> 
> The call chain is
> 
> msi_capability_init() / msix_capability_init()
>   ...
>   <- __pci_enable_msi/x_range()
>     <- pci_alloc_irq_vectors_affinity()
>       <- pci_alloc_irq_vectors()
> 
> where device msi_enabled / msix_enabled is set.
> 
> So, it may deserve to be explained in the commit message.
> 
> > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

Dejin, why Christoph's email suddenly disappeared during updating?

On Wed, Jun 16, 2021 at 02:25:43PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> > 
> > ...
> > 
> > > > @@ -1989,10 +1989,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);
> > > 
> > > If I understand correctly, this hunk is a nice simplification, but
> > > actually has nothing to do with making pcim_alloc_irq_vectors().  I
> > > have it split to a separate patch in my local tree.  Or am I wrong
> > > about that?
> > 
> > It's a good simplification that had to be done when pci_free_irq_vectors()
> > appeared.
> 
> Sorry to be pedantic.  You say the simplification "had to be done,"
> but AFAICT there was no actual *requirement* for this simplification
> to be done since pci_free_irq_vectors() is functionally identical to
> the previous code.
> I think we should do it because it's a little
> simpler, but not because it *fixes* anything.

It makes things more straightforward. So it definitely "fixes" something, but
not the code in this case, rather how we maintain this code.

> > But here is the fact that indirectly it's related to the pcim_*()
> > APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> > pcim_release().
> 
> Yes.  For posterity, my notes about the call chain (after applying
> this patch):
> 
>   pci_alloc_irq_vectors
>     pci_alloc_irq_vectors_affinity
>       __pci_enable_msix_range                 # MSI-X path
>         __pci_enable_msix
>           msix_capability_init
>             msix_setup_entries
>               for (...)
>                 entry = alloc_msi_entry
>                   kzalloc(msi_desc)           <--- alloc
>                   kmemdup(msi_desc->affinity) <--- alloc
>             dev->msix_enabled = 1             # MSI-X enabled
>       __pci_enable_msi_range                  # MSI path
>         msi_capability_init
>           msi_setup_entry
>             alloc_msi_entry                   <--- alloc
>           dev->msi_enabled = 1                # MSI enabled
> 
>   pcim_release
>     pci_free_irq_vectors
>       pci_disable_msix                        # MSI-X
>         if (!dev->msix_enabled)
>           return
>         pci_msix_shutdown
>           dev->msix_enabled = 0               # MSI-X disabled
>         free_msi_irqs
>           list_for_each_entry_safe(..., msi_list, ...)
>             free_msi_entry
>               kfree(msi_desc->affinity)       <--- free
>               kfree(msi_desc)                 <--- free
>       pci_disable_msi                         # MSI
>         if (!dev->msi_enabled)
>           return
>         pci_msi_shutdown
>           dev->msi_enabled = 0                # MSI disabled
>         free_msi_irqs                         <--- free
> 
> So I *think* (correct me if I'm wrong):
> 
>   - If a driver calls pcim_enable_device(), we will call
>     pcim_release() when the last reference to the device is dropped.
> 
>   - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
>     structures via msix_setup_entries() or msi_setup_entry().
> 
>   - pcim_release() will free those msi_desc and irq_affinity_desc
>     structures.
> 
>   - Even before this series, pcim_release() frees msi_desc and
>     irq_affinity_desc structures by calling pci_disable_msi() and
>     pci_disable_msix().
> 
>   - Calling pci_free_irq_vectors() (or pci_disable_msi() or
>     pci_disable_msix()) twice is unnecessary but probably harmless
>     because they bail out early.

> So this series actually does not fix any problems whatsoever.

I tend to disagree.

The PCI managed API is currently inconsistent and what you got is what I
already know and had been using until (see below) Christoph told not to do [1].

Even do you as PCI maintainer it took some time to figure this out. But current
APIs make it hard for mere users who wants to use it in the drivers.

So, main point of fix here is _API inconsistency_ [0].

But hey, I believe you have been Cc'ed to the initial submission of the
pci_*_irq_vector*() rework done by Christoph [2] (hmm... don't see your
name there). And he updated documentation as well [3].

Moreover, he insisted to use pci_free_irq_vectors() whenever we are using
pci_alloc_irq_vectors(). And he suggested if we want to avoid this we have to
make pcim_ variant of the API (see [1] again).

Maybe you, guys, should got some agreement and clarify it in the documentation?

[0]: We have a few functions with pcim_ prefix, few without and some from the
     latter group imply to behave _differently_ when pcim_enable_device() had
     been called.
[1]: I'm not able to find the archive of the mailing, but I remember that it
     was something like that IIRC during 8250_lpss.c development.
[2]: https://lore.kernel.org/linux-pci/1467621574-8277-1-git-send-email-hch@lst.de/
[3]: https://www.kernel.org/doc/html/latest/PCI/msi-howto.html#using-msi

> It *does* remove unnecessary pci_free_irq_vectors() calls from
> i2c-designware-pcidrv.c.
> 
> But because pci_alloc_irq_vectors() and related interfaces are
> *already* managed as soon as a driver calls pcim_enable_device(),
> we can simply remove the pci_free_irq_vectors() without doing anything
> else.
> 
> I don't think we *should* do anything else.

See above.

> There are many callers of
> pcim_enable_device() that also call pci_alloc_irq_vectors(),
> pci_enable_msix_range(), etc.  We don't have pcim_enable_msix_range(),
> pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc.  I don't
> think it's worth the churn of adding all those and changing all the
> callers to use pcim_*() (as in patch 4/4 here).
> 
> Browsing the output of this:
> 
>   git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"
> 
> leads me to believe there are similar calls of pci_free_irq_vectors()
> that could be removed here:
> 
>   mtip_pci_probe
>   sp_pci_probe
>   dw_edma_pcie_probe
>   hisi_dma_probe
>   ioat_pci_probe
>   plx_dma_probe
>   cci_pci_probe
>   hibmc_pci_probe
>   ...
> 
> and many more, but I got tired of looking.
> 
> > > > +/**
> > > > + * 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.
> > > > + */
> > > > +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))
> > > > +		return -EINVAL;
> > > > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > > 
> > > This is great, but can you explain how pci_alloc_irq_vectors()
> > > magically becomes a managed interface if we've already called
> > > pcim_enable_device()?
> > > 
> > > I certainly believe it does; I'd just like to put a hint in the commit
> > > log since my 5 minutes of grepping around didn't make it obvious to
> > > me.
> > > 
> > > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > > find the connection between that and pci_alloc_irq_vectors().
> > 
> > One needs to read and understand the code, I agree. The explanation is spread
> > between pcim_release() and __pci_enable_msi/x_range().
> > 
> > The call chain is
> > 
> > msi_capability_init() / msix_capability_init()
> >   ...
> >   <- __pci_enable_msi/x_range()
> >     <- pci_alloc_irq_vectors_affinity()
> >       <- pci_alloc_irq_vectors()
> > 
> > where device msi_enabled / msix_enabled is set.
> > 
> > So, it may deserve to be explained in the commit message.
> > 
> > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Bjorn Helgaas]

On Thu, Jun 17, 2021 at 04:20:16PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 16, 2021 at 02:25:43PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> > > 
> > > ...
> > > 
> > > > > @@ -1989,10 +1989,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);
> > > > 
> > > > If I understand correctly, this hunk is a nice simplification, but
> > > > actually has nothing to do with making pcim_alloc_irq_vectors().  I
> > > > have it split to a separate patch in my local tree.  Or am I wrong
> > > > about that?
> > > 
> > > It's a good simplification that had to be done when pci_free_irq_vectors()
> > > appeared.
> > 
> > Sorry to be pedantic.  You say the simplification "had to be done,"
> > but AFAICT there was no actual *requirement* for this simplification
> > to be done since pci_free_irq_vectors() is functionally identical to
> > the previous code.
> > I think we should do it because it's a little
> > simpler, but not because it *fixes* anything.
> 
> It makes things more straightforward. So it definitely "fixes" something, but
> not the code in this case, rather how we maintain this code.
> 
> > > But here is the fact that indirectly it's related to the pcim_*()
> > > APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> > > pcim_release().
> > 
> > Yes.  For posterity, my notes about the call chain (after applying
> > this patch):
> > 
> >   pci_alloc_irq_vectors
> >     pci_alloc_irq_vectors_affinity
> >       __pci_enable_msix_range                 # MSI-X path
> >         __pci_enable_msix
> >           msix_capability_init
> >             msix_setup_entries
> >               for (...)
> >                 entry = alloc_msi_entry
> >                   kzalloc(msi_desc)           <--- alloc
> >                   kmemdup(msi_desc->affinity) <--- alloc
> >             dev->msix_enabled = 1             # MSI-X enabled
> >       __pci_enable_msi_range                  # MSI path
> >         msi_capability_init
> >           msi_setup_entry
> >             alloc_msi_entry                   <--- alloc
> >           dev->msi_enabled = 1                # MSI enabled
> > 
> >   pcim_release
> >     pci_free_irq_vectors
> >       pci_disable_msix                        # MSI-X
> >         if (!dev->msix_enabled)
> >           return
> >         pci_msix_shutdown
> >           dev->msix_enabled = 0               # MSI-X disabled
> >         free_msi_irqs
> >           list_for_each_entry_safe(..., msi_list, ...)
> >             free_msi_entry
> >               kfree(msi_desc->affinity)       <--- free
> >               kfree(msi_desc)                 <--- free
> >       pci_disable_msi                         # MSI
> >         if (!dev->msi_enabled)
> >           return
> >         pci_msi_shutdown
> >           dev->msi_enabled = 0                # MSI disabled
> >         free_msi_irqs                         <--- free
> > 
> > So I *think* (correct me if I'm wrong):
> > 
> >   - If a driver calls pcim_enable_device(), we will call
> >     pcim_release() when the last reference to the device is dropped.
> > 
> >   - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
> >     structures via msix_setup_entries() or msi_setup_entry().
> > 
> >   - pcim_release() will free those msi_desc and irq_affinity_desc
> >     structures.
> > 
> >   - Even before this series, pcim_release() frees msi_desc and
> >     irq_affinity_desc structures by calling pci_disable_msi() and
> >     pci_disable_msix().
> > 
> >   - Calling pci_free_irq_vectors() (or pci_disable_msi() or
> >     pci_disable_msix()) twice is unnecessary but probably harmless
> >     because they bail out early.
> 
> > So this series actually does not fix any problems whatsoever.
> 
> I tend to disagree.
> 
> The PCI managed API is currently inconsistent and what you got is
> what I already know and had been using until (see below) Christoph
> told not to do [1].
> 
> Even do you as PCI maintainer it took some time to figure this out.
> But current APIs make it hard for mere users who wants to use it in
> the drivers.
> 
> So, main point of fix here is _API inconsistency_ [0].
> 
> But hey, I believe you have been Cc'ed to the initial submission of
> the pci_*_irq_vector*() rework done by Christoph [2] (hmm... don't
> see your name there). And he updated documentation as well [3].
> 
> Moreover, he insisted to use pci_free_irq_vectors() whenever we are
> using pci_alloc_irq_vectors(). And he suggested if we want to avoid
> this we have to make pcim_ variant of the API (see [1] again).

I'd like to consider this, but it's hard without a reference :)

I do think it would be helpful to have clear guidance about when
drivers need to use pci_free_irq_vectors().  The existing text in
msi-howto.rst doesn't address pcim_ at all.

> Maybe you, guys, should got some agreement and clarify it in the
> documentation?

I agree that the pcim_*() API is confusing at best and it would be
nice to improve it and document it, but I don't think this series
really does it.

There are several MSI-related interfaces that use alloc_msi_entry()
and hence magically become managed if we call pcim_enable_device():

  pci_alloc_irq_vectors()            # ~150 callers
  pci_alloc_irq_vectors_affinity()   #  ~10 callers
  pci_enable_msix_exact()            #  ~20 callers (deprecated)
  pci_enable_msix_range()            #  ~50 callers (deprecated)
  pci_enable_msi()                   # ~100 callers (deprecated)

This series adds pcim_alloc_irq_vectors(), which sort of fixes *one*
of them and makes this sequence look nice:

  pcim_enable_device();
  pcim_alloc_irq_vectors();

but all the others are still potentially managed even though the name
doesn't indicate it.  And it really doesn't improve the documentation.

Possible steps forward:

  - Add comments in include/linux/pci.h to indicate deprecation
    (AFAICS, deprecation is currently only mentioned in
    msi-howto.rst).

  - Migrate callers away from deprecated interfaces (a lot of work).

  - Remove deprecated interfaces.

  - Add pcim_ variants of remaining interfaces (I think only
    pci_alloc_*()).  Consider returning error for pci_alloc_*() usage
    by managed drivers.

  - Convert managed callers from pci_alloc_*() to pcim_alloc_*() and
    remove usage of pci_free_irq_vectors(), pci_disable_msi(),
    pci_disable_msix().

> [0]: We have a few functions with pcim_ prefix, few without and some from the
>      latter group imply to behave _differently_ when pcim_enable_device() had
>      been called.
> [1]: I'm not able to find the archive of the mailing, but I remember that it
>      was something like that IIRC during 8250_lpss.c development.
> [2]: https://lore.kernel.org/linux-pci/1467621574-8277-1-git-send-email-hch@lst.de/
> [3]: https://www.kernel.org/doc/html/latest/PCI/msi-howto.html#using-msi
> 
> > It *does* remove unnecessary pci_free_irq_vectors() calls from
> > i2c-designware-pcidrv.c.
> > 
> > But because pci_alloc_irq_vectors() and related interfaces are
> > *already* managed as soon as a driver calls pcim_enable_device(),
> > we can simply remove the pci_free_irq_vectors() without doing anything
> > else.
> > 
> > I don't think we *should* do anything else.
> 
> See above.
> 
> > There are many callers of
> > pcim_enable_device() that also call pci_alloc_irq_vectors(),
> > pci_enable_msix_range(), etc.  We don't have pcim_enable_msix_range(),
> > pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc.  I don't
> > think it's worth the churn of adding all those and changing all the
> > callers to use pcim_*() (as in patch 4/4 here).
> > 
> > Browsing the output of this:
> > 
> >   git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"
> > 
> > leads me to believe there are similar calls of pci_free_irq_vectors()
> > that could be removed here:
> > 
> >   mtip_pci_probe
> >   sp_pci_probe
> >   dw_edma_pcie_probe
> >   hisi_dma_probe
> >   ioat_pci_probe
> >   plx_dma_probe
> >   cci_pci_probe
> >   hibmc_pci_probe
> >   ...
> > 
> > and many more, but I got tired of looking.
> > 
> > > > > +/**
> > > > > + * 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.
> > > > > + */
> > > > > +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))
> > > > > +		return -EINVAL;
> > > > > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > > > 
> > > > This is great, but can you explain how pci_alloc_irq_vectors()
> > > > magically becomes a managed interface if we've already called
> > > > pcim_enable_device()?
> > > > 
> > > > I certainly believe it does; I'd just like to put a hint in the commit
> > > > log since my 5 minutes of grepping around didn't make it obvious to
> > > > me.
> > > > 
> > > > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > > > find the connection between that and pci_alloc_irq_vectors().
> > > 
> > > One needs to read and understand the code, I agree. The explanation is spread
> > > between pcim_release() and __pci_enable_msi/x_range().
> > > 
> > > The call chain is
> > > 
> > > msi_capability_init() / msix_capability_init()
> > >   ...
> > >   <- __pci_enable_msi/x_range()
> > >     <- pci_alloc_irq_vectors_affinity()
> > >       <- pci_alloc_irq_vectors()
> > > 
> > > where device msi_enabled / msix_enabled is set.
> > > 
> > > So, it may deserve to be explained in the commit message.
> > > 
> > > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Thu, Jun 17, 2021 at 10:58:23AM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 17, 2021 at 04:20:16PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 16, 2021 at 02:25:43PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.

...

> > > > > > @@ -1989,10 +1989,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);
> > > > > 
> > > > > If I understand correctly, this hunk is a nice simplification, but
> > > > > actually has nothing to do with making pcim_alloc_irq_vectors().  I
> > > > > have it split to a separate patch in my local tree.  Or am I wrong
> > > > > about that?
> > > > 
> > > > It's a good simplification that had to be done when pci_free_irq_vectors()
> > > > appeared.
> > > 
> > > Sorry to be pedantic.  You say the simplification "had to be done,"
> > > but AFAICT there was no actual *requirement* for this simplification
> > > to be done since pci_free_irq_vectors() is functionally identical to
> > > the previous code.
> > > I think we should do it because it's a little
> > > simpler, but not because it *fixes* anything.
> > 
> > It makes things more straightforward. So it definitely "fixes" something, but
> > not the code in this case, rather how we maintain this code.
> > 
> > > > But here is the fact that indirectly it's related to the pcim_*()
> > > > APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> > > > pcim_release().
> > > 
> > > Yes.  For posterity, my notes about the call chain (after applying
> > > this patch):
> > > 
> > >   pci_alloc_irq_vectors
> > >     pci_alloc_irq_vectors_affinity
> > >       __pci_enable_msix_range                 # MSI-X path
> > >         __pci_enable_msix
> > >           msix_capability_init
> > >             msix_setup_entries
> > >               for (...)
> > >                 entry = alloc_msi_entry
> > >                   kzalloc(msi_desc)           <--- alloc
> > >                   kmemdup(msi_desc->affinity) <--- alloc
> > >             dev->msix_enabled = 1             # MSI-X enabled
> > >       __pci_enable_msi_range                  # MSI path
> > >         msi_capability_init
> > >           msi_setup_entry
> > >             alloc_msi_entry                   <--- alloc
> > >           dev->msi_enabled = 1                # MSI enabled
> > > 
> > >   pcim_release
> > >     pci_free_irq_vectors
> > >       pci_disable_msix                        # MSI-X
> > >         if (!dev->msix_enabled)
> > >           return
> > >         pci_msix_shutdown
> > >           dev->msix_enabled = 0               # MSI-X disabled
> > >         free_msi_irqs
> > >           list_for_each_entry_safe(..., msi_list, ...)
> > >             free_msi_entry
> > >               kfree(msi_desc->affinity)       <--- free
> > >               kfree(msi_desc)                 <--- free
> > >       pci_disable_msi                         # MSI
> > >         if (!dev->msi_enabled)
> > >           return
> > >         pci_msi_shutdown
> > >           dev->msi_enabled = 0                # MSI disabled
> > >         free_msi_irqs                         <--- free
> > > 
> > > So I *think* (correct me if I'm wrong):
> > > 
> > >   - If a driver calls pcim_enable_device(), we will call
> > >     pcim_release() when the last reference to the device is dropped.
> > > 
> > >   - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
> > >     structures via msix_setup_entries() or msi_setup_entry().
> > > 
> > >   - pcim_release() will free those msi_desc and irq_affinity_desc
> > >     structures.
> > > 
> > >   - Even before this series, pcim_release() frees msi_desc and
> > >     irq_affinity_desc structures by calling pci_disable_msi() and
> > >     pci_disable_msix().
> > > 
> > >   - Calling pci_free_irq_vectors() (or pci_disable_msi() or
> > >     pci_disable_msix()) twice is unnecessary but probably harmless
> > >     because they bail out early.
> > 
> > > So this series actually does not fix any problems whatsoever.
> > 
> > I tend to disagree.
> > 
> > The PCI managed API is currently inconsistent and what you got is
> > what I already know and had been using until (see below) Christoph
> > told not to do [1].
> > 
> > Even do you as PCI maintainer it took some time to figure this out.
> > But current APIs make it hard for mere users who wants to use it in
> > the drivers.
> > 
> > So, main point of fix here is _API inconsistency_ [0].
> > 
> > But hey, I believe you have been Cc'ed to the initial submission of
> > the pci_*_irq_vector*() rework done by Christoph [2] (hmm... don't
> > see your name there). And he updated documentation as well [3].
> > 
> > Moreover, he insisted to use pci_free_irq_vectors() whenever we are
> > using pci_alloc_irq_vectors(). And he suggested if we want to avoid
> > this we have to make pcim_ variant of the API (see [1] again).
> 
> I'd like to consider this, but it's hard without a reference :)

Sorry it took a bit too long to answer here.

https://lore.kernel.org/linux-serial/0250c46e-da6c-71f3-50ae-b7c17fd0bd2c@siemens.com/T/#u

> I do think it would be helpful to have clear guidance about when
> drivers need to use pci_free_irq_vectors().  The existing text in
> msi-howto.rst doesn't address pcim_ at all.

Christoph suggested to create an explicit managed API. For now it may be
looking the same as non-managed, but by design it will require different
approaches in case it divert.

> > Maybe you, guys, should got some agreement and clarify it in the
> > documentation?
> 
> I agree that the pcim_*() API is confusing at best and it would be
> nice to improve it and document it, but I don't think this series
> really does it.
> 
> There are several MSI-related interfaces that use alloc_msi_entry()
> and hence magically become managed if we call pcim_enable_device():
> 
>   pci_alloc_irq_vectors()            # ~150 callers
>   pci_alloc_irq_vectors_affinity()   #  ~10 callers
>   pci_enable_msix_exact()            #  ~20 callers (deprecated)
>   pci_enable_msix_range()            #  ~50 callers (deprecated)
>   pci_enable_msi()                   # ~100 callers (deprecated)
> 
> This series adds pcim_alloc_irq_vectors(), which sort of fixes *one*
> of them and makes this sequence look nice:
> 
>   pcim_enable_device();
>   pcim_alloc_irq_vectors();
> 
> but all the others are still potentially managed even though the name
> doesn't indicate it.

> And it really doesn't improve the documentation.
> 
> Possible steps forward:
> 
>   - Add comments in include/linux/pci.h to indicate deprecation
>     (AFAICS, deprecation is currently only mentioned in
>     msi-howto.rst).
> 
>   - Migrate callers away from deprecated interfaces (a lot of work).

AFAIU you the proposal is to convert all drivers to use explicit
error handling for pci_alloc_irq_vectors() and then undo that after
introducing pcim_alloc_irq_vectors().

Why not having less churn by applying this series and then clean up things?
We have a lot of unneeded churn now because of this rather lexicographical
issue.

>   - Remove deprecated interfaces.
> 
>   - Add pcim_ variants of remaining interfaces (I think only
>     pci_alloc_*()).  Consider returning error for pci_alloc_*() usage
>     by managed drivers.
> 
>   - Convert managed callers from pci_alloc_*() to pcim_alloc_*() and
>     remove usage of pci_free_irq_vectors(), pci_disable_msi(),
>     pci_disable_msix().

Seems to me a lot of useless churn, but maybe I'm missing the point?

> > [0]: We have a few functions with pcim_ prefix, few without and some from the
> >      latter group imply to behave _differently_ when pcim_enable_device() had
> >      been called.
> > [1]: I'm not able to find the archive of the mailing, but I remember that it
> >      was something like that IIRC during 8250_lpss.c development.
> > [2]: https://lore.kernel.org/linux-pci/1467621574-8277-1-git-send-email-hch@lst.de/
> > [3]: https://www.kernel.org/doc/html/latest/PCI/msi-howto.html#using-msi
> > 
> > > It *does* remove unnecessary pci_free_irq_vectors() calls from
> > > i2c-designware-pcidrv.c.
> > > 
> > > But because pci_alloc_irq_vectors() and related interfaces are
> > > *already* managed as soon as a driver calls pcim_enable_device(),
> > > we can simply remove the pci_free_irq_vectors() without doing anything
> > > else.
> > > 
> > > I don't think we *should* do anything else.
> > 
> > See above.
> > 
> > > There are many callers of
> > > pcim_enable_device() that also call pci_alloc_irq_vectors(),
> > > pci_enable_msix_range(), etc.  We don't have pcim_enable_msix_range(),
> > > pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc.  I don't
> > > think it's worth the churn of adding all those and changing all the
> > > callers to use pcim_*() (as in patch 4/4 here).
> > > 
> > > Browsing the output of this:
> > > 
> > >   git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"
> > > 
> > > leads me to believe there are similar calls of pci_free_irq_vectors()
> > > that could be removed here:
> > > 
> > >   mtip_pci_probe
> > >   sp_pci_probe
> > >   dw_edma_pcie_probe
> > >   hisi_dma_probe
> > >   ioat_pci_probe
> > >   plx_dma_probe
> > >   cci_pci_probe
> > >   hibmc_pci_probe
> > >   ...
> > > 
> > > and many more, but I got tired of looking.
> > > 
> > > > > > +/**
> > > > > > + * 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.
> > > > > > + */
> > > > > > +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))
> > > > > > +		return -EINVAL;
> > > > > > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > > > > 
> > > > > This is great, but can you explain how pci_alloc_irq_vectors()
> > > > > magically becomes a managed interface if we've already called
> > > > > pcim_enable_device()?
> > > > > 
> > > > > I certainly believe it does; I'd just like to put a hint in the commit
> > > > > log since my 5 minutes of grepping around didn't make it obvious to
> > > > > me.
> > > > > 
> > > > > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > > > > find the connection between that and pci_alloc_irq_vectors().
> > > > 
> > > > One needs to read and understand the code, I agree. The explanation is spread
> > > > between pcim_release() and __pci_enable_msi/x_range().
> > > > 
> > > > The call chain is
> > > > 
> > > > msi_capability_init() / msix_capability_init()
> > > >   ...
> > > >   <- __pci_enable_msi/x_range()
> > > >     <- pci_alloc_irq_vectors_affinity()
> > > >       <- pci_alloc_irq_vectors()
> > > > 
> > > > where device msi_enabled / msix_enabled is set.
> > > > 
> > > > So, it may deserve to be explained in the commit message.
> > > > 
> > > > > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

On Thu, Jun 17, 2021 at 04:20:16PM +0300, Andy Shevchenko wrote:
> Dejin, why Christoph's email suddenly disappeared during updating?
>
Due to my negligence, I forgot to add Christoph to mail list.

Hi Christoph,

I can't tell you how sorry I am, I will be more careful in the future��

BR��
Dejin

> On Wed, Jun 16, 2021 at 02:25:43PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 11, 2021 at 12:37:22PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 10, 2021 at 05:41:43PM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.
> > > 
> > > ...
> > > 
> > > > > @@ -1989,10 +1989,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);
> > > > 
> > > > If I understand correctly, this hunk is a nice simplification, but
> > > > actually has nothing to do with making pcim_alloc_irq_vectors().  I
> > > > have it split to a separate patch in my local tree.  Or am I wrong
> > > > about that?
> > > 
> > > It's a good simplification that had to be done when pci_free_irq_vectors()
> > > appeared.
> > 
> > Sorry to be pedantic.  You say the simplification "had to be done,"
> > but AFAICT there was no actual *requirement* for this simplification
> > to be done since pci_free_irq_vectors() is functionally identical to
> > the previous code.
> > I think we should do it because it's a little
> > simpler, but not because it *fixes* anything.
> 
> It makes things more straightforward. So it definitely "fixes" something, but
> not the code in this case, rather how we maintain this code.
> 
> > > But here is the fact that indirectly it's related to the pcim_*()
> > > APIs, i.e. pcim_alloc_irq_vectors(), because you may noticed this is inside
> > > pcim_release().
> > 
> > Yes.  For posterity, my notes about the call chain (after applying
> > this patch):
> > 
> >   pci_alloc_irq_vectors
> >     pci_alloc_irq_vectors_affinity
> >       __pci_enable_msix_range                 # MSI-X path
> >         __pci_enable_msix
> >           msix_capability_init
> >             msix_setup_entries
> >               for (...)
> >                 entry = alloc_msi_entry
> >                   kzalloc(msi_desc)           <--- alloc
> >                   kmemdup(msi_desc->affinity) <--- alloc
> >             dev->msix_enabled = 1             # MSI-X enabled
> >       __pci_enable_msi_range                  # MSI path
> >         msi_capability_init
> >           msi_setup_entry
> >             alloc_msi_entry                   <--- alloc
> >           dev->msi_enabled = 1                # MSI enabled
> > 
> >   pcim_release
> >     pci_free_irq_vectors
> >       pci_disable_msix                        # MSI-X
> >         if (!dev->msix_enabled)
> >           return
> >         pci_msix_shutdown
> >           dev->msix_enabled = 0               # MSI-X disabled
> >         free_msi_irqs
> >           list_for_each_entry_safe(..., msi_list, ...)
> >             free_msi_entry
> >               kfree(msi_desc->affinity)       <--- free
> >               kfree(msi_desc)                 <--- free
> >       pci_disable_msi                         # MSI
> >         if (!dev->msi_enabled)
> >           return
> >         pci_msi_shutdown
> >           dev->msi_enabled = 0                # MSI disabled
> >         free_msi_irqs                         <--- free
> > 
> > So I *think* (correct me if I'm wrong):
> > 
> >   - If a driver calls pcim_enable_device(), we will call
> >     pcim_release() when the last reference to the device is dropped.
> > 
> >   - pci_alloc_irq_vectors() allocates msi_desc and irq_affinity_desc
> >     structures via msix_setup_entries() or msi_setup_entry().
> > 
> >   - pcim_release() will free those msi_desc and irq_affinity_desc
> >     structures.
> > 
> >   - Even before this series, pcim_release() frees msi_desc and
> >     irq_affinity_desc structures by calling pci_disable_msi() and
> >     pci_disable_msix().
> > 
> >   - Calling pci_free_irq_vectors() (or pci_disable_msi() or
> >     pci_disable_msix()) twice is unnecessary but probably harmless
> >     because they bail out early.
> 
> > So this series actually does not fix any problems whatsoever.
> 
> I tend to disagree.
> 
> The PCI managed API is currently inconsistent and what you got is what I
> already know and had been using until (see below) Christoph told not to do [1].
> 
> Even do you as PCI maintainer it took some time to figure this out. But current
> APIs make it hard for mere users who wants to use it in the drivers.
> 
> So, main point of fix here is _API inconsistency_ [0].
> 
> But hey, I believe you have been Cc'ed to the initial submission of the
> pci_*_irq_vector*() rework done by Christoph [2] (hmm... don't see your
> name there). And he updated documentation as well [3].
> 
> Moreover, he insisted to use pci_free_irq_vectors() whenever we are using
> pci_alloc_irq_vectors(). And he suggested if we want to avoid this we have to
> make pcim_ variant of the API (see [1] again).
> 
> Maybe you, guys, should got some agreement and clarify it in the documentation?
> 
> [0]: We have a few functions with pcim_ prefix, few without and some from the
>      latter group imply to behave _differently_ when pcim_enable_device() had
>      been called.
> [1]: I'm not able to find the archive of the mailing, but I remember that it
>      was something like that IIRC during 8250_lpss.c development.
> [2]: https://lore.kernel.org/linux-pci/1467621574-8277-1-git-send-email-hch@lst.de/
> [3]: https://www.kernel.org/doc/html/latest/PCI/msi-howto.html#using-msi
> 
> > It *does* remove unnecessary pci_free_irq_vectors() calls from
> > i2c-designware-pcidrv.c.
> > 
> > But because pci_alloc_irq_vectors() and related interfaces are
> > *already* managed as soon as a driver calls pcim_enable_device(),
> > we can simply remove the pci_free_irq_vectors() without doing anything
> > else.
> > 
> > I don't think we *should* do anything else.
> 
> See above.
> 
> > There are many callers of
> > pcim_enable_device() that also call pci_alloc_irq_vectors(),
> > pci_enable_msix_range(), etc.  We don't have pcim_enable_msix_range(),
> > pcim_enable_msi(), pcim_alloc_irq_vectors_affinity(), etc.  I don't
> > think it's worth the churn of adding all those and changing all the
> > callers to use pcim_*() (as in patch 4/4 here).
> > 
> > Browsing the output of this:
> > 
> >   git grep -En "pcim_enable_device|pci_alloc_irq_vectors|pci_enable_msix_|pci_free_irq_vectors|pci_disable_msi"
> > 
> > leads me to believe there are similar calls of pci_free_irq_vectors()
> > that could be removed here:
> > 
> >   mtip_pci_probe
> >   sp_pci_probe
> >   dw_edma_pcie_probe
> >   hisi_dma_probe
> >   ioat_pci_probe
> >   plx_dma_probe
> >   cci_pci_probe
> >   hibmc_pci_probe
> >   ...
> > 
> > and many more, but I got tired of looking.
> > 
> > > > > +/**
> > > > > + * 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.
> > > > > + */
> > > > > +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))
> > > > > +		return -EINVAL;
> > > > > +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> > > > 
> > > > This is great, but can you explain how pci_alloc_irq_vectors()
> > > > magically becomes a managed interface if we've already called
> > > > pcim_enable_device()?
> > > > 
> > > > I certainly believe it does; I'd just like to put a hint in the commit
> > > > log since my 5 minutes of grepping around didn't make it obvious to
> > > > me.
> > > > 
> > > > I see that pcim_enable_device() sets pdev->is_managed, but I didn't
> > > > find the connection between that and pci_alloc_irq_vectors().
> > > 
> > > One needs to read and understand the code, I agree. The explanation is spread
> > > between pcim_release() and __pci_enable_msi/x_range().
> > > 
> > > The call chain is
> > > 
> > > msi_capability_init() / msix_capability_init()
> > >   ...
> > >   <- __pci_enable_msi/x_range()
> > >     <- pci_alloc_irq_vectors_affinity()
> > >       <- pci_alloc_irq_vectors()
> > > 
> > > where device msi_enabled / msix_enabled is set.
> > > 
> > > So, it may deserve to be explained in the commit message.
> > > 
> > > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v7 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Christoph Hellwig]

On Mon, Jun 07, 2021 at 11:39:13PM +0800, 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.

How does it do that when it just is a trivial wrapper erroring out
for the unmanaged case?

[PATCH v7 2/4] Documentation: devres: Add pcim_alloc_irq_vectors()

[Dejin Zheng]

Add 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.

Reviewed-by: Robert Richter <rric@kernel.org>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v6 -> v7:
	- rebase to PCI next branch
v5 -> v6:
	- rebase to 5.13-rc4
v4 -> v5:
	- No change
v3 -> v4:
	- No change
v2 -> v3:
	- No change
v1 -> v2:
	- Modify some commit messages.

 Documentation/driver-api/driver-model/devres.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index e0814d214048..fad7d26ccf35 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -382,6 +382,7 @@ PCI
   devm_pci_alloc_host_bridge()  : managed PCI host bridge allocation
   devm_pci_remap_cfgspace()	: ioremap PCI configuration space
   devm_pci_remap_cfg_resource()	: ioremap PCI configuration space resource
+  pcim_alloc_irq_vectors()      : managed IRQ vectors allocation
   pcim_enable_device()		: after success, all PCI ops become managed
   pcim_pin_device()		: keep PCI device enabled after release
 
-- 
2.30.1

Re: [PATCH v7 2/4] Documentation: devres: Add pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Mon, Jun 07, 2021 at 11:39:14PM +0800, Dejin Zheng wrote:
> Add 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.

This is good one, thanks.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Reviewed-by: Robert Richter <rric@kernel.org>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> v6 -> v7:
> 	- rebase to PCI next branch
> v5 -> v6:
> 	- rebase to 5.13-rc4
> v4 -> v5:
> 	- No change
> v3 -> v4:
> 	- No change
> v2 -> v3:
> 	- No change
> v1 -> v2:
> 	- Modify some commit messages.
> 
>  Documentation/driver-api/driver-model/devres.rst | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index e0814d214048..fad7d26ccf35 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -382,6 +382,7 @@ PCI
>    devm_pci_alloc_host_bridge()  : managed PCI host bridge allocation
>    devm_pci_remap_cfgspace()	: ioremap PCI configuration space
>    devm_pci_remap_cfg_resource()	: ioremap PCI configuration space resource
> +  pcim_alloc_irq_vectors()      : managed IRQ vectors allocation
>    pcim_enable_device()		: after success, all PCI ops become managed
>    pcim_pin_device()		: keep PCI device enabled after release
>  
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v7 3/4] i2c: designware: Use pcim_alloc_irq_vectors() to allocate IRQ vectors

[Dejin Zheng]

The pcim_alloc_irq_vectors() function, an explicit device-managed version
of pci_alloc_irq_vectors(). If pcim_enable_device() has been called
before, then pci_alloc_irq_vectors() is actually a device-managed
function. It is used here as a device-managed function, So replace it
with pcim_alloc_irq_vectors(). At the same time, Remove the
pci_free_irq_vectors() function to simplify the error handling path.
the freeing resources will take automatically when device is gone.

Reviewed-by: Robert Richter <rric@kernel.org>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v6 -> v7:
	- rebase to PCI next branch
v5 -> v6:
	- rebase to 5.13-rc4
v4 -> v5:
	- Modify the subject name.
v3 -> v4:
	- add some commit comments.
v2 -> v3:
	- simplify the error handling path.
v1 -> v2:
	- Modify some commit messages.

 drivers/i2c/busses/i2c-designware-pcidrv.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 0f409a4c2da0..2b1ef0934445 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -268,7 +268,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	if (!dev)
 		return -ENOMEM;
 
-	r = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+	r = pcim_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
 	if (r < 0)
 		return r;
 
@@ -283,10 +283,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	if (controller->setup) {
 		r = controller->setup(pdev, controller);
-		if (r) {
-			pci_free_irq_vectors(pdev);
+		if (r)
 			return r;
-		}
 	}
 
 	i2c_dw_adjust_bus_speed(dev);
@@ -295,10 +293,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 		i2c_dw_acpi_configure(&pdev->dev);
 
 	r = i2c_dw_validate_speed(dev);
-	if (r) {
-		pci_free_irq_vectors(pdev);
+	if (r)
 		return r;
-	}
 
 	i2c_dw_configure(dev);
 
@@ -318,10 +314,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	adap->nr = controller->bus_num;
 
 	r = i2c_dw_probe(dev);
-	if (r) {
-		pci_free_irq_vectors(pdev);
+	if (r)
 		return r;
-	}
 
 	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
 		r = navi_amd_register_client(dev);
@@ -349,7 +343,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
 
 	i2c_del_adapter(&dev->adapter);
 	devm_free_irq(&pdev->dev, dev->irq, dev);
-	pci_free_irq_vectors(pdev);
 }
 
 /* work with hotplug and coldplug */
-- 
2.30.1

Re: [PATCH v7 3/4] i2c: designware: Use pcim_alloc_irq_vectors() to allocate IRQ vectors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Mon, Jun 07, 2021 at 11:39:15PM +0800, Dejin Zheng wrote:
> The pcim_alloc_irq_vectors() function, an explicit device-managed version
> of pci_alloc_irq_vectors(). If pcim_enable_device() has been called
> before, then pci_alloc_irq_vectors() is actually a device-managed
> function. It is used here as a device-managed function, So replace it
> with pcim_alloc_irq_vectors(). At the same time, Remove the
> pci_free_irq_vectors() function to simplify the error handling path.
> the freeing resources will take automatically when device is gone.
> 
> Reviewed-by: Robert Richter <rric@kernel.org>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

Dunno the status of this series, but if it ever goes upstream, I am fine
with the I2C changes:

Acked-by: Wolfram Sang <wsa@kernel.org> # for I2C


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v7 4/4] i2c: thunderx: Use pcim_alloc_irq_vectors() to allocate IRQ vectors

[Dejin Zheng]

The pcim_alloc_irq_vectors() function, an explicit device-managed version
of pci_alloc_irq_vectors(). If pcim_enable_device() has been called
before, then pci_alloc_irq_vectors() is actually a device-managed
function. It is used here as a device-managed function, So replace it
with pcim_alloc_irq_vectors().

Acked-by: Robert Richter <rric@kernel.org>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v6 -> v7:
	- rebase to PCI next branch
v5 -> v6:
	- rebase to 5.13-rc4
v4 -> v5:
	- Modify the subject name.
v3 -> v4:
	- No change.
v2 -> v3:
	- No change.
v1 -> v2:
	- Modify some commit messages.

 drivers/i2c/busses/i2c-thunderx-pcidrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-thunderx-pcidrv.c b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
index 12c90aa0900e..63354e9fb726 100644
--- a/drivers/i2c/busses/i2c-thunderx-pcidrv.c
+++ b/drivers/i2c/busses/i2c-thunderx-pcidrv.c
@@ -192,7 +192,7 @@ static int thunder_i2c_probe_pci(struct pci_dev *pdev,
 	i2c->hlc_int_enable = thunder_i2c_hlc_int_enable;
 	i2c->hlc_int_disable = thunder_i2c_hlc_int_disable;
 
-	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
+	ret = pcim_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
 	if (ret < 0)
 		goto error;
 
-- 
2.30.1

Re: [PATCH v7 4/4] i2c: thunderx: Use pcim_alloc_irq_vectors() to allocate IRQ vectors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Mon, Jun 07, 2021 at 11:39:16PM +0800, Dejin Zheng wrote:
> The pcim_alloc_irq_vectors() function, an explicit device-managed version
> of pci_alloc_irq_vectors(). If pcim_enable_device() has been called
> before, then pci_alloc_irq_vectors() is actually a device-managed
> function. It is used here as a device-managed function, So replace it
> with pcim_alloc_irq_vectors().
> 
> Acked-by: Robert Richter <rric@kernel.org>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

Acked-by: Wolfram Sang <wsa@kernel.org> # for I2C


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]