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] [day] [month] [year] [list]

Message-ID: <CAE3SzaSvF-JLtUamnib+psG3pONJ9gPxFK+KJ0UtXtb_PfV6_g@mail.gmail.com>
Date: Sat, 30 Aug 2025 08:43: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 v2] iio: light: ltr390: Implement runtime PM support

On Sat, Aug 30, 2025 at 12:19 AM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> 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

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

Thanks for the detailed explanation Andy.
I do not have any technical reason to return value from _put function.
My thought was, since we are doing for _get, we should do it for _put.
I agree with your observations. That being said, I will update the code
as follows:

ltr390_read_raw()
{
   ret = pm_runtime_resume_and_get()
   if (ret < 0)
      return ret;
   ret = __ltr390_read_raw();
   pm_runtime_put_autosuspend();
   return ret;
}