diff mbox series

spi: pca2xx-pci: Fix an issue about missing call to 'pci_free_irq_vectors()'

Message ID 20210214145746.602770-1-zhengdejin5@gmail.com (mailing list archive)
State New
Headers show
Series spi: pca2xx-pci: Fix an issue about missing call to 'pci_free_irq_vectors()' | expand

Commit Message

Dejin Zheng Feb. 14, 2021, 2:57 p.m. UTC
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(-)

Comments

Jan Kiszka Feb. 15, 2021, 9:23 a.m. UTC | #1
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
Jarkko Nikula Feb. 15, 2021, 9:41 a.m. UTC | #2
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>
Dejin Zheng Feb. 15, 2021, 12:38 p.m. UTC | #3
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>
Andy Shevchenko Feb. 15, 2021, 1:22 p.m. UTC | #4
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.
Jan Kiszka Feb. 15, 2021, 1:42 p.m. UTC | #5
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
Dejin Zheng Feb. 15, 2021, 2:55 p.m. UTC | #6
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
> 
>
Andy Shevchenko Feb. 15, 2021, 2:55 p.m. UTC | #7
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.
diff mbox series

Patch

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);
 }