[PATCH v4 0/4] Introduce pcim_alloc_irq_vectors()

[PATCH v4 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
---------
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 the correct name of device-managed function
  i2c: thunderx: Use the correct name of device-managed function

 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/i2c/busses/i2c-designware-pcidrv.c    | 15 +++------
 drivers/i2c/busses/i2c-thunderx-pcidrv.c      |  2 +-
 drivers/pci/pci.c                             | 33 ++++++++++++++++---
 include/linux/pci.h                           |  3 ++
 5 files changed, 38 insertions(+), 16 deletions(-)

-- 
2.25.0

[PATCH v4 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.

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);
+
 /*
  * 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>
-- 
2.25.0

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

[Robert Richter]

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
> 

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

[Robert Richter]

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);
> }

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

[Krzysztof Wilczyński]

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

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

[Robert Richter]

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

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

[Andy Shevchenko]

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().

-- 
With Best Regards,
Andy Shevchenko

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

[Robert Richter]

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

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

[Dejin Zheng]

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

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

[Robert Richter]

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

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

[Dejin Zheng]

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

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

[Robert Richter]

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

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

[Dejin Zheng]

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

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

[Robert Richter]

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

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

[Dejin Zheng]

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

[PATCH v4 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.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
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 cd8b6e657b94..a52f65b6352f 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -380,6 +380,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.25.0

[PATCH v4 3/4] i2c: designware: Use the correct name of device-managed function

[Dejin Zheng]

Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
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.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
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 55c83a7a24f3..620b41e373b6 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -219,7 +219,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;
 
@@ -234,10 +234,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);
@@ -246,10 +244,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);
 
@@ -269,10 +265,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;
-	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 	pm_runtime_use_autosuspend(&pdev->dev);
@@ -292,7 +286,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.25.0

Re: [PATCH v4 3/4] i2c: designware: Use the correct name of device-managed function

[Andy Shevchenko]

On Thu, Feb 18, 2021 at 11:04:57PM +0800, Dejin Zheng wrote:
> Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
> 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.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
One side note below.

> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> 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 55c83a7a24f3..620b41e373b6 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -219,7 +219,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;
>  
> @@ -234,10 +234,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);
> @@ -246,10 +244,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);
>  
> @@ -269,10 +265,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;
> -	}
>  
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>  	pm_runtime_use_autosuspend(&pdev->dev);
> @@ -292,7 +286,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);

I'm wondering if we need explicit devm_free_irq() call now. If we don't, it can
be cleaned up in the separate change with a motivation that we got to device
managed PCI IRQ allocation and thus the ordering will be correct (needs a
double check!).

>  }
>  
>  /* work with hotplug and coldplug */
> -- 
> 2.25.0
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 4/4] i2c: thunderx: Use the correct name of device-managed function

[Dejin Zheng]

Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
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().

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
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.25.0

Re: [PATCH v4 4/4] i2c: thunderx: Use the correct name of device-managed function

[Robert Richter]

On 18.02.21 23:04:58, Dejin Zheng wrote:
> Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
> 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().
> 
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

The title of this patch suggests there was something incorrect before
and this is a fix, which it isn't. Please reword.

Thanks,

-Robert