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

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

[Dejin Zheng]

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

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

[Jan Kiszka]

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

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

[Jarkko Nikula]

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>

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

[Dejin Zheng]

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>

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

[Andy Shevchenko]

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

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

[Jan Kiszka]

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

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

[Dejin Zheng]

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

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

[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