linux-kernel - Re: [PATCH] staging: iio: ad5933: rework probe to use devm

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: <20200508134307.0000233a@Huawei.com>
Date:   Fri, 8 May 2020 13:43:07 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        <linux-iio@...r.kernel.org>, <devel@...verdev.osuosl.org>,
        <linux-kernel@...r.kernel.org>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] staging: iio: ad5933: rework probe to use devm_
 function variants

On Thu, 7 May 2020 12:50:16 +0300
Dan Carpenter <dan.carpenter@...cle.com> wrote:

> On Sat, May 02, 2020 at 07:25:42PM +0100, Jonathan Cameron wrote:
> > On Tue, 28 Apr 2020 12:31:28 +0300
> > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:  
> > > +static void ad5933_cleanup(void *data)
> > > +{
> > > +	struct ad5933_state *st = data;
> > > +
> > > +	clk_disable_unprepare(st->mclk);
> > > +	regulator_disable(st->reg);  
> > 
> > Please do two separate callbacks so that these can be handled
> > in the correct places.  I.e. you do something then immediately
> > register the handler to undo it.
> > 
> > Currently you can end up disabling a clock you haven't enabled
> > (which I am fairly sure will give you an error message).  
> 
> Yeah.  It does.
> 
> It feels like we should just make a devm_ version of regulator_enable().
> Or potentially this is more complicated than it seems, but in that case
> probably adding devm_add_action_or_reset() is more complicated than it
> seems as well.
> 
> regards,
> dan carpenter

It has been a while since that was last proposed.   At the time the
counter argument was that you should almost always be doing some form
of PM and hence the regulator shouldn't have the same lifetime as the
driver.   Reality is that a lot of simple drivers either don't do
PM or have elected to not turn the regulator off so as to retain state
etc.

Mark what do you think?

Jonathan