| Message ID | 20210214145746.602770-1-zhengdejin5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: pca2xx-pci: Fix an issue about missing call to 'pci_free_irq_vectors()' | expand |
On 14.02.21 15:57, Dejin Zheng wrote: > Call to 'pci_free_irq_vectors()' are missing both in the error handling > path of the probe function, and in the remove function. So add them. > > Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > drivers/spi/spi-pxa2xx-pci.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c > index 14fc41ed2361..1ec840e78ff4 100644 > --- a/drivers/spi/spi-pxa2xx-pci.c > +++ b/drivers/spi/spi-pxa2xx-pci.c > @@ -254,8 +254,10 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, > snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id); > ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0, > c->max_clk_rate); > - if (IS_ERR(ssp->clk)) > - return PTR_ERR(ssp->clk); > + if (IS_ERR(ssp->clk)) { > + ret = PTR_ERR(ssp->clk); > + goto err_irq; > + } > > memset(&pi, 0, sizeof(pi)); > pi.fwnode = dev->dev.fwnode; > @@ -268,12 +270,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, > pdev = platform_device_register_full(&pi); > if (IS_ERR(pdev)) { > clk_unregister(ssp->clk); > - return PTR_ERR(pdev); > + ret = PTR_ERR(pdev); > + goto err_irq; > } > > pci_set_drvdata(dev, pdev); > > return 0; > +err_irq: > + pci_free_irq_vectors(dev); > + return ret; > } > > static void pxa2xx_spi_pci_remove(struct pci_dev *dev) > @@ -283,6 +289,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev) > > spi_pdata = dev_get_platdata(&pdev->dev); > > + pci_free_irq_vectors(dev); > platform_device_unregister(pdev); > clk_unregister(spi_pdata->ssp.clk); > } > Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com> Thanks! Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On 2/15/21 11:23 AM, Jan Kiszka wrote: > On 14.02.21 15:57, Dejin Zheng wrote: >> Call to 'pci_free_irq_vectors()' are missing both in the error handling >> path of the probe function, and in the remove function. So add them. >> >> Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") >> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> >> --- >> drivers/spi/spi-pxa2xx-pci.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c >> index 14fc41ed2361..1ec840e78ff4 100644 >> --- a/drivers/spi/spi-pxa2xx-pci.c >> +++ b/drivers/spi/spi-pxa2xx-pci.c >> @@ -254,8 +254,10 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, >> snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id); >> ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0, >> c->max_clk_rate); >> - if (IS_ERR(ssp->clk)) >> - return PTR_ERR(ssp->clk); >> + if (IS_ERR(ssp->clk)) { >> + ret = PTR_ERR(ssp->clk); >> + goto err_irq; >> + } >> >> memset(&pi, 0, sizeof(pi)); >> pi.fwnode = dev->dev.fwnode; >> @@ -268,12 +270,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, >> pdev = platform_device_register_full(&pi); >> if (IS_ERR(pdev)) { >> clk_unregister(ssp->clk); >> - return PTR_ERR(pdev); >> + ret = PTR_ERR(pdev); >> + goto err_irq; >> } >> >> pci_set_drvdata(dev, pdev); >> >> return 0; >> +err_irq: >> + pci_free_irq_vectors(dev); >> + return ret; >> } >> >> static void pxa2xx_spi_pci_remove(struct pci_dev *dev) >> @@ -283,6 +289,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev) >> >> spi_pdata = dev_get_platdata(&pdev->dev); >> >> + pci_free_irq_vectors(dev); >> platform_device_unregister(pdev); >> clk_unregister(spi_pdata->ssp.clk); >> } >> > > Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com> > Please fix pca2xx-pci -> pxa2xx-pci in the subject line. With that change you may add: Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Mon, Feb 15, 2021 at 11:41:50AM +0200, Jarkko Nikula wrote: > On 2/15/21 11:23 AM, Jan Kiszka wrote: > > On 14.02.21 15:57, Dejin Zheng wrote: > > > Call to 'pci_free_irq_vectors()' are missing both in the error handling > > > path of the probe function, and in the remove function. So add them. > > > > > > Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") > > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > > > --- > > > drivers/spi/spi-pxa2xx-pci.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c > > > index 14fc41ed2361..1ec840e78ff4 100644 > > > --- a/drivers/spi/spi-pxa2xx-pci.c > > > +++ b/drivers/spi/spi-pxa2xx-pci.c > > > @@ -254,8 +254,10 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, > > > snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id); > > > ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0, > > > c->max_clk_rate); > > > - if (IS_ERR(ssp->clk)) > > > - return PTR_ERR(ssp->clk); > > > + if (IS_ERR(ssp->clk)) { > > > + ret = PTR_ERR(ssp->clk); > > > + goto err_irq; > > > + } > > > memset(&pi, 0, sizeof(pi)); > > > pi.fwnode = dev->dev.fwnode; > > > @@ -268,12 +270,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, > > > pdev = platform_device_register_full(&pi); > > > if (IS_ERR(pdev)) { > > > clk_unregister(ssp->clk); > > > - return PTR_ERR(pdev); > > > + ret = PTR_ERR(pdev); > > > + goto err_irq; > > > } > > > pci_set_drvdata(dev, pdev); > > > return 0; > > > +err_irq: > > > + pci_free_irq_vectors(dev); > > > + return ret; > > > } > > > static void pxa2xx_spi_pci_remove(struct pci_dev *dev) > > > @@ -283,6 +289,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev) > > > spi_pdata = dev_get_platdata(&pdev->dev); > > > + pci_free_irq_vectors(dev); > > > platform_device_unregister(pdev); > > > clk_unregister(spi_pdata->ssp.clk); > > > } > > > > > > > Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com> > > > Please fix pca2xx-pci -> pxa2xx-pci in the subject line. With that change > you may add: Jan and Jarkko, Thanks very much for your review. I will modify the subject name in patch v2, Thanks again! Dejin > > Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Sun, Feb 14, 2021 at 10:57:46PM +0800, Dejin Zheng wrote: > Call to 'pci_free_irq_vectors()' are missing both in the error handling > path of the probe function, and in the remove function. So add them. I'm wondering if you noticed that it's done by pcim_* API. Perhaps you can introduce pcim_alloc_irq_vectors() or so and do not add these calls at all? > Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") No, it doesn't fix anything. -- With Best Regards, Andy Shevchenko
On 15.02.21 14:22, Andy Shevchenko wrote: > On Sun, Feb 14, 2021 at 10:57:46PM +0800, Dejin Zheng wrote: >> Call to 'pci_free_irq_vectors()' are missing both in the error handling >> path of the probe function, and in the remove function. So add them. > > I'm wondering if you noticed that it's done by pcim_* API. > Perhaps you can introduce pcim_alloc_irq_vectors() or so and do not add these > calls at all? You mean as plain wrapper for pci_alloc_irq_vectors, just to document it's managed? > >> Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") > > No, it doesn't fix anything. > Ah, now I recall: imbalanced APIs. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On Mon, Feb 15, 2021 at 03:22:38PM +0200, Andy Shevchenko wrote: > On Sun, Feb 14, 2021 at 10:57:46PM +0800, Dejin Zheng wrote: > > Call to 'pci_free_irq_vectors()' are missing both in the error handling > > path of the probe function, and in the remove function. So add them. > > I'm wondering if you noticed that it's done by pcim_* API. > Perhaps you can introduce pcim_alloc_irq_vectors() or so and do not add these > calls at all? Andy, Thanks for your reminder, You mean introduce pcim_alloc_irq_vectors() as like pcim_set_mwi() and free irq vectors by pcim_release()? > > > Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") > > No, it doesn't fix anything. > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Feb 15, 2021 at 3:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 15.02.21 14:22, Andy Shevchenko wrote: > > On Sun, Feb 14, 2021 at 10:57:46PM +0800, Dejin Zheng wrote: > >> Call to 'pci_free_irq_vectors()' are missing both in the error handling > >> path of the probe function, and in the remove function. So add them. > > > > I'm wondering if you noticed that it's done by pcim_* API. > > Perhaps you can introduce pcim_alloc_irq_vectors() or so and do not add these > > calls at all? > > You mean as plain wrapper for pci_alloc_irq_vectors, just to document > it's managed? Last time we discussed that with Christoph Hellwig he was on the side that naming is problematic. So he insisted that it's pure luck that it works like this. And IIUC his point, we need to create an explicit managed version of pci_alloc_irq_vectorrs() that the caller will have clear understanding what it does. > >> Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") > > > > No, it doesn't fix anything. > > Ah, now I recall: imbalanced APIs. -- With Best Regards, Andy Shevchenko
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c index 14fc41ed2361..1ec840e78ff4 100644 --- a/drivers/spi/spi-pxa2xx-pci.c +++ b/drivers/spi/spi-pxa2xx-pci.c @@ -254,8 +254,10 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id); ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0, c->max_clk_rate); - if (IS_ERR(ssp->clk)) - return PTR_ERR(ssp->clk); + if (IS_ERR(ssp->clk)) { + ret = PTR_ERR(ssp->clk); + goto err_irq; + } memset(&pi, 0, sizeof(pi)); pi.fwnode = dev->dev.fwnode; @@ -268,12 +270,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, pdev = platform_device_register_full(&pi); if (IS_ERR(pdev)) { clk_unregister(ssp->clk); - return PTR_ERR(pdev); + ret = PTR_ERR(pdev); + goto err_irq; } pci_set_drvdata(dev, pdev); return 0; +err_irq: + pci_free_irq_vectors(dev); + return ret; } static void pxa2xx_spi_pci_remove(struct pci_dev *dev) @@ -283,6 +289,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev) spi_pdata = dev_get_platdata(&pdev->dev); + pci_free_irq_vectors(dev); platform_device_unregister(pdev); clk_unregister(spi_pdata->ssp.clk); }
Call to 'pci_free_irq_vectors()' are missing both in the error handling path of the probe function, and in the remove function. So add them. Fixes: 64e02cb0bdfc7c ("spi: pca2xx-pci: Allow MSI") Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- drivers/spi/spi-pxa2xx-pci.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)