Date:   Sun, 18 Apr 2021 10:43:53 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        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, 12 Apr 2021 13:47:58 +0200
Linus Walleij <linus.walleij@...aro.org> wrote:

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

I'm still far from completely convinced that it is 'necessary'
to take the reference whilst going through this sequence because
there is nothing to kick off the suspend until we tell it to use
autosuspend.  However, I appreciate (much like taking locks in
general in probe) that it makes it easy to see there is no race.

Anyhow, fix is still valid either way so applied to the fixes
togreg branch of iio.git with a fixes tag added to the initial
introduction of the driver (which I think is where this came in).

Thanks,

Jonathan

> 
> Yours,
> Linus Walleij

Powered by blists - more mailing lists