| Message ID | 20210216160249.749799-4-zhengdejin5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Introduce pcim_alloc_irq_vectors() | expand |
Hi Dejin, Thank you for all the changes, looks good! You could improve the subject line, as it is very vague - what is the new function name more correct? Was the other and/or the previous one not correct? Seems like you are correcting a typo of sorts, rather than introducing a new function in this file. > Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors, > the pcim_alloc_irq_vectors() function, an explicit device-managed > version of pci_alloc_irq_vectors(). If pcim_enable_device() has been > called before, then pci_alloc_irq_vectors() is actually > a device-managed function. It is used here as a device-managed > function, So replace it with pcim_alloc_irq_vectors(). The commit is good, but it could use some polish, so to speak. A few suggestions to think about: - What are we adding and/or changing, and why - Why is using pcim_alloc_irq_vectors(), which is part of the managed devices framework, a better alternative to the pci_alloc_irq_vectors() - And finally why this change allowed us to remove the pci_free_irq_vectors() > At the same time, simplify the error handling path. The change simplifies the error handling path, how? A line of two which explains how it has been achieved might help should someone reads the commit message in the future. [...] > if (controller->setup) { > r = controller->setup(pdev, controller); > - if (r) { > - pci_free_irq_vectors(pdev); > + if (r) > return r; > - } > } > > i2c_dw_adjust_bus_speed(dev); > @@ -246,10 +244,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > i2c_dw_acpi_configure(&pdev->dev); > > r = i2c_dw_validate_speed(dev); > - if (r) { > - pci_free_irq_vectors(pdev); > + if (r) > return r; > - } > > i2c_dw_configure(dev); > > @@ -269,10 +265,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > adap->nr = controller->bus_num; > > r = i2c_dw_probe(dev); > - if (r) { > - pci_free_irq_vectors(pdev); > + if (r) > return r; > - } > > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > pm_runtime_use_autosuspend(&pdev->dev); > @@ -292,7 +286,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev) > > i2c_del_adapter(&dev->adapter); > devm_free_irq(&pdev->dev, dev->irq, dev); > - pci_free_irq_vectors(pdev); If pcim_release() is called should the pci_driver's probe callback fail, and I assume that this is precisely the case, then all of the above make sense in the view of using pcim_alloc_irq_vectors(). Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
On Tue, Feb 16, 2021 at 06:46:01PM +0100, Krzysztof Wilczyński wrote: Hi Krzysztof, > Hi Dejin, > > Thank you for all the changes, looks good! > > You could improve the subject line, as it is very vague - what is the > new function name more correct? Was the other and/or the previous one > not correct? Seems like you are correcting a typo of sorts, rather than > introducing a new function in this file. > If you have read the following commit comments, As you know, the pci_alloc_irq_vectors() is not a real device-managed function. But in some specific cases, it will act as an device-managed function. Such naming will cause controversy, So In the case of need device-managed, should be used pcim_alloc_irq_vectors(), an explicit device-managed function. So the subject name is "Use the correct name of device-managed function". > > Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors, > > the pcim_alloc_irq_vectors() function, an explicit device-managed > > version of pci_alloc_irq_vectors(). If pcim_enable_device() has been > > called before, then pci_alloc_irq_vectors() is actually > > a device-managed function. It is used here as a device-managed > > function, So replace it with pcim_alloc_irq_vectors(). > > The commit is good, but it could use some polish, so to speak. > > A few suggestions to think about: > > - What are we adding and/or changing, and why > - Why is using pcim_alloc_irq_vectors(), which is part > of the managed devices framework, a better alternative > to the pci_alloc_irq_vectors() > - And finally why this change allowed us to remove the > pci_free_irq_vectors() > These are all explained by the device-managed function mechanism. > > At the same time, simplify the error handling path. > > The change simplifies the error handling path, how? A line of two which > explains how it has been achieved might help should someone reads the > commit message in the future. > To put it simply, if the driver probe fail, the device-managed function mechanism will automatically call pcim_release(), then the pci_free_irq_vectors() will be executed. For details, please see the relevant code. > [...] > > if (controller->setup) { > > r = controller->setup(pdev, controller); > > - if (r) { > > - pci_free_irq_vectors(pdev); > > + if (r) > > return r; > > - } > > } > > > > i2c_dw_adjust_bus_speed(dev); > > @@ -246,10 +244,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > > i2c_dw_acpi_configure(&pdev->dev); > > > > r = i2c_dw_validate_speed(dev); > > - if (r) { > > - pci_free_irq_vectors(pdev); > > + if (r) > > return r; > > - } > > > > i2c_dw_configure(dev); > > > > @@ -269,10 +265,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > > adap->nr = controller->bus_num; > > > > r = i2c_dw_probe(dev); > > - if (r) { > > - pci_free_irq_vectors(pdev); > > + if (r) > > return r; > > - } > > > > pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > > pm_runtime_use_autosuspend(&pdev->dev); > > @@ -292,7 +286,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev) > > > > i2c_del_adapter(&dev->adapter); > > devm_free_irq(&pdev->dev, dev->irq, dev); > > - pci_free_irq_vectors(pdev); > > If pcim_release() is called should the pci_driver's probe callback fail, Yes, you guessed right. > and I assume that this is precisely the case, then all of the above make > sense in the view of using pcim_alloc_irq_vectors(). > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Krzysztof BR, Dejin
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".
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 > >
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 */
Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors, the pcim_alloc_irq_vectors() function, an explicit device-managed version of pci_alloc_irq_vectors(). If pcim_enable_device() has been called before, then pci_alloc_irq_vectors() is actually a device-managed function. It is used here as a device-managed function, So replace it with pcim_alloc_irq_vectors(). At the same time, simplify the error handling path. Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- v2 -> v3: - simplify the error handling path. v1 -> v2: - Modify some commit messages. drivers/i2c/busses/i2c-designware-pcidrv.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)