linux-kernel - Re: [PATCH v7] iio: light: ltr390: Implement runtime PM support

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: <CAHp75VfpQ9c4cptnNGzFYakQxY7JjtUEMDsysS9KJ60xrzaE4g@mail.gmail.com>
Date: Wed, 10 Sep 2025 10:17:00 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, jic23@...nel.org, 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 v7] iio: light: ltr390: Implement runtime PM support

On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
>
> Implement runtime power management for the LTR390 sensor. The device
> autosuspends after 1s of idle time, reducing current consumption from
> 100 µA in active mode to 1 µA in standby mode as per the datasheet.
>
> Ensure that interrupts continue to be delivered with runtime PM.
> Since the LTR390 cannot be used as a wakeup source during runtime
> suspend, therefore increment the runtime PM refcount when enabling
> events and decrement it when disabling events or powering down.
> This prevents event loss while still allowing power savings when IRQs
> are unused.

...

> +static int ltr390_read_raw(struct iio_dev *iio_device,
> +                          struct iio_chan_spec const *chan, int *val,
> +                          int *val2, long mask)

Isn't the mask unsigned long? Jonathan, do we get this fixed already?

Also logical split might be better, i.e. putting val and val2 on the
same line. Then mask will be on the next one

...

>  static void ltr390_powerdown(void *priv)
>  {
>         struct ltr390_data *data = priv;
> +       struct device *dev = &data->client->dev;
> +       int ret;
>
>         guard(mutex)(&data->lock);
>
>         /* Ensure that power off and interrupts are disabled */
> -       if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
> -                               LTR390_LS_INT_EN) < 0)
> -               dev_err(&data->client->dev, "failed to disable interrupts\n");
> +       if (data->irq_enabled) {
> +               ret = regmap_clear_bits(data->regmap, LTR390_INT_CFG, LTR390_LS_INT_EN);
> +               if (ret < 0)
> +                       dev_err(dev, "failed to disable interrupts\n");

In event_config we assure that IRQ is enabled.

> +               data->irq_enabled = false;

Here we may lie about the facts. What will the driver do, if the IRQ
is triggered just before this line?

> +               pm_runtime_put_autosuspend(&data->client->dev);

You have dev, use it.

But where is the symmetrical pm_runtime_get*()?

> +       }
> +
> +       ret = regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> +       if (ret < 0)
> +               dev_err(dev, "failed to disable sensor\n");
> +}


-- 
With Best Regards,
Andy Shevchenko