Re: [PATCH v5 7/8] iio: imu: adis16607: Add external clock sync support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: imu: adis16607: unlocked clk_freq_mHz read in adis16607_get_freq()
- [Medium] iio: imu: adis16607: stale sample_period_ns on error in adis16607_set_freq()
--

commit d3f4ee30496626b77ece21c139b6d2175717f5c5
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: adis16607: Add external clock sync support

This commit adds support for an optional external sync clock on the
adis16607 IMU, selecting between direct sync mode (1 kHz to 8 kHz) and
scaled sync mode (1 Hz to 400 Hz) based on the supplied clock frequency.
In scaled mode, it dynamically selects the sync scale and decimation rate
when setting the sampling frequency, and reports sync or PLL lock loss from
the FIFO drain handler.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> index bd7752f7a1ec8..58504d351de16 100644
> --- a/drivers/iio/imu/adis16607.c
> +++ b/drivers/iio/imu/adis16607.c

[ ... ]

> @@ -873,6 +947,21 @@ static int adis16607_set_freq(struct adis16607 *st, u32 freq)
>  		if (ret)
>  			return ret;
>  
> +		/*
> +		 * Writing USER_SYNC restarts the PLL lock, so only do it when
> +		 * the scale actually changes.
> +		 */
> +		if (scale != st->sync_scale) {
> +			ret = __adis_write_reg_16(&st->adis,
> +						  ADIS16607_REG_USER_SYNC,
> +						  ADIS16607_SYNC_MODE_MASK | scale);
> +			if (ret)
> +				return ret;
> +
> +			st->sync_scale = scale;
> +			st->clk_freq_mHz = clk_freq_mHz;
> +		}

[Severity: Medium]
Now that st->clk_freq_mHz is updated at runtime under st->adis.state_lock in
adis16607_set_freq() when operating in ADIS16607_SYNC_SCALED mode, can a
concurrent sysfs read of sampling_frequency race with this update?

In adis16607_read_raw() -> adis16607_get_freq():

	ret = adis_read_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, &dec);
	if (ret)
		return ret;

	*freq = DIV_ROUND_CLOSEST(st->clk_freq_mHz, dec + 1);

adis_read_reg_16() releases st->adis.state_lock before st->clk_freq_mHz is
read, and adis16607_read_raw() does not claim IIO direct mode for
IIO_CHAN_INFO_SAMP_FREQ.

Could adis16607_get_freq() hold st->adis.state_lock across both
__adis_read_reg_16() and the division using st->clk_freq_mHz so it cannot
pair a stale or intermediate zero ADIS16607_REG_DEC_RATE value with the new
st->clk_freq_mHz (or vice versa)?

> +
>  		if (dec) {
>  			ret = __adis_write_reg_16(&st->adis,
>  						  ADIS16607_REG_DEC_RATE, dec);

[Severity: Medium]
In adis16607_set_freq(), __adis_write_reg_16(&st->adis,
ADIS16607_REG_DEC_RATE, 0) clears the hardware decimation rate to 0 at the
start of the critical section, and if scale != st->sync_scale,
ADIS16607_REG_USER_SYNC and st->clk_freq_mHz are also updated before the new
decimation rate is written:

	adis_dev_auto_scoped_lock(&st->adis) {
		/* Reset the decimation accumulator before the new value */
		ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, 0);
		if (ret)
			return ret;

		if (scale != st->sync_scale) {
			ret = __adis_write_reg_16(&st->adis,
						  ADIS16607_REG_USER_SYNC,
						  ADIS16607_SYNC_MODE_MASK | scale);
			if (ret)
				return ret;

			st->sync_scale = scale;
			st->clk_freq_mHz = clk_freq_mHz;
		}

		if (dec) {
			ret = __adis_write_reg_16(&st->adis,
						  ADIS16607_REG_DEC_RATE, dec);
			if (ret)
				return ret;
		}

		st->sample_period_ns = period_ns;
	}

If the write to ADIS16607_REG_USER_SYNC or the final write to
ADIS16607_REG_DEC_RATE fails, the hardware is left running with DEC_RATE = 0
(and possibly the new sync scale and st->clk_freq_mHz), while
st->sample_period_ns retains its old value from before the call.

Would this cause adis16607_fifo_drain() to compute skewed sample timestamps
on subsequent buffered reads?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-adis16607-v5-0-4f98b0d271ac@analog.com?part=7