Received-SPF: pass (google.com: domain of linux-kernel+bounces-197866-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 1 Jun 2024 14:21:47 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Johan Hovold <johan+linaro@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org, Nuno Sa <nuno.sa@analog.com>
Subject: Re: [PATCH] iio: inkern: fix channel read regression
Message-ID: <20240601142147.3ac40207@jic23-huawei>
In-Reply-To: <20240530074416.13697-1-johan+linaro@kernel.org>
References: <20240530074416.13697-1-johan+linaro@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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)