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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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