Re: [PATCH] staging: iio: ad5933: fix type mismatch regression

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 22 Jan 2024 14:49:17 +0100
David Schiller <david.schiller@jku.at> wrote:

> Commit 4c3577db3e4f ("Staging: iio: impedance-analyzer: Fix sparse
> warning") fixed a compiler warning, but introduced a bug that resulted
> in one of the two 16 bit IIO channels always being zero (when both are
> enabled).

> 
> This is because int is 32 bits wide on most architectures and in the
> case of a little-endian machine the two most significant bytes would
> occupy the buffer for the second channel as 'val' is being passed as a
> void pointer to 'iio_push_to_buffers()'.
> 
> Fix by defining 'val' as u16. Tested working on ARM64.
> 
> Fixes: 4c3577db3e4f ("Staging: iio: impedance-analyzer: Fix sparse warning")
> Signed-off-by: David Schiller <david.schiller@jku.at>

Applied to the fixes-togreg branch of iio.git and marked for stable.

Note that if you happen to fancy doing some cleanup of this driver, it should
just be returning them big endian (and describing the channels
as such) which would simplify the code and remove the need for this
temporary buffer.

> ---
> So apparently this has gone unnoticed for over eight years. It appears
> that I'm one of only a handful of Linux people with access to AD5933:
> https://lore.kernel.org/linux-iio/20230606113013.00000530@Huawei.com/

I would love us to have emulation in place for devices like this but
even if say roadtest lands upstream, it will be a while before we
have enough tests in place to pick up this sort of error in a little used
(as you've discovered) driver.

Thanks,

Jonathan



> 
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 793918e1c45f..b682d0f94b0b 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -608,7 +608,7 @@ static void ad5933_work(struct work_struct *work)
>  		struct ad5933_state, work.work);
>  	struct iio_dev *indio_dev = i2c_get_clientdata(st->client);
>  	__be16 buf[2];
> -	int val[2];
> +	u16 val[2];
>  	unsigned char status;
>  	int ret;
>