linux-kernel - Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes

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] [day] [month] [year] [list]

Date:   Fri, 16 Sep 2022 16:22:14 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Crt Mori <cmo@...exis.com>
CC:     Jonathan Cameron <jic23@...nel.org>, <linux-iio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v3 1/3] iio: temperature: mlx90632 Add runtime
 powermanagement modes

On Thu, 15 Sep 2022 16:35:33 +0200
Crt Mori <cmo@...exis.com> wrote:

> > > +     pm_runtime_get_sync(dev);  
> > So, this isn't quite enough.
> >
> > Take a look at what devm_pm_runtime_enable()
> > does as the documentation for
> > pm_runtime_use_autosuspend()
> >
> > I'd suggest using devm_pm_runtime_enable() and
> > an additional callback to turn the device on that
> > is registered after devm_pm_runtime_enable()
> > (so will maintain the ordering you have here).
> >
> >
 Oops. I should have looked for a reply before responding to your v4.

> >  
> I copied this from pressure/bmp280-core driver. I had devm_pm
> originally, but was asked to replace it.

It is a little odd to mix and match, but I think it makes sense in
this case.  Can see why people might disagree (maybe it was me!)

> 
> > > +     pm_runtime_put_noidle(dev);
> > > +     pm_runtime_disable(dev);
> > > +}
> > > +
> > >  static int mlx90632_probe(struct i2c_client *client,
> > >                         const struct i2c_device_id *id)
> > >  {
> > > @@ -902,6 +1132,7 @@ static int mlx90632_probe(struct i2c_client *client,
> > >       mlx90632->client = client;
> > >       mlx90632->regmap = regmap;
> > >       mlx90632->mtyp = MLX90632_MTYP_MEDICAL;
> > > +     mlx90632->powerstatus = MLX90632_PWR_STATUS_HALT;
> > >
> > >       mutex_init(&mlx90632->lock);
> > >       indio_dev->name = id->name;
> > > @@ -961,16 +1192,25 @@ static int mlx90632_probe(struct i2c_client *client,
> > >
> > >       mlx90632->emissivity = 1000;
> > >       mlx90632->object_ambient_temperature = 25000; /* 25 degrees milliCelsius */
> > > +     mlx90632->interaction_ts = jiffies; /* Set initial value */
> > >
> > > -     pm_runtime_disable(&client->dev);
> > > +     pm_runtime_get_noresume(&client->dev);
> > >       ret = pm_runtime_set_active(&client->dev);
> > >       if (ret < 0) {
> > >               mlx90632_sleep(mlx90632);
> > >               return ret;
> > >       }
> > > +
> > >       pm_runtime_enable(&client->dev);
> > >       pm_runtime_set_autosuspend_delay(&client->dev, MLX90632_SLEEP_DELAY_MS);
> > >       pm_runtime_use_autosuspend(&client->dev);
> > > +     pm_runtime_put_autosuspend(&client->dev);
> > > +
> > > +     ret = devm_add_action_or_reset(&client->dev, mlx90632_pm_disable, &client->dev);  
> >
> > Having moved those over to devm you need to also have dropped the calls in remove()
> > (I only noticed this whilst trying to fix the autosuspend issue above.)  
> 
> So in remove, there should be no pm calls, because mlx90632_pm_disable
> function handle all of it? I still keep the sleep call, so that it
> also puts the sensor in lowest state, or I rather keep it only in
> regulator_disable (which should also be called at removal)?

No, you still need some calls, because the devm_pm_runtime_enable()
registers a callback that only does part of what you have - it leaves
the device in an unknown state. If you want to power it up again so
that you can in turn switch it off cleanly (and hence handle the
non runtime pm case nicely) then you still need to do that.

The sleep is still useful because we may not have a controllable regulator.
In that case we want to go to a low power state anyway.

If we do have a controllable regulator, then sleeping followed by hitting
the power button does no harm.


Jonathan


> 
> > > +     if (ret) {
> > > +             mlx90632_sleep(mlx90632);
> > > +             return ret;
> > > +     }
> > >
> > >       return iio_device_register(indio_dev);
> > >  }  
> >