linux-kernel - Re: [PATCH] iio: light: gp2ap002: Fix rumtime PM imbalance on error

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: <CACRpkdY9sDbVSjBULg3tV-Zpr_Tizd4HKeTG-NjCjUru=94QYQ@mail.gmail.com>
Date:   Mon, 12 Apr 2021 13:47:58 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Dinghao Liu <dinghao.liu@....edu.cn>,
        Kangjie Lu <kjlu@....edu>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH] iio: light: gp2ap002: Fix rumtime PM imbalance on error

On Mon, Apr 12, 2021 at 12:16 PM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:

> An example would be the bmc150_magn driver which does exactly the
> same call sequence as this one, but without the reference count increment
> and decrement.  Basically I want to know if there is a problem in
> those other drivers that is being protected against here!

The bmc150_magn driver does not look like I would have done it.

That said, I think it works, because the only thing it is calling is
bmc150_magn_set_power_mode() and that seems stateless,
just poking some regmap bits. The state is tracked by the driver
AFAICT and we don't need pm_runtime_get_noresume() because
it doesn't really matter if the pm_runtime_suspend() callback
gets called immediately or randomly out of sync with what we are
doing from this point.

I would anyways patch it like the gp2ap002 driver because it
is easier to follow the code. Including using only runtime PM
att setting SET_SYSTEM_SLEEP_PM_OPS() to the
pm_runtime_force_suspend and pm_runtime_force_resume
functions, everything just get so much easier when you use
only one type of PM and not two orthogonal ones.

drivers/iio/light/bh1780.c
should be a good example of how to do it idiomatically
because it was reviewed by Ulf Hansson who knows this
runtime PM stuff better than me.

A sequence like this:

   pm_runtime_get_noresume(&client->dev);
   pm_runtime_set_active(&client->dev);
   pm_runtime_enable(&client->dev);
   pm_runtime_set_autosuspend_delay(&client->dev, 5000);
   pm_runtime_use_autosuspend(&client->dev);
   pm_runtime_put(&client->dev);

is very nice because you can clearly see that it will not race
and after the last put() unless something happens the
runtime suspend will kick in after 5000 ms.

Likewise when disabling:

    pm_runtime_get_sync(&client->dev);
    pm_runtime_put_noidle(&client->dev);
    pm_runtime_disable(&client->dev);

same thing: crystal clear there are no races, the device is
definately runtime resumed and we can proceed to
shut down resources explicitly after this point.

If you then add:

SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
                                pm_runtime_force_resume)

Now you have ordinary sleep PM for free. It will just
force the same suspend/resume callbacks and they are
guaranteed to be race free.

This doesn't work for everyone but surprisingly often this is
what you want.

Yours,
Linus Walleij