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: <CAE3SzaTZ8PXM_B8FBetOTSfz2myGZ=WzPp8h2d79Q95zKLq5hw@mail.gmail.com>
Date: Wed, 10 Sep 2025 18:06:32 +0530
From: Akshay Jindal <akshayaj.lkd@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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

Hi Andy,
Thank you very much for your valuable feedback.
I do have a small request regarding the review process. Over the past 3–4
versions,most of the comments have been about fixing indentations and
improving code readability. I would kindly request if it would be possible
to consolidate such cosmetic comments into a single review round.

I completely understand that incremental feedback makes sense when the code
is actively changing, but if the changes are minimal, spreading out minor
suggestions over multiple review cycles tends to unnecessarily increase the
turnaround time.

Your support in this would help me address the comments more efficiently.

On Wed, Sep 10, 2025 at 12:47 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Tue, Sep 9, 2025 at 10:47 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
> > +static int ltr390_read_raw(struct iio_dev *iio_device,
> > +                          struct iio_chan_spec const *chan, int *val,
> > +                          int *val2, long mask)
>
>
> Also logical split might be better, i.e. putting val and val2 on the
> same line. Then mask will be on the next one
Ok, will fix.

> >  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.
What do you mean here?
>
> > +               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?
I don't see why the device will trigger an IRQ, when we are disabling
the INT via
regmap_clear_bits before this.

>
> > +               pm_runtime_put_autosuspend(&data->client->dev);
>
> You have dev, use it.
Ok, will fix.

>
> But where is the symmetrical pm_runtime_get*()?

This is the fundamental approach of managing IRQ handling + runtime PM.
suggested by Jonathan in preliminary rounds and employed by many drivers.
"When enabling IRQ, increase the refcount, and decrease when disabling"
This is done because ltr390 does not have a wakeup functionality.

put_autosuspend is tied to disable which can happen in 2 places:
1. event_config.
2. powerdown (if irq enabled).

pm_runtime_get* is tied to enable which can happen only at 1 place:
1. event_config.

If IRQ was enabled before power down, that means in event_config
we had already called pm_runtime_get* and increased the refcount to 1.
This will come down to 0 as a result of either of disabling event_config
or powerdown.

Thanks,
Akshay.