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: <20250910120245.000033e8@huawei.com>
Date: Wed, 10 Sep 2025 12:02:45 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Akshay Jindal <akshayaj.lkd@...il.com>, <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 Wed, 10 Sep 2025 10:17:00 +0300
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

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

Whilst it could (and probably should) be unsigned, it's not actually a mask.
That naming is a historical mess up / evolution thing - long ago it was a bitmap.
It is now the index of a bit in the mask.  So this is unrelated(ish) to the
recent fixes around the actual bitmaps/bitmasks.

Changing this one is a lot more painful than the recent fix to the infomask
as it means changing the signature in every driver.
I'm doubtful on whether this one is worth the churn.

> 
> 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");
> > +}  
> 
>