LinuxLists.cc - [PATCH v7 0/4] Introduce pcim_alloc_irq

[permalink] [raw]

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

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 <[email protected]>
Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Robert Richter <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
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

2021-06-07 15:42:46

[permalink] [raw]

Subject: [PATCH v7 3/4] i2c: designware: Use 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.

Reviewed-by: Robert Richter <[email protected]>
Acked-by: Andy Shevchenko <[email protected]>
Acked-by: Jarkko Nikula <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
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

2021-06-07 15:43:05

[permalink] [raw]

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

2021-06-07 15:43:28

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
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

2021-06-07 16:18:56

[permalink] [raw]

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

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 <[email protected]>

> Reviewed-by: Robert Richter <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> 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

2021-06-07 16:24:47

[permalink] [raw]

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

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 <[email protected]>

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 <[email protected]>
> Reviewed-by: Robert Richter <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Acked-by: Bjorn Helgaas <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> 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

2021-06-07 17:17:04

[permalink] [raw]

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

2021-06-09 01:46:06

[permalink] [raw]

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

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 <[email protected]>
>
> 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 <[email protected]>
> > Reviewed-by: Robert Richter <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Acked-by: Bjorn Helgaas <[email protected]>
> > Signed-off-by: Dejin Zheng <[email protected]>
> > ---
> > 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
>
>

2021-06-09 01:46:39

[permalink] [raw]

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

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 <[email protected]>
> >
> > 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

2021-06-10 22:43:24

[permalink] [raw]

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

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
>

2021-06-11 09:39:08

[permalink] [raw]

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

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

2021-06-17 01:48:38

[permalink] [raw]

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

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

2021-06-17 17:59:43

[permalink] [raw]

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

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/[email protected]/
[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

2021-06-17 18:31:42

[permalink] [raw]

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

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/[email protected]/
> [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
>
>

2021-06-17 19:29:03

[permalink] [raw]

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

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/[email protected]/
> [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
>
>

2021-11-29 22:33:24

by Wolfram Sang

[permalink] [raw]

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

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 <[email protected]>
> Acked-by: Andy Shevchenko <[email protected]>
> Acked-by: Jarkko Nikula <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>

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

Acked-by: Wolfram Sang <[email protected]> # for I2C

Attachments:

(No filename) (923.00 B)
signature.asc (833.00 B)
Download all attachments

2021-11-29 23:00:17

by Wolfram Sang

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>

Acked-by: Wolfram Sang <[email protected]> # for I2C

Attachments:

(No filename) (533.00 B)
signature.asc (833.00 B)
Download all attachments

2022-02-24 11:18:28

by Christoph Hellwig

[permalink] [raw]

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

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?

2022-12-16 08:41:42