linux-kernel - Re: [PATCH v9 2/2] iio: light: Add support for ltrf216a sensor

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: <20220719131808.7899acd4@jic23-huawei>
Date:   Tue, 19 Jul 2022 13:19:28 +0100
From:   Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     Shreeya Patel <shreeya.patel@...labora.com>, lars@...afoo.de,
        robh+dt@...nel.org, Zhigang.Shi@...eon.com, krisman@...labora.com,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com,
        alvaro.soliverez@...labora.com, andy.shevchenko@...il.com
Subject: Re: [PATCH v9 2/2] iio: light: Add support for ltrf216a sensor

On Tue, 19 Jul 2022 14:56:51 +0300
Dmitry Osipenko <dmitry.osipenko@...labora.com> wrote:

> On 7/18/22 20:25, Jonathan Cameron wrote:
> > What turns this off again?  I'd expect to see a devm_add_action_or_reset()
> > to do that in the !CONFIG_PM case.
> > 
> > This is also an unusual pattern. As far as I can tell it works.
> > Normal trick for ensuring !CONFIG_PM works is to:
> > 
> > 1) Unconditionally turn device on.
> > 2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
> >    due to runtime pm.  
> 
> If CONFIG_PM is disabled, do we really need to care about the power
> management on removal?
> 

Best effort + in general if we do something probe(), we want to do the
reverse in remove().  Sure it's not super important, but it's a nice
to have.  This tends to get 'fixed' by people revisiting the driver
after it has merged.

> > 3) Then call pm_runtime_set_active() so the state tracking matches.  
> 
> We can add pm_runtime_set_active() before h/w is touched for more
> consistency. On Steam Deck supplies are always enabled, but this may be
> not true for other devices.

Generally set it wherever you 'enable' the device as you are indicating
the state after that has happened. That might be really early though.

> 
> > 4) Call 	
> >   pm_runtime_mark_last_busy(dev);
> >   pm_runtime_put_autosuspend(dev);
> >   (here you have a function to do this anyway)
> >   to let runtime_pm use same path as normal to autosuspend
> > 
> > the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
> > is left turned on.  Is there something I'm missing that makes that cycle
> > inappropriate here?  The main reason to do this is it then looks exactly
> > like any other runtime_pm calls elsewhere in the driver, so easier to review.  
> 
> It's appropriate, although caring about PM when it's disabled in kernel
> config could be unnecessary, IMO. It was my suggestion to keep the h/w
> enabled on driver's removal with !CONFIG_PM, minimizing the code.
> 
For the cost of about 4-8 lines of code, I think it's worth having, but can
also see why you decided against.

Jonathan