| 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
| ||
|
Message-ID: <20250825152608.6468c27b@jic23-huawei>
Date: Mon, 25 Aug 2025 15:26:08 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, shuah@...nel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: ltr390: Add runtime PM support
On Fri, 22 Aug 2025 23:33:26 +0530
Akshay Jindal <akshayaj.lkd@...il.com> wrote:
> Implement runtime power management for the LTR390 sensor.
> The device would now autosuspend after 1s of idle time.
> This would save the overall power consumption by the sensor.
>
> Ensure that interrupts continue to be delivered during
> runtime suspend by disabling the sensor only when no
> interrupts are enabled. This prevents loss of events
> while still allowing power savings when IRQs are unused.
Wrap closer to 75 chars.
Main comment that follows is that runtime pm is reference
counted. That is you can take multiple references in different
paths at the same time and only when they are all released does
the put actually cause it to be suspended. So for events
just grab an extra reference. Lots of drivers do this in the
buffer enables for example - probably some in event handling
as well I just can't remember which one right now and am too lazy
to go find out.
>
> Signed-off-by: Akshay Jindal <akshayaj.lkd@...il.com>
> ---
>
> Testing summary:
> ================
> -> Tested on Raspberrypi 4B. Following tests were performed.
> 1. Verified that /sys/bus/i2c/devices/i2c-1/1-0053/power/control contains ‘auto’ value.
> 2. Verified the /sys/bus/i2c/devices/i2c-1/1-0053/power/autosuspend_delay_ms contains 1000 which is assigned by the driver.
> 3. Changed the autosuspend_delay_ms value from 1000 to 2000ms and verified it.
> 3.1 Verified through the timestamp that whatever autosuspend_delay_ms is set, it is being honoured.
> 4. Verified that runtime_suspend and runtime_resume callbacks are called whenever any IO is done on a channel attribute.
> 4.1 Verified that power/runtime_status first becomes active and then becomes suspended.
> 4.2 Verified that power/runtime_active_time keeps on increasing with a delta of autosuspend_delay_ms.
>
> Interrupt Handling Verification:
> --------------------------------
> 1. Verified that when interrupts are enabled on the device, then the device does not get put in standby mode and keeps sampling.
> a. As a result interrupts are delivered to the driver and are handled.
> 2. Verified that when interrupts are disabled, the device is put in standby mode and stops sampling.
> a.Since there is no sampling, so no IRQs will be generated. They are only generated when the device is resumed due to I/O on some sysfs attribute from userspace.
>
> drivers/iio/light/ltr390.c | 246 +++++++++++++++++++++++++++++--------
> 1 file changed, 193 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index 2e1cf62e8201..9e2f33a401f2 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -30,6 +30,7 @@
>
> #include <linux/iio/iio.h>
> #include <linux/iio/events.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/unaligned.h>
>
> @@ -105,6 +106,7 @@ struct ltr390_data {
> enum ltr390_mode mode;
> int gain;
> int int_time_us;
> + bool irq_enabled;
> };
>
> static const struct regmap_range ltr390_readable_reg_ranges[] = {
> @@ -154,6 +156,25 @@ static const int ltr390_samp_freq_table[][2] = {
> [7] = { 500, 2000 },
> };
>
> +static int ltr390_set_power_state(struct ltr390_data *data, bool on)
> +{
> + struct device *dev = &data->client->dev;
> + int ret = 0;
> +
> + if (on) {
> + ret = pm_runtime_resume_and_get(dev);
David touched on this. Put the calls inline - there is no benefit to this
function as it calls one of two unrelated paths at each call site.
> + if (ret) {
> + dev_err(dev, "failed to resume runtime PM: %d\n", ret);
> + return ret;
> + }
> + } else {
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + }
> +
> + return ret;
> +}
> +
> static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> {
> struct device *dev = &data->client->dev;
> @@ -223,61 +244,76 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
> struct ltr390_data *data = iio_priv(iio_device);
>
> guard(mutex)(&data->lock);
> +
> + ltr390_set_power_state(data, true);
Can fail so you should check.
Wrap ltr390_register_read() in an outer function that does the powerstate
management. Then no need to have all the gotos in here.
I am intending to see what appetite there is for a ACQUIRE() conditional
guard set of macros around autosuspend, but for now a separate wrapper function
is the cleanest path. Be careful with the locking though.
> +
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> switch (chan->type) {
> case IIO_UVINDEX:
> ret = ltr390_set_mode(data, LTR390_SET_UVS_MODE);
> if (ret < 0)
> - return ret;
> + goto handle_pm;
>
> ret = ltr390_register_read(data, LTR390_UVS_DATA);
> if (ret < 0)
> - return ret;
> + goto handle_pm;
> break;
>
> case IIO_LIGHT:
> ret = ltr390_set_mode(data, LTR390_SET_ALS_MODE);
> if (ret < 0)
> - return ret;
> + goto handle_pm;
>
> ret = ltr390_register_read(data, LTR390_ALS_DATA);
> if (ret < 0)
> - return ret;
> + goto handle_pm;
> break;
>
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> + goto handle_pm;
> }
> *val = ret;
> - return IIO_VAL_INT;
> + ret = IIO_VAL_INT;
> + break;
> +
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_UVINDEX:
> *val = LTR390_WINDOW_FACTOR * LTR390_FRACTIONAL_PRECISION;
> *val2 = ltr390_counts_per_uvi(data);
> - return IIO_VAL_FRACTIONAL;
> + ret = IIO_VAL_FRACTIONAL;
> + break;
>
> case IIO_LIGHT:
> *val = LTR390_WINDOW_FACTOR * 6 * 100;
> *val2 = data->gain * data->int_time_us;
> - return IIO_VAL_FRACTIONAL;
> + ret = IIO_VAL_FRACTIONAL;
> + break;
>
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> + break;
>
> case IIO_CHAN_INFO_INT_TIME:
> *val = data->int_time_us;
> - return IIO_VAL_INT;
> + ret = IIO_VAL_INT;
> + break;
>
> case IIO_CHAN_INFO_SAMP_FREQ:
> *val = ltr390_get_samp_freq_or_period(data, LTR390_GET_FREQ);
> - return IIO_VAL_INT;
> + ret = IIO_VAL_INT;
> + break;
>
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> +
> +handle_pm:
> + ltr390_set_power_state(data, false);
> + return ret;
> }
> @@ -595,32 +670,43 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
> struct ltr390_data *data = iio_priv(indio_dev);
> int ret;
>
> - if (!state)
> - return regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> + ltr390_set_power_state(data, true);
>
> guard(mutex)(&data->lock);
> +
> + if (!state) {
> + ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> + data->irq_enabled = false;
Just take an extra reference to runtime pm on enable of event and put it disable.
Then no need for special handling with a local flag etc.
> +static int ltr390_pm_init(struct ltr390_data *data)
> +{
> + int ret;
> + struct device *dev = &data->client->dev;
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_runtime_enable(dev);
devm_pm_runtime_set_active_enabled() I think would work here.
That shortens this setup enough I'd not bother with this function.
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to enable powermanagement\n");
> +
> + pm_runtime_set_autosuspend_delay(dev, 1000);
> + pm_runtime_use_autosuspend(dev);
> + return 0;
> +}
> +
> static int ltr390_probe(struct i2c_client *client)
> {
> struct ltr390_data *data;
> @@ -708,6 +820,8 @@ static int ltr390_probe(struct i2c_client *client)
> if (!indio_dev)
> return -ENOMEM;
>
> + i2c_set_clientdata(client, indio_dev);
> +
> data = iio_priv(indio_dev);
> data->regmap = devm_regmap_init_i2c(client, <r390_regmap_config);
> if (IS_ERR(data->regmap))
> @@ -721,6 +835,8 @@ static int ltr390_probe(struct i2c_client *client)
> data->gain = 3;
> /* default mode for ltr390 is ALS mode */
> data->mode = LTR390_SET_ALS_MODE;
> + /* default irq_enabled is false */
> + data->irq_enabled = false;
>
> mutex_init(&data->lock);
>
> @@ -763,6 +879,7 @@ static int ltr390_probe(struct i2c_client *client)
> "request irq (%d) failed\n", client->irq);
> }
>
> + ltr390_pm_init(data);
> return devm_iio_device_register(dev, indio_dev);
> }
>
> @@ -784,7 +901,30 @@ static int ltr390_resume(struct device *dev)
> LTR390_SENSOR_ENABLE);
> }
>
> -static DEFINE_SIMPLE_DEV_PM_OPS(ltr390_pm_ops, ltr390_suspend, ltr390_resume);
> +static int ltr390_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct ltr390_data *data = iio_priv(indio_dev);
> +
> + guard(mutex)(&data->lock);
> + if (data->irq_enabled)
As above. When you have events enabled, grab an extra reference and don't
put it until you disable the event. That way we never enter
runtime_suspend whilst they are enabled.
> + return 0;
> + return regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
> + LTR390_SENSOR_ENABLE);
> +}
> +
> +static int ltr390_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct ltr390_data *data = iio_priv(indio_dev);
> +
> + return regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> + LTR390_SENSOR_ENABLE);
> +}
> +
> +static _DEFINE_DEV_PM_OPS(ltr390_pm_ops,
> + ltr390_suspend, ltr390_resume,
> + ltr390_runtime_suspend, ltr390_runtime_resume, NULL);
>
> static const struct i2c_device_id ltr390_id[] = {
> { "ltr390" },
> @@ -802,7 +942,7 @@ static struct i2c_driver ltr390_driver = {
> .driver = {
> .name = "ltr390",
> .of_match_table = ltr390_of_table,
> - .pm = pm_sleep_ptr(<r390_pm_ops),
> + .pm = pm_ptr(<r390_pm_ops),
> },
> .probe = ltr390_probe,
> .id_table = ltr390_id,
Powered by blists - more mailing lists