linux-kernel - Re: [PATCH 1/2] misc/pvpanic: Fix error handling in 'pvpanic_pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YKJGrGTHz+DzfiHN@smile.fi.intel.com>
Date:   Mon, 17 May 2021 13:34:20 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     arnd@...db.de, gregkh@...uxfoundation.org,
        mihai.carabas@...cle.com, pizhenwei@...edance.com,
        pbonzini@...hat.com, linqiheng@...wei.com,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] misc/pvpanic: Fix error handling in
 'pvpanic_pci_probe()'

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@...ux.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