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

[permalink] [raw]

Subject: [PATCH v3 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.

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

2021-02-16 16:05:42

[permalink] [raw]

Subject: [PATCH v3 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.

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

2021-02-16 16:06:01

[permalink] [raw]

Subject: [PATCH v3 3/4] i2c: designware: 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(). At the same time, simplify the error
handling path.

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

2021-02-16 16:06:16

[permalink] [raw]

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

2021-02-16 17:13:32

[permalink] [raw]

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

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

Krzysztof

2021-02-16 17:50:23

[permalink] [raw]

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

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

Krzysztof

2021-02-16 17:53:38

[permalink] [raw]

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

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

2021-02-16 17:57:00

[permalink] [raw]

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

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

Krzysztof

2021-02-17 10:53:10

[permalink] [raw]

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

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

2021-02-17 11:47:47

[permalink] [raw]

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

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

BR,
Dejin

2021-02-17 13:51:41

[permalink] [raw]

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

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

2021-02-17 18:08:33

[permalink] [raw]

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

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

2021-02-18 10:56:39

by Robert Richter

[permalink] [raw]

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

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
>

2021-02-18 16:57:07

[permalink] [raw]

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

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

2021-02-18 17:08:15

[permalink] [raw]

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

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

2021-02-19 11:26:49

by Robert Richter

[permalink] [raw]

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

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

2021-02-19 13:57:45