Re: [PATCH v6 4/6] iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect return value check for i2c_transfer() causes silent failures and bogus data returns.
--

commit 93acc0420c1accec74aba11d13ed25dc18d01455
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: dac: ad5686: read_raw/write_raw: use guard(mutex)()

This commit transitions the read_raw and write_raw functions in the AD5686 
driver to use guard(mutex) for locking. This simplifies the error paths by 
allowing early returns without explicit unlock calls.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 75dd7921c67b8..db175e77b0b7c 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
[ ... ]
> @@ -177,11 +178,11 @@ static int ad5686_read_raw(struct iio_dev *indio_dev,
>  	struct ad5686_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> +	guard(mutex)(&st->lock);
> +
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&st->lock);
>  		ret = ad5686_read(st, chan->address);

[Severity: High]
This isn't a bug introduced by this patch, but looking at ad5686_read()
which calls into ad5686_i2c_read() for I2C devices, is there an issue with
the return value check for i2c_transfer()?

drivers/iio/dac/ad5696-i2c.c:ad5686_i2c_read() {
    ...
    ret = i2c_transfer(i2c->adapter, msg, 2);
    if (ret < 0)
        return ret;

    return be16_to_cpu(st->data[0].d16);
}

If the first message (write) succeeds but the second (read) fails,
i2c_transfer() returns 1 (the number of successfully transferred messages).

Since 1 is not less than 0, the check passes and the function erroneously
returns the contents of st->data[0].d16, which holds the command bytes that
were written to the buffer just before the transfer.

Should this check verify that all 2 expected messages were transferred
instead, such as checking if (ret != 2)?

> -		mutex_unlock(&st->lock);
>  		if (ret < 0)
>  			return ret;
>  		*val = (ret >> chan->scan_type.shift) &

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-ad5686-new-features-v6-0-269594c7aae5@analog.com?part=4