[v1] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

[PATCH] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

Posted by Hsin-Te Yuan 10 months ago

When userspace nonblocking reads temperature via sysfs, EAGAIN error
returned by thermal driver will confuse user from the usual meaning of
EAGAIN, the read would block. Change to throw ENODATA instead of EAGAIN
to userspace. Also, ENODATA more accurately reflects that data is not
currently available.

Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
---
 drivers/thermal/thermal_sysfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 24b9055a0b6c515b865e0d7e2db1d0de176ff767..3d1713e053dfb867933d95131f1f2491d2ecd07e 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -40,8 +40,11 @@ temp_show(struct device *dev, struct device_attribute *attr, char *buf)
 
 	ret = thermal_zone_get_temp(tz, &temperature);
 
-	if (ret)
+	if (ret) {
+		if (ret == -EAGAIN)
+			return -ENODATA;
 		return ret;
+	}
 
 	return sprintf(buf, "%d\n", temperature);
 }

---
base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
change-id: 20250409-temp-6ebd13ad0dbd

Best regards,
-- 
Hsin-Te Yuan <yuanhsinte@chromium.org>

Re: [PATCH] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

Posted by Rafael J. Wysocki 10 months ago

On Wed, Apr 9, 2025 at 11:13 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
>
> When userspace nonblocking reads temperature via sysfs, EAGAIN error
> returned by thermal driver will confuse user from the usual meaning of
> EAGAIN, the read would block.

Why would it block?

> Change to throw ENODATA instead of EAGAIN to userspace.

Casting error codes tends to be confusing.

> Also, ENODATA more accurately reflects that data is not currently available.

It means something else, "try again" vs "no data available (possibly at all)".

The error code comes from the thermal driver and if it wants to say
"try again" then this is what it wants to say.

> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> ---
>  drivers/thermal/thermal_sysfs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 24b9055a0b6c515b865e0d7e2db1d0de176ff767..3d1713e053dfb867933d95131f1f2491d2ecd07e 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -40,8 +40,11 @@ temp_show(struct device *dev, struct device_attribute *attr, char *buf)
>
>         ret = thermal_zone_get_temp(tz, &temperature);
>
> -       if (ret)
> +       if (ret) {
> +               if (ret == -EAGAIN)
> +                       return -ENODATA;
>                 return ret;
> +       }
>
>         return sprintf(buf, "%d\n", temperature);
>  }
>
> ---
> base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
> change-id: 20250409-temp-6ebd13ad0dbd
>
> Best regards,
> --
> Hsin-Te Yuan <yuanhsinte@chromium.org>
>
>

Re: [PATCH] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

Posted by Hsin-Te Yuan 10 months ago

On Thu, Apr 10, 2025 at 1:31 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Apr 9, 2025 at 11:13 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> >
> > When userspace nonblocking reads temperature via sysfs, EAGAIN error
> > returned by thermal driver will confuse user from the usual meaning of
> > EAGAIN, the read would block.
>
> Why would it block?

I mean EAGAIN returned by nonblocking read implies the read would
block (source: manpage of read)
>
> > Change to throw ENODATA instead of EAGAIN to userspace.
>
> Casting error codes tends to be confusing.
>
> > Also, ENODATA more accurately reflects that data is not currently available.
>
> It means something else, "try again" vs "no data available (possibly at all)".
>
> The error code comes from the thermal driver and if it wants to say
> "try again" then this is what it wants to say.

Yes, but EAGAIN has special meaning when returned by nonblocking read.
Hence, we need to avoid returning EAGAIN to userspace and ENODATA is
the most suitable alternative in my opinion.

>
> > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > ---
> >  drivers/thermal/thermal_sysfs.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > index 24b9055a0b6c515b865e0d7e2db1d0de176ff767..3d1713e053dfb867933d95131f1f2491d2ecd07e 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -40,8 +40,11 @@ temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> >
> >         ret = thermal_zone_get_temp(tz, &temperature);
> >
> > -       if (ret)
> > +       if (ret) {
> > +               if (ret == -EAGAIN)
> > +                       return -ENODATA;
> >                 return ret;
> > +       }
> >
> >         return sprintf(buf, "%d\n", temperature);
> >  }
> >
> > ---
> > base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
> > change-id: 20250409-temp-6ebd13ad0dbd
> >
> > Best regards,
> > --
> > Hsin-Te Yuan <yuanhsinte@chromium.org>
> >
> >

Re: [PATCH] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

Posted by Rafael J. Wysocki 10 months ago

On Thu, Apr 10, 2025 at 11:59 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
>
> On Thu, Apr 10, 2025 at 1:31 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Apr 9, 2025 at 11:13 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> > >
> > > When userspace nonblocking reads temperature via sysfs, EAGAIN error
> > > returned by thermal driver will confuse user from the usual meaning of
> > > EAGAIN, the read would block.
> >
> > Why would it block?
>
> I mean EAGAIN returned by nonblocking read implies the read would
> block (source: manpage of read)
> >
> > > Change to throw ENODATA instead of EAGAIN to userspace.
> >
> > Casting error codes tends to be confusing.
> >
> > > Also, ENODATA more accurately reflects that data is not currently available.
> >
> > It means something else, "try again" vs "no data available (possibly at all)".
> >
> > The error code comes from the thermal driver and if it wants to say
> > "try again" then this is what it wants to say.
>
> Yes, but EAGAIN has special meaning when returned by nonblocking read.
> Hence, we need to avoid returning EAGAIN to userspace and ENODATA is
> the most suitable alternative in my opinion.

So any sysfs interface returning an error code would be affected by
this, wouldn't it?

Why do you want to change this particular one?

> >
> > > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > > ---
> > >  drivers/thermal/thermal_sysfs.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > > index 24b9055a0b6c515b865e0d7e2db1d0de176ff767..3d1713e053dfb867933d95131f1f2491d2ecd07e 100644
> > > --- a/drivers/thermal/thermal_sysfs.c
> > > +++ b/drivers/thermal/thermal_sysfs.c
> > > @@ -40,8 +40,11 @@ temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> > >
> > >         ret = thermal_zone_get_temp(tz, &temperature);
> > >
> > > -       if (ret)
> > > +       if (ret) {
> > > +               if (ret == -EAGAIN)
> > > +                       return -ENODATA;
> > >                 return ret;
> > > +       }
> > >
> > >         return sprintf(buf, "%d\n", temperature);
> > >  }
> > >
> > > ---
> > > base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
> > > change-id: 20250409-temp-6ebd13ad0dbd
> > >
> > > Best regards,
> > > --
> > > Hsin-Te Yuan <yuanhsinte@chromium.org>
> > >
> > >

Re: [PATCH] thermal: sysfs: Return ENODATA instead of EAGAIN for reads

Posted by Hsin-Te Yuan 10 months ago

On Thu, Apr 10, 2025 at 6:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Apr 10, 2025 at 11:59 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> >
> > On Thu, Apr 10, 2025 at 1:31 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Apr 9, 2025 at 11:13 AM Hsin-Te Yuan <yuanhsinte@chromium.org> wrote:
> > > >
> > > > When userspace nonblocking reads temperature via sysfs, EAGAIN error
> > > > returned by thermal driver will confuse user from the usual meaning of
> > > > EAGAIN, the read would block.
> > >
> > > Why would it block?
> >
> > I mean EAGAIN returned by nonblocking read implies the read would
> > block (source: manpage of read)
> > >
> > > > Change to throw ENODATA instead of EAGAIN to userspace.
> > >
> > > Casting error codes tends to be confusing.
> > >
> > > > Also, ENODATA more accurately reflects that data is not currently available.
> > >
> > > It means something else, "try again" vs "no data available (possibly at all)".
> > >
> > > The error code comes from the thermal driver and if it wants to say
> > > "try again" then this is what it wants to say.
> >
> > Yes, but EAGAIN has special meaning when returned by nonblocking read.
> > Hence, we need to avoid returning EAGAIN to userspace and ENODATA is
> > the most suitable alternative in my opinion.
>
> So any sysfs interface returning an error code would be affected by
> this, wouldn't it?
>
> Why do you want to change this particular one?

The background is that the implementation of read in golang tries to
nonblocking read a file and poll it when that read returns EAGAIN.
That implementation is following POSIX spec. However, the sysfs node
of the thermal zone can't be polled since the thermal sensor doesn't
inform kernel when data is ready.
Hence, the poll in golang implementation never returns and makes read
never return when the thermal driver returns EAGAIN.

I'm not sure whether other sysfs interfaces have this issue or not.
Simultaneously satisfying both returning EAGAIN when reading and not
supporting being polled might not be that common.

>
> > >
> > > > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> > > > ---
> > > >  drivers/thermal/thermal_sysfs.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > > > index 24b9055a0b6c515b865e0d7e2db1d0de176ff767..3d1713e053dfb867933d95131f1f2491d2ecd07e 100644
> > > > --- a/drivers/thermal/thermal_sysfs.c
> > > > +++ b/drivers/thermal/thermal_sysfs.c
> > > > @@ -40,8 +40,11 @@ temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > >
> > > >         ret = thermal_zone_get_temp(tz, &temperature);
> > > >
> > > > -       if (ret)
> > > > +       if (ret) {
> > > > +               if (ret == -EAGAIN)
> > > > +                       return -ENODATA;
> > > >                 return ret;
> > > > +       }
> > > >
> > > >         return sprintf(buf, "%d\n", temperature);
> > > >  }
> > > >
> > > > ---
> > > > base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
> > > > change-id: 20250409-temp-6ebd13ad0dbd
> > > >
> > > > Best regards,
> > > > --
> > > > Hsin-Te Yuan <yuanhsinte@chromium.org>
> > > >
> > > >