linux-kernel - Re: [PATCH v2] 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: <CAHp75Vf5--WGoeb9qu0QoYz0PmnexUvLyChJFcwYAW3u=_nOwg@mail.gmail.com>
Date: Fri, 29 Aug 2025 21:49:06 +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 v2] iio: light: ltr390: Implement runtime PM support

On Fri, Aug 29, 2025 at 9:03 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
> On Fri, Aug 29, 2025 at 9:16 PM Andy Shevchenko
> <andy.shevchenko@...il.com> wrote:
> > On Thu, Aug 28, 2025 at 9:47 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
> > > On Thu, Aug 28, 2025 at 7:17 AM Andy Shevchenko
> > > <andy.shevchenko@...il.com> wrote:
> > > > On Wed, Aug 27, 2025 at 10:19 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)
> > > > > +{
> > > > > +       int ret, retval;
> > > > > +       struct ltr390_data *data = iio_priv(iio_device);
> > > > > +       struct device *dev = &data->client->dev;
> > > > > +
> > > > > +       ret = pm_runtime_resume_and_get(dev);
> > > > > +       if (ret < 0)
> > > > > +               dev_err(dev, "runtime PM failed to resume: %d\n", ret);
> > > >
> > > > If it fails, there is no point to read the value, it will be garbage
> > > > or even can make the bus stuck.
> > > >
> > > My rationale behind this approach is that earlier ltr390_read_raw()
> > > had all the functionality
> > > of the .read_raw callback so the return value whether success or
> > > failure was of the core functionality.
> > > But now, since the core functionality has been relocated into
> > > __ltr390_read_raw(), I felt the return value
> > > ltr390_read_raw should be the return value of __ltr390_read_raw().
> >
> > "Main" returned value. But this is not the point. The Q is, how do you
> > expect to get not garbage from, e.g., powered off device, please?
>
> I got your point.

I don't think so.

> My Rationale:
> ------------------
> From a functionality point of view, if before runtime PM usage, the
> core retval was being
> returned (success/failure), then now with runtime PM also, it should
> be the same core retval
> which should be returned, but I think that is not correct thinking.

>From the pure SW concept that's correct, but the important detail is
we are communicating to the device via the special bus like i²c.

> Updated Approach:
> -------------------------
> Since we are introducing new functionality of runtime PM in the
> callbacks, the return
> value should reflect ANY failure in the callback, be it failure of
> core functionality
> or failure of runtime PM calls.

It's not about SW or refactoring. Think about what will happen on the
actual line. The I²C bus is very sensitive to the issues on the line.

> With this in mind, I will be
> refactoring the callbacks
> as follows:

But the end result is in the right direction.

> ret = pm_runtime_resume_and_get();
> if (ret < 0)
>      return ret; //main functionality will not be called.
> retval = __ltr390_read_raw();
> // not checking retval because irrespective of retval we need refcount
> decrement and suspension.

> ret = pm_runtime_put_autosuspend();-
> if (ret < 0)
>      return ret;

So, returning an error here, how would it help? For example, if this
returns -EAGAIN and user space tries again, we will have always on
device. The same, btw, will happen if we do not check for the error
code here. That said, I don't know why we should care about returning
value from _put_autosuspend. I have just checked randomly chosen 20+
drivers in the v6.16 and only 2 check for error:
- one for the pure message printing (say the same as not checking)
- one for real, BUT it is done in the specific PM related method that
does change the power state and callers know about this.
And it seems that all of them (20+ drivers) check for the _resume
errors. I really need a good explanation of your discoveries why in
the normal case we need to check for the error code on _put part of
runtime PM. I might learn something new.

> return retval;

> Let me know your thoughts.



-- 
With Best Regards,
Andy Shevchenko