Re: [PATCH 08/10] iio: frequency: ad9910: support iio backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: ad9910: missing error rollback causing state desync and resource leaks
- [Medium] iio: frequency: ad9910: backend channel leak on preenable error
- [Medium] iio: frequency: ad9910: missing backend cleanup on postdisable error
--

commit 7c7244ed8ac1bd865ad6a2d0af50cec74add275e
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Subject: iio: frequency: ad9910: support iio backend

This commit adds support for an optional IIO backend to the AD9910 parallel
data port. It handles backend data rate control and synchronizes hardware
states with buffer callbacks.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 3d517879dee39..39d65dafd5a18 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> @@ -568,6 +580,11 @@ static int ad9910_set_sysclk_freq(struct ad9910_state *st, u32 freq_hz,
>  
>  	st->data.sysclk_freq_hz = sysclk_freq_hz;
>  
> +	if (st->back)
> +		return iio_backend_set_sampling_freq(st->back,
> +						     AD9910_CHAN_IDX_PHY,
> +						     sysclk_freq_hz);
> +
>  	return 0;
>  }

[Severity: High]
If iio_backend_set_sampling_freq() fails, are the software cache and SPI
hardware states left desynchronized from the IIO backend?

It appears that st->data.sysclk_freq_hz and potentially the SPI hardware
(via earlier updates in this function) are modified before the backend call.
If the backend rejects the configuration, this returns an error without
rolling back those earlier updates, leaving the driver state inconsistent.

[ ... ]
> @@ -2278,6 +2400,72 @@ static inline void ad9910_debugfs_init(struct ad9910_state *st,
[ ... ]
> +static int ad9910_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->scan_mask = *indio_dev->active_scan_mask;
> +	ret = iio_backend_chan_enable(st->back,
> +				      ad9910_buffer_chan_idx(indio_dev));
> +	if (ret)
> +		return ret;
> +
> +	return ad9910_reg32_update(st, AD9910_REG_CFR2,
> +				   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +				   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +				   true);
> +}

[Severity: High]
Does returning early here leak the backend channel if ad9910_reg32_update()
fails?

The function successfully enables the backend channel via
iio_backend_chan_enable(), but if the subsequent SPI write fails, it returns
the error code directly. Because the IIO core handles preenable() failures
by aborting the buffer enablement process without invoking postdisable(),
the backend channel seems to remain permanently enabled.

Should there be an error path here that calls iio_backend_chan_disable()
when the register update fails?

> +
> +static int ad9910_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->scan_mask = 0;
> +	ret = ad9910_reg32_update(st, AD9910_REG_CFR2,
> +				  AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +				  0, true);
> +	if (ret)
> +		return ret;
> +
> +	return iio_backend_chan_disable(st->back,
> +					ad9910_buffer_chan_idx(indio_dev));
> +}

[Severity: Medium]
Could this early return skip the required backend cleanup?

If ad9910_reg32_update() fails during this teardown path, it immediately
returns the error code, completely skipping iio_backend_chan_disable().
This could leak the backend channel and leave the backend state desynchronized
from the IIO buffer state.

Would it be better to always attempt iio_backend_chan_disable() on a
best-effort basis, even if the hardware write fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-ad9910-backend-support-v1-0-3c21648c8e26@analog.com?part=8