[PATCH v1 0/4] Introduce pcim_alloc_irq_vectors()

[PATCH v1 0/4] Introduce pcim_alloc_irq_vectors()

[Dejin Zheng]

Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
pci_alloc_irq_vectors(). and use the correct name of device-managed
function to alloc irq vectors in i2c drivers.

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    |  2 +-
 drivers/i2c/busses/i2c-thunderx-pcidrv.c      |  2 +-
 drivers/pci/pci.c                             | 19 +++++++++++++++++++
 include/linux/pci.h                           |  3 +++
 5 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.25.0

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

[Dejin Zheng]

Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
pci_alloc_irq_vectors().

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/pci/pci.c   | 19 +++++++++++++++++++
 include/linux/pci.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b67c4327d307..33244b512057 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2054,6 +2054,25 @@ void pcim_pin_device(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL(pcim_pin_device);
 
+/**
+ * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors()
+ *
+ * It depends on calling pcim_enable_device() to make irq resources manageable.
+ */
+int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
+				unsigned int max_vecs, unsigned int flags)
+{
+	struct pci_devres *dr;
+
+       /*Ensure that the pcim_enable_device() function has been called*/
+	dr = find_pci_dr(dev);
+	if (!dr || !dr->enabled)
+		return -EINVAL;
+
+	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
+}
+EXPORT_SYMBOL(pcim_alloc_irq_vectors);
+
 /*
  * pcibios_add_device - provide arch specific hooks when adding device dev
  * @dev: the PCI device being added
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..d75ba85ddfc5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1818,6 +1818,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 					      NULL);
 }
 
+int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
+				unsigned int max_vecs, unsigned int flags);
+
 /* Include architecture-dependent settings and functions */
 
 #include <asm/pci.h>
-- 
2.25.0

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

[Krzysztof Wilczyński]

Hi Dejin,

Thank you for all the work here!

The subject and the commit message could be improved to include a little
more details about why do you want to do it, and what problems does it
aims to solve.

> Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
> pci_alloc_irq_vectors().

You can probably drop the "explicit" word from the sentence above.
 
> +/**
> + * pcim_alloc_irq_vectors - a device-managed pci_alloc_irq_vectors()
> + *
> + * It depends on calling pcim_enable_device() to make irq resources manageable.
> + */

It would be "IRQ" in the sentence above.  Also see [1] for more details
about how to make a patch ready to be accepted.

Also, this comment looks like it's intended to be compliant with the
kernel-doc format, and if so, then you should describe each argument as
the bare minimum, so that the entire comment would become this function
documentation making it also a little more useful.  See [2] for an
example of how to use kernel-doc.

> +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
> +				unsigned int max_vecs, unsigned int flags)
> +{
> +	struct pci_devres *dr;
> +
> +       /*Ensure that the pcim_enable_device() function has been called*/

The comment above has to be correctly aligned and it's also missing
spaces around the sentence to be properly formatted, see [3].

> +	dr = find_pci_dr(dev);
> +	if (!dr || !dr->enabled)
> +		return -EINVAL;
> +
> +	return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> +}

Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
possibly to pcim_release() callback?  Although, I am not sure where the
right place would be.

I am asking, as the documentation (see [4]) suggests that one would have
to release allocated IRQ vectors (relevant exceprt):

>> To automatically use MSI or MSI-X interrupt vectors, use the following
>> function:
>>
>>  int pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>		unsigned int max_vecs, unsigned int flags);
>>
>> which allocates up to max_vecs interrupt vectors for a PCI device.
>>
>> (...)
>>
>> Any allocated resources should be freed before removing the device using
>> the following function:
>>
>>  void pci_free_irq_vectors(struct pci_dev *dev);

What do you think?

1. https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/
2. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
3. https://www.kernel.org/doc/html/latest/process/coding-style.html
4. https://www.kernel.org/doc/html/latest/PCI/msi-howto.html

Krzysztof

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

[Andy Shevchenko]

On Mon, Feb 15, 2021 at 09:55:06PM +0100, Krzysztof Wilczyński wrote:

> Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
> possibly to pcim_release() callback?  Although, I am not sure where the
> right place would be.
> 
> I am asking, as the documentation (see [4]) suggests that one would have
> to release allocated IRQ vectors (relevant exceprt):

It's done in pcim_release() but not explicitly.

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

Maybe above can be replaced by pci_free_irq_vectors() to be sure that any
future change to PCI IRQ allocation APIs.

Yes, I have checked and currently the above code is equivalent to
pci_free_irq_vectors().

Dejin, please update your patch accordingly.


-- 
With Best Regards,
Andy Shevchenko

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

[Dejin Zheng]

On Tue, Feb 16, 2021 at 12:12:39PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 09:55:06PM +0100, Krzysztof Wilczyński wrote:
> 
> > Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
> > possibly to pcim_release() callback?  Although, I am not sure where the
> > right place would be.
> > 
> > I am asking, as the documentation (see [4]) suggests that one would have
> > to release allocated IRQ vectors (relevant exceprt):
> 
> It's done in pcim_release() but not explicitly.
> 
>         if (dev->msi_enabled)
>                 pci_disable_msi(dev);
>         if (dev->msix_enabled)
>                 pci_disable_msix(dev);
> 
> Maybe above can be replaced by pci_free_irq_vectors() to be sure that any
> future change to PCI IRQ allocation APIs.
> 
> Yes, I have checked and currently the above code is equivalent to
> pci_free_irq_vectors().
> 
> Dejin, please update your patch accordingly.
>
Hi Andy and Krzysztof,

I have modified it and sent patch v2. thank you very much!

BR,
Dejin

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

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

[Krzysztof Wilczyński]

Hi Dejin and Andy,

[...]
> > > Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
> > > possibly to pcim_release() callback?  Although, I am not sure where the
> > > right place would be.
> > > 
> > > I am asking, as the documentation (see [4]) suggests that one would have
> > > to release allocated IRQ vectors (relevant exceprt):
> > 
> > It's done in pcim_release() but not explicitly.
> > 
> >         if (dev->msi_enabled)
> >                 pci_disable_msi(dev);
> >         if (dev->msix_enabled)
> >                 pci_disable_msix(dev);
> > 
> > Maybe above can be replaced by pci_free_irq_vectors() to be sure that any
> > future change to PCI IRQ allocation APIs.
> > 
> > Yes, I have checked and currently the above code is equivalent to
> > pci_free_irq_vectors().
> > 
> > Dejin, please update your patch accordingly.
> >
> Hi Andy and Krzysztof,
> 
> I have modified it and sent patch v2. thank you very much!

Thank you Andy for double-checking!  Much appreciated.

Moving to pci_free_irq_vectors() directly looks great as we are also
removing the surplus checks that pcim_release() is doing - since both
the pci_disable_msi() and the pci_disable_msix() are doing internal
validation to see whether MSI/MSI-X is currently enabled.

Thank you again, Dejin and Andy. :)

Krzysztof

[PATCH v1 2/4] Documentation: devres: add pcim_alloc_irq_vectors()

[Dejin Zheng]

add pcim_alloc_irq_vectors(), a explicit device-managed version of
pci_alloc_irq_vectors().

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 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..cd53106fbf5e 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 v1 2/4] Documentation: devres: add pcim_alloc_irq_vectors()

[Krzysztof Wilczyński]

Hello Dejin,

[...]
> add pcim_alloc_irq_vectors(), a explicit device-managed version of
> pci_alloc_irq_vectors().

It would be "Add" at the start of the sentence.  You could also drop
the "explicit" word or replace it with "an explicit", if you want to
keep it.

Generally, this commit message could be improved - you could explain
that you are updating the documentation to reflect the addition of this
new function called pcim_alloc_irq_vectors(), and also briefly explain
why it was added, etc.

[...]
>    devm_pci_remap_cfg_resource()	: ioremap PCI configuration space resource
> +  pcim_alloc_irq_vectors()      : managed irq vectors allocation
[...]

It would be "IRQ" here.

Krzysztof

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

[Dejin Zheng]

Use the correct name of device-managed function to alloc irq vectors,
the pcim_alloc_irq_vectors() function, a explicit device-managed version
of pci_alloc_irq_vectors().

Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 55c83a7a24f3..444533be49ee 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;
 
-- 
2.25.0

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

[Krzysztof Wilczyński]

Hi Dejin,

See my comments regarding a very similar patch:

  https://lore.kernel.org/linux-pci/YCrnn+L42SR4N1PA@rocinante/

Krzysztof

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

[Dejin Zheng]

Use the correct name of device-managed function to alloc irq vectors,
the pcim_alloc_irq_vectors() function, a explicit device-managed version
of pci_alloc_irq_vectors().

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

[Krzysztof Wilczyński]

Hi Dejin,

> Use the correct name of device-managed function to alloc irq vectors,
> the pcim_alloc_irq_vectors() function, a explicit device-managed version
> of pci_alloc_irq_vectors().
[...]

It would be "IRQ" in the sentence above.

Perhaps the "Use the new function pcim_alloc_irq_vectors() to allocate
IRQ vectors ..." would read a little better - generally, the above
sentence could be improved.  You might also want to add a more details
about why to use this new function instead of the previous one, etc.

Krzysztof

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

[Krzysztof Wilczyński]

Hi Dejin,

Thank you for working on this series!

Do you have a link to the conversation that prompted addition of this
new function?  If so, then it would be nice to include a reference to it
here (as a link to http://lore.kernel.org/) in the cover letter for
reference, if possible, of course.

Generally, it would also be nice to expand on things a little bit and
explain why do you want to add this new function, and what problems does
it solve.

[...]
> Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
> pci_alloc_irq_vectors(). and use the correct name of device-managed
> function to alloc irq vectors in i2c drivers.

Did you want to use a comma, instead of a period, in the sentence
above?  You could also probably drop the word "explicit".

Krzysztof