[PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

[PATCH v3 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
---------
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 v3 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>
---
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 v3 1/4] PCI: Introduce pcim_alloc_irq_vectors()

[Krzysztof Wilczyński]

Hi Dejin,

> 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.
[...]

Some suggestions about the commit message as per:

  https://lore.kernel.org/linux-pci/YCwE2cf9X%2FGd6lWy@rocinante/

> +/**
> + * 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);
[...]

Looks good!  Thank you for adding kernel-doc here!  Much appreciated.

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof

[PATCH v3 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>
---
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

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

[Krzysztof Wilczyński]

Hi Dejin,

Thank you again for all the work here!

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

The second sentence should most likely start with a capital letter.

Having said that, people might ask - how does it simplify the error
handling path?

You might have to back this with a line of two to explain how does the
change achieved that, so that when someone looks at the commit message
it would be clear what the benefits of the change were.

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof

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

[Dejin Zheng]

On Tue, Feb 16, 2021 at 06:10:52PM +0100, Krzysztof Wilczyński wrote:
> Hi Dejin,
> 
> Thank you again for all the work here!
> 
> > 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.
> 
> The second sentence should most likely start with a capital letter.
> 
> Having said that, people might ask - how does it simplify the error
> handling path?
> 
> You might have to back this with a line of two to explain how does the
> change achieved that, so that when someone looks at the commit message
> it would be clear what the benefits of the change were.
>
Hi Krzysztof,

The device-managed function is a conventional concept that every developer
knows. So don't worry about this. And I really can't explain its operation
mechanism to you in a sentence or two. If you are really interested, you
can read the relevant code.

I think it is meaningless to add a lot of explanations of general
concepts in the commit comments. Maybe it will be better let us put more
energy and time on the code.

BR,
Dejin

> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> Krzysztof

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

[Andy Shevchenko]

On Wed, Feb 17, 2021 at 06:50:04PM +0800, Dejin Zheng wrote:
> On Tue, Feb 16, 2021 at 06:10:52PM +0100, Krzysztof Wilczyński wrote:

...

> > Having said that, people might ask - how does it simplify the error
> > handling path?
> > 
> > You might have to back this with a line of two to explain how does the
> > change achieved that, so that when someone looks at the commit message
> > it would be clear what the benefits of the change were.

> The device-managed function is a conventional concept that every developer
> knows. So don't worry about this. And I really can't explain its operation
> mechanism to you in a sentence or two. If you are really interested, you
> can read the relevant code.

I tend on agree on the above. It would be enough to spell it clearly that it's
part of devres API (Managed Device Resource) and we are fine.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 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, simplify the error
handling path.

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
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 v3 3/4] i2c: designware: Use the correct name of device-managed function

[Krzysztof Wilczyński]

Hi Dejin,

Thank you for all the changes, looks good!

You could improve the subject line, as it is very vague - what is the
new function name more correct?  Was the other and/or the previous one
not correct?  Seems like you are correcting a typo of sorts, rather than
introducing a new function in this file.

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

The commit is good, but it could use some polish, so to speak.

A few suggestions to think about:

  - What are we adding and/or changing, and why
  - Why is using pcim_alloc_irq_vectors(), which is part
    of the managed devices framework, a better alternative
    to the pci_alloc_irq_vectors()
  - And finally why this change allowed us to remove the
    pci_free_irq_vectors()

> At the same time, simplify the error handling path.

The change simplifies the error handling path, how?  A line of two which
explains how it has been achieved might help should someone reads the
commit message in the future.

[...]
>  	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);

If pcim_release() is called should the pci_driver's probe callback fail,
and I assume that this is precisely the case, then all of the above make
sense in the view of using pcim_alloc_irq_vectors().

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof

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

[Dejin Zheng]

On Tue, Feb 16, 2021 at 06:46:01PM +0100, Krzysztof Wilczyński wrote:
Hi Krzysztof,
> Hi Dejin,
> 
> Thank you for all the changes, looks good!
> 
> You could improve the subject line, as it is very vague - what is the
> new function name more correct?  Was the other and/or the previous one
> not correct?  Seems like you are correcting a typo of sorts, rather than
> introducing a new function in this file.
>
If you have read the following commit comments, As you know, the
pci_alloc_irq_vectors() is not a real device-managed function. But
in some specific cases, it will act as an device-managed function.
Such naming will cause controversy, So In the case of need device-managed,
should be used pcim_alloc_irq_vectors(), an explicit device-managed
function. So the subject name is "Use the correct name of device-managed function".

> > 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().
> 
> The commit is good, but it could use some polish, so to speak.
> 
> A few suggestions to think about:
> 
>   - What are we adding and/or changing, and why
>   - Why is using pcim_alloc_irq_vectors(), which is part
>     of the managed devices framework, a better alternative
>     to the pci_alloc_irq_vectors()
>   - And finally why this change allowed us to remove the
>     pci_free_irq_vectors()
> 
These are all explained by the device-managed function mechanism.

> > At the same time, simplify the error handling path.
> 
> The change simplifies the error handling path, how?  A line of two which
> explains how it has been achieved might help should someone reads the
> commit message in the future.
> 
To put it simply, if the driver probe fail, the device-managed function
mechanism will automatically call pcim_release(), then the pci_free_irq_vectors()
will be executed. For details, please see the relevant code.

> [...]
> >  	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);
> 
> If pcim_release() is called should the pci_driver's probe callback fail,
Yes, you guessed right.

> and I assume that this is precisely the case, then all of the above make
> sense in the view of using pcim_alloc_irq_vectors().
> 
> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> Krzysztof

BR,
Dejin

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

[Andy Shevchenko]

On Wed, Feb 17, 2021 at 07:40:14PM +0800, Dejin Zheng wrote:
> On Tue, Feb 16, 2021 at 06:46:01PM +0100, Krzysztof Wilczyński wrote:

...

> > The change simplifies the error handling path, how?  A line of two which
> > explains how it has been achieved might help should someone reads the
> > commit message in the future.
> > 
> To put it simply, if the driver probe fail, the device-managed function
> mechanism will automatically call pcim_release(), then the pci_free_irq_vectors()
> will be executed. For details, please see the relevant code.

Perhaps as a compromise you may add this short sentence to your commit
messages, like "the freeing resources will take automatically when device
is gone".

-- 
With Best Regards,
Andy Shevchenko

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

[Dejin Zheng]

On Wed, Feb 17, 2021 at 03:47:01PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 17, 2021 at 07:40:14PM +0800, Dejin Zheng wrote:
> > On Tue, Feb 16, 2021 at 06:46:01PM +0100, Krzysztof Wilczyński wrote:
> 
> ...
> 
> > > The change simplifies the error handling path, how?  A line of two which
> > > explains how it has been achieved might help should someone reads the
> > > commit message in the future.
> > > 
> > To put it simply, if the driver probe fail, the device-managed function
> > mechanism will automatically call pcim_release(), then the pci_free_irq_vectors()
> > will be executed. For details, please see the relevant code.
> 
> Perhaps as a compromise you may add this short sentence to your commit
> messages, like "the freeing resources will take automatically when device
> is gone".
>
Ok, I will do it. Andy, Thanks for your help. And so sorry for the late
reply. Yesterday was the last day of the Chinese New Year holiday. There
were a lot of things to deal with.

BR,
Dejin
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Krzysztof Wilczyński]

Hi Dejin,

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

A few suggestions about the commit message in the following reply:

  https://lore.kernel.org/linux-pci/YCwE2cf9X%2FGd6lWy@rocinante/

Krzysztof

Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

[Robert Richter]

On 17.02.21 00:02:45, Dejin Zheng wrote:
> Introduce pcim_alloc_irq_vectors(), a device-managed version of
> pci_alloc_irq_vectors(), In some i2c drivers, If pcim_enable_device()
> has been called before, then pci_alloc_irq_vectors() is actually a
> device-managed function. It is used as a device-managed function, So
> replace it with pcim_alloc_irq_vectors().
> 
> Changelog
> ---------
> v2 -> v3:
> 	- Add some commit comments for replace some codes in
> 	  pcim_release() by pci_free_irq_vectors().
> 	- Simplify the error handling path in i2c designware
> 	  driver.
> v1 -> v2:
> 	- Use pci_free_irq_vectors() to replace some code in
> 	  pcim_release().
> 	- Modify some commit messages.
> 
> Dejin Zheng (4):
>   PCI: Introduce pcim_alloc_irq_vectors()
>   Documentation: devres: Add pcim_alloc_irq_vectors()

This is already taken care of, see pcim_release():

        if (dev->msi_enabled)
                pci_disable_msi(dev);
        if (dev->msix_enabled)
                pci_disable_msix(dev);

Activated when used with pcim_enable_device().

This series is not required.

-Robert

>   i2c: designware: Use the correct name of device-managed function
>   i2c: thunderx: Use the correct name of device-managed function
> 
>  .../driver-api/driver-model/devres.rst        |  1 +
>  drivers/i2c/busses/i2c-designware-pcidrv.c    | 15 +++------
>  drivers/i2c/busses/i2c-thunderx-pcidrv.c      |  2 +-
>  drivers/pci/pci.c                             | 33 ++++++++++++++++---
>  include/linux/pci.h                           |  3 ++
>  5 files changed, 38 insertions(+), 16 deletions(-)
> 
> -- 
> 2.25.0
> 

Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Thu, Feb 18, 2021 at 10:36:28AM +0100, Robert Richter wrote:
> On 17.02.21 00:02:45, Dejin Zheng wrote:
> > Introduce pcim_alloc_irq_vectors(), a device-managed version of
> > pci_alloc_irq_vectors(), In some i2c drivers, If pcim_enable_device()
> > has been called before, then pci_alloc_irq_vectors() is actually a
> > device-managed function. It is used as a device-managed function, So
> > replace it with pcim_alloc_irq_vectors().
> > 
> > Changelog
> > ---------
> > v2 -> v3:
> > 	- Add some commit comments for replace some codes in
> > 	  pcim_release() by pci_free_irq_vectors().
> > 	- Simplify the error handling path in i2c designware
> > 	  driver.
> > v1 -> v2:
> > 	- Use pci_free_irq_vectors() to replace some code in
> > 	  pcim_release().
> > 	- Modify some commit messages.
> > 
> > Dejin Zheng (4):
> >   PCI: Introduce pcim_alloc_irq_vectors()
> >   Documentation: devres: Add pcim_alloc_irq_vectors()
> 
> This is already taken care of, see pcim_release():
> 
>         if (dev->msi_enabled)
>                 pci_disable_msi(dev);
>         if (dev->msix_enabled)
>                 pci_disable_msix(dev);
> 
> Activated when used with pcim_enable_device().
> 
> This series is not required.

The problem this series solves is an imbalanced API.
Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
caller must know what's going on. Hiding this behind the scenes is not good.
And this series unhides that.

Also, you may go and clean up all pci_free_irq_vectors() when
pcim_enable_device() is called, but I guess you will get painful process and
rejection in a pile of cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

[Robert Richter]

On 18.02.21 16:01:56, Andy Shevchenko wrote:
> The problem this series solves is an imbalanced API.

This (added) API is bloated and incomplete. It adds functions without
benefit, the only is to have a single pcim alloc function in addition
to the pairing of alloc/free functions. I agree, it is hard to detect
which parts are released if pcim_enable_device() is used.

Additional, you need to go through pcim_release() to add other
pcim_*() functions for everything else that is released there.
Otherwise that new API is still incomplete. But this adds another
bunch of useless functions.

> Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
> caller must know what's going on. Hiding this behind the scenes is not good.
> And this series unhides that.

IMO, this is more a documentation issue. pcim_enable_device() must be
better documented and list all enable/alloc functions that are going
to be released out of the box later.

Even better, make sure everything is managed and thus all of a pci_dev
is released, no matter how it was setup (this could even already be
the case).

In addition you could implement a static code checker.

> Also, you may go and clean up all pci_free_irq_vectors() when
> pcim_enable_device() is called, but I guess you will get painful process and
> rejection in a pile of cases.

Why should something be rejected if it is not correctly freed?

Even if pci_free_irq_vectors() is called, pcim_release() will not
complain if it was already freed before. So using
pci_free_irq_vectors() is ok even in conjunction with
pcim_enable_device().

In the end, let's make sure everything is released in pci_dev if it is
managed and document this.

Thanks,

-Robert

Re: [PATCH v3 0/4] Introduce pcim_alloc_irq_vectors()

[Andy Shevchenko]

On Fri, Feb 19, 2021 at 12:19:20PM +0100, Robert Richter wrote:
> On 18.02.21 16:01:56, Andy Shevchenko wrote:
> > The problem this series solves is an imbalanced API.
> 
> This (added) API is bloated and incomplete. It adds functions without
> benefit, the only is to have a single pcim alloc function in addition
> to the pairing of alloc/free functions. I agree, it is hard to detect
> which parts are released if pcim_enable_device() is used.

No, this API solves the above mentioned problem (what makes so special about
pci_free_irq_vectors() that it should be present?) Why do we have pcim_iomap*()
variations and not the rest?

The PCIm API is horrible in the sense of being used properly. Yes, I know how
it works and I was trying to help with that, the problem is that people didn't
and don't get how it works and stream of patches like the ones that add
pci_free_irq_vectors() are coming.

> Additional, you need to go through pcim_release() to add other
> pcim_*() functions for everything else that is released there.
> Otherwise that new API is still incomplete.

True. And here is the part that most annoying right now.
Btw, I never saw you fought against these small clean ups here and there, that
*add* explicit calls to freeing resources. Also I haven't noticed anybody
trying to correct documentation.

This series is a step to a right direction.

> But this adds another
> bunch of useless functions.

Wrong. This is quite useful to have balanced APIs. How many patches you have
seen related to the PCIm imbalanced APIs? I could tell from my experience, I
saw plenty and each time I'm trying to explain how it works people don't easily
get.

> > Christoph IIRC was clear that if we want to use PCI IRQ allocation API the
> > caller must know what's going on. Hiding this behind the scenes is not good.
> > And this series unhides that.
> 
> IMO, this is more a documentation issue. pcim_enable_device() must be
> better documented and list all enable/alloc functions that are going
> to be released out of the box later.
> 
> Even better, make sure everything is managed and thus all of a pci_dev
> is released, no matter how it was setup (this could even already be
> the case).
> 
> In addition you could implement a static code checker.

It's open source, why should we do that and not what we are proposing here?
Propose your ideas and we will discuss the patches, right?

> > Also, you may go and clean up all pci_free_irq_vectors() when
> > pcim_enable_device() is called, but I guess you will get painful process and
> > rejection in a pile of cases.
> 
> Why should something be rejected if it is not correctly freed?

Why it's not correctly freed? The function is idempotent.

> Even if pci_free_irq_vectors() is called, pcim_release() will not
> complain if it was already freed before. So using
> pci_free_irq_vectors() is ok even in conjunction with
> pcim_enable_device().

No, it's not okay from API namespace / semantics perspective.

> In the end, let's make sure everything is released in pci_dev if it is
> managed and document this.

Feel free to submit a patch!

-- 
With Best Regards,
Andy Shevchenko