linux-kernel - Re: [PATCH v3] 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: <CAE3SzaSYLFFRL4OuqUbk8J0dWCuxedCyGiX2_tJySG1FC=w95g@mail.gmail.com>
Date: Sat, 30 Aug 2025 18:54:05 +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 v3] iio: light: ltr390: Implement runtime PM support

Thanks for the speedy review Andy. Follow-up inline.

On Sat, Aug 30, 2025 at 6:04 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Sat, Aug 30, 2025 at 2:35 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
> > @@ -27,6 +27,7 @@
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/regmap.h>
> > +#include <linux/pm_runtime.h>
>
> You missed my comment.
Yeah, this got missed. Will address this.

>
> > +static int ltr390_write_event_config(struct iio_dev *indio_dev,
> > +                               const struct iio_chan_spec *chan,
> > +                               enum iio_event_type type,
> > +                               enum iio_event_direction dir,
> > +                               bool state)
> > +{
> > +       int ret;
> > +       struct ltr390_data *data = iio_priv(indio_dev);
> > +       struct device *dev = &data->client->dev;
> > +
> > +       guard(mutex)(&data->lock);
> > +
> > +       if (state && !data->irq_enabled) {
> > +               ret = pm_runtime_resume_and_get(dev);
> > +               if (ret < 0) {
> > +                       dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> > +                       return ret;
> > +               }
> > +               data->irq_enabled = true;
> > +       }
> > +
> > +       ret = __ltr390_write_event_config(indio_dev, chan, type, dir, state);
> > +
> > +       if (!state && data->irq_enabled) {
> > +               data->irq_enabled = false;
> > +               pm_runtime_put_autosuspend(dev);
> > +       }
> > +
> > +       return ret;
> > +}
>
> This looks like overcomplicated and duplicate checks. Just make two
> functions with and without IRQ enabling handling.
>
LTR390 only supports 1 event/interrupt which is toggled in this callback based
on value provided to sysfs entry. There cannot be a version of this without IRQ
handling. It is supposed to do IRQ handling only.

Pseudo code of the said function will be something as follows:
ltr390_write_event_config() {
if (interrupt needs to be enabled && previously it was disabled)
     pm_runtime_resume_and_get()
do_actual_reg_writes()
if (interrupt needs to be disabled && previously it was enabled)
    pm_runtime_put_autosuspend().
}

With the current function , we achieve the following objectives:
1. idempotency in refcount change. Meaning if IRQ is already enabled and
if someone enables it again, it will not increase the refcount, same goes for
double disable case. This has been tested as well.
2. Only if the new and previous config is different, then only the refcount will
change.
3. Adheres to previous comments received regarding checking return value
of _get and ignoring that of _put.

I genuinely don't see any duplicate checks here. In addition, I feel the above
function is fine from a simplification point of view and cannot be bifurcated
further.

Although, if you could clarify what you mean by further bifurcation, I might be
able to connect with your thoughts.

Thanks,
Akshay