[PATCH] iio: inkern: fix channel read regression

[PATCH] iio: inkern: fix channel read regression

[Johan Hovold]

A recent "cleanup" broke IIO channel read outs and thereby thermal
mitigation on the Lenovo ThinkPad X13s by returning zero instead of the
expected IIO value type in iio_read_channel_processed_scale():

	thermal thermal_zone12: failed to read out thermal zone (-22)

Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic")
Cc: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/iio/inkern.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


Someone should re-review the offending commit so that there are no more
of these subtle regressions that are bound to happen when people use
cleanup.h to save a few lines of code at the cost of readability.

Johan



diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 52d773261828..485e6fc44a04 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -721,7 +721,7 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
 			return ret;
 		*val *= scale;
 
-		return 0;
+		return ret;
 	} else {
 		ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW);
 		if (ret < 0)
-- 
2.44.1

Re: [PATCH] iio: inkern: fix channel read regression

[Jonathan Cameron]

On Thu, 30 May 2024 09:44:16 +0200
Johan Hovold <johan+linaro@kernel.org> wrote:

> A recent "cleanup" broke IIO channel read outs and thereby thermal
> mitigation on the Lenovo ThinkPad X13s by returning zero instead of the
> expected IIO value type in iio_read_channel_processed_scale():
> 
> 	thermal thermal_zone12: failed to read out thermal zone (-22)
> 
> Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic")
> Cc: Nuno Sa <nuno.sa@analog.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Hi Johan,

The documentation for this function unfortunately says 
* Returns an error code or 0
so not surprising this went wrong.

All those docs should be dragged alongside the implementations
to make it easier to spot errors and seems we need to do some
fixing up of those docs in general :(

In meantime, Nuno please take another look at these and see if
we have additional problem cases like this.  Given the patch
queue I have and a busy few days it will be a while before I
get to it but I'll try and take a close look soon as well.

Longer term, in my view the readability and chance of bugs
is reduced, but churn always introduces the possibility of
issues like this in the short term :(


Jonathan


> ---
>  drivers/iio/inkern.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> Someone should re-review the offending commit so that there are no more
> of these subtle regressions that are bound to happen when people use
> cleanup.h to save a few lines of code at the cost of readability.
> 
> Johan
> 
> 
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 52d773261828..485e6fc44a04 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -721,7 +721,7 @@ int iio_read_channel_processed_scale(struct iio_channel *chan, int *val,
>  			return ret;
>  		*val *= scale;
>  
> -		return 0;
> +		return ret;
>  	} else {
>  		ret = iio_channel_read(chan, val, NULL, IIO_CHAN_INFO_RAW);
>  		if (ret < 0)

Re: [PATCH] iio: inkern: fix channel read regression

[Johan Hovold]

Hi Jonathan,

On Sat, Jun 01, 2024 at 02:21:47PM +0100, Jonathan Cameron wrote:
> On Thu, 30 May 2024 09:44:16 +0200
> Johan Hovold <johan+linaro@kernel.org> wrote:
> 
> > A recent "cleanup" broke IIO channel read outs and thereby thermal
> > mitigation on the Lenovo ThinkPad X13s by returning zero instead of the
> > expected IIO value type in iio_read_channel_processed_scale():
> > 
> > 	thermal thermal_zone12: failed to read out thermal zone (-22)
> > 
> > Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic")
> > Cc: Nuno Sa <nuno.sa@analog.com>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

> In meantime, Nuno please take another look at these and see if
> we have additional problem cases like this.  Given the patch
> queue I have and a busy few days it will be a while before I
> get to it but I'll try and take a close look soon as well.

Please consider getting this one into mainline as soon as possible as it
breaks thermal mitigation, which can cause literal skin burns, on all
Qualcomm platforms (and probably other platforms too).

> Longer term, in my view the readability and chance of bugs
> is reduced, but churn always introduces the possibility of
> issues like this in the short term :(

I just worry about some of the cleanup.h conversion I've seen where
inexperienced developers potentially break tested and reviewed code for
something which is often not very readable and for very little gain.

Johan

Re: [PATCH] iio: inkern: fix channel read regression

[Jonathan Cameron]

On Mon, 3 Jun 2024 09:24:38 +0200
Johan Hovold <johan@kernel.org> wrote:

> Hi Jonathan,
> 
> On Sat, Jun 01, 2024 at 02:21:47PM +0100, Jonathan Cameron wrote:
> > On Thu, 30 May 2024 09:44:16 +0200
> > Johan Hovold <johan+linaro@kernel.org> wrote:
> >   
> > > A recent "cleanup" broke IIO channel read outs and thereby thermal
> > > mitigation on the Lenovo ThinkPad X13s by returning zero instead of the
> > > expected IIO value type in iio_read_channel_processed_scale():
> > > 
> > > 	thermal thermal_zone12: failed to read out thermal zone (-22)
> > > 
> > > Fixes: 3092bde731ca ("iio: inkern: move to the cleanup.h magic")
> > > Cc: Nuno Sa <nuno.sa@analog.com>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>  
> 
> > In meantime, Nuno please take another look at these and see if
> > we have additional problem cases like this.  Given the patch
> > queue I have and a busy few days it will be a while before I
> > get to it but I'll try and take a close look soon as well.  
> 
> Please consider getting this one into mainline as soon as possible as it
> breaks thermal mitigation, which can cause literal skin burns, on all
> Qualcomm platforms (and probably other platforms too).

I'll queue this fix and we can follow with any others that surface
from reviews.

Need it to sit in linux-next for a day or two before a pull request though
as I've just pushed some other fixes out that needs some build coverage.

> 
> > Longer term, in my view the readability and chance of bugs
> > is reduced, but churn always introduces the possibility of
> > issues like this in the short term :(  
> 
> I just worry about some of the cleanup.h conversion I've seen where
> inexperienced developers potentially break tested and reviewed code for
> something which is often not very readable and for very little gain.
> 

Fair comment. There have been some 'interesting' patches.
On the plus side this one highlighted that our docs are wrong.

Applied to the fixes-togreg branch of iio.git

Jonathan


> Johan