Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

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