Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci_probe()'
From: Christophe JAILLET
Date: Mon May 17 2021 - 07:24:52 EST
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@xxxxxxxxxxxxxxx>
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