All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
@ 2021-05-16 14:36 Christophe JAILLET
  2021-05-16 14:37 ` [PATCH 2/2] misc/pvpanic: Use GFP_KERNEL instead of GFP_ATOMIC Christophe JAILLET
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Christophe JAILLET @ 2021-05-16 14:36 UTC (permalink / raw)
  To: arnd, gregkh, mihai.carabas, andriy.shevchenko, pizhenwei,
	pbonzini, linqiheng
  Cc: linux-kernel, kernel-janitors, Christophe JAILLET

There is no error handling path in the probe function.
Switch to managed resource so that errors in the probe are handled easily
and simplify the remove function accordingly.

Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
index 9ecc4e8559d5..046ce4ecc195 100644
--- a/drivers/misc/pvpanic/pvpanic-pci.c
+++ b/drivers/misc/pvpanic/pvpanic-pci.c
@@ -78,15 +78,15 @@ static int pvpanic_pci_probe(struct pci_dev *pdev,
 	void __iomem *base;
 	int ret;
 
-	ret = pci_enable_device(pdev);
+	ret = pcim_enable_device(pdev);
 	if (ret < 0)
 		return ret;
 
-	base = pci_iomap(pdev, 0, 0);
+	base = pcim_iomap(pdev, 0, 0);
 	if (!base)
 		return -ENOMEM;
 
-	pi = kmalloc(sizeof(*pi), GFP_ATOMIC);
+	pi = devm_kmalloc(&pdev->dev, sizeof(*pi), GFP_ATOMIC);
 	if (!pi)
 		return -ENOMEM;
 
@@ -107,9 +107,6 @@ static void pvpanic_pci_remove(struct pci_dev *pdev)
 	struct pvpanic_instance *pi = dev_get_drvdata(&pdev->dev);
 
 	pvpanic_remove(pi);
-	iounmap(pi->base);
-	kfree(pi);
-	pci_disable_device(pdev);
 }
 
 static struct pci_driver pvpanic_pci_driver = {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] misc/pvpanic: Use GFP_KERNEL instead of GFP_ATOMIC
  2021-05-16 14:36 [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Christophe JAILLET
@ 2021-05-16 14:37 ` Christophe JAILLET
  2021-05-17  8:01 ` [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Andy Shevchenko
  2021-05-21 12:36 ` Greg KH
  2 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2021-05-16 14:37 UTC (permalink / raw)
  To: arnd, gregkh, mihai.carabas, andriy.shevchenko, pizhenwei,
	pbonzini, linqiheng
  Cc: linux-kernel, kernel-janitors, Christophe JAILLET

There is no need to use GFP_ATOMIC in a probe function. Use GFP_KERNEL
instead.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/misc/pvpanic/pvpanic-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
index 046ce4ecc195..3d7f9efb3dd4 100644
--- a/drivers/misc/pvpanic/pvpanic-pci.c
+++ b/drivers/misc/pvpanic/pvpanic-pci.c
@@ -86,7 +86,7 @@ static int pvpanic_pci_probe(struct pci_dev *pdev,
 	if (!base)
 		return -ENOMEM;
 
-	pi = devm_kmalloc(&pdev->dev, sizeof(*pi), GFP_ATOMIC);
+	pi = devm_kmalloc(&pdev->dev, sizeof(*pi), GFP_KERNEL);
 	if (!pi)
 		return -ENOMEM;
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-16 14:36 [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Christophe JAILLET
  2021-05-16 14:37 ` [PATCH 2/2] misc/pvpanic: Use GFP_KERNEL instead of GFP_ATOMIC Christophe JAILLET
@ 2021-05-17  8:01 ` Andy Shevchenko
  2021-05-17 10:02   ` Christophe JAILLET
  2021-05-21 12:36 ` Greg KH
  2 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17  8:01 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: arnd, gregkh, mihai.carabas, pizhenwei, pbonzini, linqiheng,
	linux-kernel, kernel-janitors

On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
> There is no error handling path in the probe function.
> Switch to managed resource so that errors in the probe are handled easily
> and simplify the remove function accordingly.

Yes, that's what I suggested earlier to another contributor.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

P.S. You may consider the following things as well:
 1) converting to use pci_set_drvdata() / pci_get_drvdata()
 2) providing devm_pvpanic_probe() [via devm_add_action() /
    devm_add_action_or_reset()]

> Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
> index 9ecc4e8559d5..046ce4ecc195 100644
> --- a/drivers/misc/pvpanic/pvpanic-pci.c
> +++ b/drivers/misc/pvpanic/pvpanic-pci.c
> @@ -78,15 +78,15 @@ static int pvpanic_pci_probe(struct pci_dev *pdev,
>  	void __iomem *base;
>  	int ret;
>  
> -	ret = pci_enable_device(pdev);
> +	ret = pcim_enable_device(pdev);
>  	if (ret < 0)
>  		return ret;
>  
> -	base = pci_iomap(pdev, 0, 0);
> +	base = pcim_iomap(pdev, 0, 0);
>  	if (!base)
>  		return -ENOMEM;
>  
> -	pi = kmalloc(sizeof(*pi), GFP_ATOMIC);
> +	pi = devm_kmalloc(&pdev->dev, sizeof(*pi), GFP_ATOMIC);
>  	if (!pi)
>  		return -ENOMEM;
>  
> @@ -107,9 +107,6 @@ static void pvpanic_pci_remove(struct pci_dev *pdev)
>  	struct pvpanic_instance *pi = dev_get_drvdata(&pdev->dev);
>  
>  	pvpanic_remove(pi);
> -	iounmap(pi->base);
> -	kfree(pi);
> -	pci_disable_device(pdev);
>  }
>  
>  static struct pci_driver pvpanic_pci_driver = {
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-17  8:01 ` [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Andy Shevchenko
@ 2021-05-17 10:02   ` Christophe JAILLET
  2021-05-17 10:34     ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe JAILLET @ 2021-05-17 10:02 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: arnd, gregkh, mihai.carabas, pizhenwei, pbonzini, linqiheng,
	linux-kernel, kernel-janitors

Le 17/05/2021 à 10:01, Andy Shevchenko a écrit :
> On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
>> There is no error handling path in the probe function.
>> Switch to managed resource so that errors in the probe are handled easily
>> and simplify the remove function accordingly.
> 
> Yes, that's what I suggested earlier to another contributor.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Thanks!
> 
> P.S. You may consider the following things as well:
>   1) converting to use pci_set_drvdata() / pci_get_drvdata()

I can send a patch for that if you want.
But it looks really low value for a driver that is already very short 
and clean.

>   2) providing devm_pvpanic_probe() [via devm_add_action() /
>      devm_add_action_or_reset()]

I don't follow you here.
The goal would be to avoid the remove function and "record" the needed 
action directly in the probe?

If this is it, I would only see an unusual pattern and a harder to 
follow logic.

Did I miss something?
What would be the benefit?

CJ

> 
>> Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
>> index 9ecc4e8559d5..046ce4ecc195 100644
>> --- a/drivers/misc/pvpanic/pvpanic-pci.c
>> +++ b/drivers/misc/pvpanic/pvpanic-pci.c
>> @@ -78,15 +78,15 @@ static int pvpanic_pci_probe(struct pci_dev *pdev,
>>   	void __iomem *base;
>>   	int ret;
>>   
>> -	ret = pci_enable_device(pdev);
>> +	ret = pcim_enable_device(pdev);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	base = pci_iomap(pdev, 0, 0);
>> +	base = pcim_iomap(pdev, 0, 0);
>>   	if (!base)
>>   		return -ENOMEM;
>>   
>> -	pi = kmalloc(sizeof(*pi), GFP_ATOMIC);
>> +	pi = devm_kmalloc(&pdev->dev, sizeof(*pi), GFP_ATOMIC);
>>   	if (!pi)
>>   		return -ENOMEM;
>>   
>> @@ -107,9 +107,6 @@ static void pvpanic_pci_remove(struct pci_dev *pdev)
>>   	struct pvpanic_instance *pi = dev_get_drvdata(&pdev->dev);
>>   
>>   	pvpanic_remove(pi);
>> -	iounmap(pi->base);
>> -	kfree(pi);
>> -	pci_disable_device(pdev);
>>   }
>>   
>>   static struct pci_driver pvpanic_pci_driver = {
>> -- 
>> 2.30.2
>>
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-17 10:02   ` Christophe JAILLET
@ 2021-05-17 10:34     ` Andy Shevchenko
  2021-05-17 11:24       ` Christophe JAILLET
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17 10:34 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: arnd, gregkh, mihai.carabas, pizhenwei, pbonzini, linqiheng,
	linux-kernel, kernel-janitors

On Mon, May 17, 2021 at 12:02:24PM +0200, Christophe JAILLET wrote:
> Le 17/05/2021 à 10:01, Andy Shevchenko a écrit :
> > On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
> > > There is no error handling path in the probe function.
> > > Switch to managed resource so that errors in the probe are handled easily
> > > and simplify the remove function accordingly.
> > 
> > Yes, that's what I suggested earlier to another contributor.
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Thanks!
> > 
> > P.S. You may consider the following things as well:
> >   1) converting to use pci_set_drvdata() / pci_get_drvdata()
> 
> I can send a patch for that if you want.
> But it looks really low value for a driver that is already very short and
> clean.

Yep, that's why 2) below came to my mind (then you will remove drvdata call).

> >   2) providing devm_pvpanic_probe() [via devm_add_action() /
> >      devm_add_action_or_reset()]
> 
> I don't follow you here.
> The goal would be to avoid the remove function and "record" the needed
> action directly in the probe?
> 
> If this is it, I would only see an unusual pattern and a harder to follow
> logic.

> Did I miss something?
> What would be the benefit?

First of all it's a usual pattern when one, often used in ->probe(), function
gains its devm variant. See, for example, `return devm_gpiochip_add_data(...);`
used in the code.

Benefit is to have everything under managed resources and yes, no ->remove()
will be needed in the individual drivers.

But it's up to you. It was just a proposal that you may simply refuse to follow,
it's fine.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-17 10:34     ` Andy Shevchenko
@ 2021-05-17 11:24       ` Christophe JAILLET
  0 siblings, 0 replies; 9+ messages in thread
From: Christophe JAILLET @ 2021-05-17 11:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: arnd, gregkh, mihai.carabas, pizhenwei, pbonzini, linqiheng,
	linux-kernel, kernel-janitors

Le 17/05/2021 à 12:34, Andy Shevchenko a écrit :
> On Mon, May 17, 2021 at 12:02:24PM +0200, Christophe JAILLET wrote:
>> Le 17/05/2021 à 10:01, Andy Shevchenko a écrit :
>>> On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
>>>> There is no error handling path in the probe function.
>>>> Switch to managed resource so that errors in the probe are handled easily
>>>> and simplify the remove function accordingly.
>>>
>>> Yes, that's what I suggested earlier to another contributor.
>>>
>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>>
>>> Thanks!
>>>
>>> P.S. You may consider the following things as well:
>>>    1) converting to use pci_set_drvdata() / pci_get_drvdata()
>>
>> I can send a patch for that if you want.
>> But it looks really low value for a driver that is already very short and
>> clean.
> 
> Yep, that's why 2) below came to my mind (then you will remove drvdata call).
> 
>>>    2) providing devm_pvpanic_probe() [via devm_add_action() /
>>>       devm_add_action_or_reset()]
>>
>> I don't follow you here.
>> The goal would be to avoid the remove function and "record" the needed
>> action directly in the probe?
>>
>> If this is it, I would only see an unusual pattern and a harder to follow
>> logic.
> 
>> Did I miss something?
>> What would be the benefit?
> 
> First of all it's a usual pattern when one, often used in ->probe(), function
> gains its devm variant. See, for example, `return devm_gpiochip_add_data(...);`
> used in the code.
> 
> Benefit is to have everything under managed resources and yes, no ->remove()
> will be needed in the individual drivers.
> 
> But it's up to you. It was just a proposal that you may simply refuse to follow,
> it's fine.
> 

Ok, I'll propose something when/if my first patches reach -next.

I now better see your point. I first read devm_pvpanic_pci_probe (i.e. 
with pci inside), instead of devm_pvpanic_probe.

CJ

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-16 14:36 [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Christophe JAILLET
  2021-05-16 14:37 ` [PATCH 2/2] misc/pvpanic: Use GFP_KERNEL instead of GFP_ATOMIC Christophe JAILLET
  2021-05-17  8:01 ` [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Andy Shevchenko
@ 2021-05-21 12:36 ` Greg KH
  2021-05-21 12:41   ` Christophe JAILLET
  2 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2021-05-21 12:36 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: arnd, mihai.carabas, andriy.shevchenko, pizhenwei, pbonzini,
	linqiheng, linux-kernel, kernel-janitors

On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
> There is no error handling path in the probe function.
> Switch to managed resource so that errors in the probe are handled easily
> and simplify the remove function accordingly.
> 
> Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

I see two different series for these patches, so I don't know which to
take :(

Please fix up and send a v2 series so that I have a clue...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-21 12:36 ` Greg KH
@ 2021-05-21 12:41   ` Christophe JAILLET
  2021-05-21 13:15     ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe JAILLET @ 2021-05-21 12:41 UTC (permalink / raw)
  To: Greg KH
  Cc: arnd, mihai.carabas, andriy.shevchenko, pizhenwei, pbonzini,
	linqiheng, linux-kernel, kernel-janitors

Le 21/05/2021 à 14:36, Greg KH a écrit :
> On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
>> There is no error handling path in the probe function.
>> Switch to managed resource so that errors in the probe are handled easily
>> and simplify the remove function accordingly.
>>
>> Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> I see two different series for these patches, so I don't know which to
> take :(
> 
> Please fix up and send a v2 series so that I have a clue...
> 
> thanks,
> 
> greg k-h
> 

Both have to be taken. One is for -pci.c and one is for -mmio.c.

I'll resend both with a more complete subject and will include Andy 
Shevchenko's comments.

CJ

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
  2021-05-21 12:41   ` Christophe JAILLET
@ 2021-05-21 13:15     ` Greg KH
  0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2021-05-21 13:15 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: arnd, mihai.carabas, andriy.shevchenko, pizhenwei, pbonzini,
	linqiheng, linux-kernel, kernel-janitors

On Fri, May 21, 2021 at 02:41:16PM +0200, Christophe JAILLET wrote:
> Le 21/05/2021 à 14:36, Greg KH a écrit :
> > On Sun, May 16, 2021 at 04:36:55PM +0200, Christophe JAILLET wrote:
> > > There is no error handling path in the probe function.
> > > Switch to managed resource so that errors in the probe are handled easily
> > > and simplify the remove function accordingly.
> > > 
> > > Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   drivers/misc/pvpanic/pvpanic-pci.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > I see two different series for these patches, so I don't know which to
> > take :(
> > 
> > Please fix up and send a v2 series so that I have a clue...
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> Both have to be taken. One is for -pci.c and one is for -mmio.c.

Ah, I totally missed that :(

> I'll resend both with a more complete subject and will include Andy
> Shevchenko's comments.

Wonderful, thanks for doing that.

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-21 13:15 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-16 14:36 [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Christophe JAILLET
2021-05-16 14:37 ` [PATCH 2/2] misc/pvpanic: Use GFP_KERNEL instead of GFP_ATOMIC Christophe JAILLET
2021-05-17  8:01 ` [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()' Andy Shevchenko
2021-05-17 10:02   ` Christophe JAILLET
2021-05-17 10:34     ` Andy Shevchenko
2021-05-17 11:24       ` Christophe JAILLET
2021-05-21 12:36 ` Greg KH
2021-05-21 12:41   ` Christophe JAILLET
2021-05-21 13:15     ` Greg KH

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.