Re: [PATCH v4 3/4] iio: adc: ltc2378: Enable high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing verification of SPI offload trigger offset after validation loop in `ltc2378_update_conversion_rate()` allows silent data corruption.
- [Medium] In-out parameters accumulate drift across iterative rounding loops in `ltc2378_update_conversion_rate()`.
- [Medium] Torn state on error path in `ltc2378_update_conversion_rate()` due to partially updated `st->offload_trigger_config`.
- [Low] Data race on `st->cnv_Hz` between sysfs readers and writers.
--

commit 57c4b9b3af36aafb3888623014ea397ad6cf0cd6
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ltc2378: Enable high-speed data capture
    
Make use of SPI transfer offloading to speed up data capture, enabling data
acquisition at faster sample rates (up to 2 MSPS).

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> --- a/drivers/iio/adc/ltc2378.c
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> @@ -254,7 +345,126 @@ static int ltc2378_read_raw(struct iio_dev *indio_dev,
>  			*val2 = chan->scan_type.realbits;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->cnv_Hz;

[Severity: Low]
When reading the frequency via ltc2378_read_raw(), st->cnv_Hz is read without
acquiring the direct mode lock. Since it can be concurrently modified by
ltc2378_update_conversion_rate() during a write, does this cause a data race?
Should this be protected or accessed via READ_ONCE()?

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +static int ltc2378_update_conversion_rate(struct ltc2378_state *st, int freq_Hz)
> +{
[ ... ]
> +	/* Configure CNV PWM waveform */
> +	cnv_wf.period_length_ns = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq_Hz);
>  
> +	/*
> +	 * Ensure CNV high time meets minimum requirement (20ns). The PWM
> +	 * hardware may round the duty cycle, so iterate until we get at least
> +	 * the minimum required high time.
> +	 */
> +	do {
> +		cnv_wf.duty_length_ns = target;
> +		ret = pwm_round_waveform_might_sleep(st->cnv_trigger, &cnv_wf);

[Severity: Medium]
The pwm_round_waveform_might_sleep() function can modify the passed cnv_wf
structure in place. Since the base period (cnv_wf.period_length_ns) is set
before the loop and not reset inside, does this cause the period to drift
across the iterations if the hardware constraints cause it to be repeatedly
rounded?

> +		if (ret)
> +			return ret;
> +		target += 10;  /* Increment by PWM duty cycle period */
> +	} while (count++ < 100 && cnv_wf.duty_length_ns < LTC2378_TCNV_HIGH_NS);
> +
> +	/* Double check the minimum CNV high time is met */
> +	if (cnv_wf.duty_length_ns < LTC2378_TCNV_HIGH_NS)
> +		return -EIO;
> +
> +	/*
> +	 * Configure SPI offload PWM trigger.
> +	 * The trigger should fire after tBUSYLH + tCONV + tDSDOBUSYL.
> +	 * Minimum time needed: TBUSYLH (13ns) + TCONV (part-specific) + TDSDOBUSYL (5ns)
> +	 *
> +	 * Use the same period as CNV PWM to avoid timing issues.
> +	 * Convert back from period to frequency for the SPI offload API.
> +	 */
> +	offload_period_ns = cnv_wf.period_length_ns;
> +	config->periodic.frequency_hz = DIV_ROUND_UP(HZ_PER_GHZ, offload_period_ns);
> +	min_read_offset = LTC2378_TBUSYLH_NS + st->info->tconv_ns + LTC2378_TDSDOBUSYL_NS;
> +	offload_offset_ns = min_read_offset;
> +	count = 0;
> +	do {
> +		config->periodic.offset_ns = offload_offset_ns;
> +		ret = spi_offload_trigger_validate(st->offload_trigger, config);

[Severity: Medium]
The config pointer aliases st->offload_trigger_config. If this validation
fails, the function returns the error directly below without rolling back
the st->offload_trigger_config state. Could this leave the device in an
inconsistent state where the old st->cnv_wf configuration is paired with
a newly modified but rejected SPI trigger configuration upon the next
postenable?

Additionally, similar to the PWM loop above, spi_offload_trigger_validate()
modifies config in place. Does config->periodic.frequency_hz drift across
iterations since it is not reset inside the loop?

> +		if (ret)
> +			return ret;
> +		offload_offset_ns += 10;
> +	} while (count++ < 100 && config->periodic.offset_ns < min_read_offset);

[Severity: High]
If the loop exhausts its 100 retries without meeting the minimum offset
requirement, won't it silently proceed here without returning an error?

Unlike the preceding PWM loop which explicitly checks the bounds afterward,
could this missing check cause silent data corruption if the SPI controller
ends up reading while the ADC is still converting?

> +
> +	st->cnv_wf = cnv_wf;
> +	st->cnv_Hz = DIV_ROUND_CLOSEST_ULL(HZ_PER_GHZ, cnv_wf.period_length_ns);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782397418.git.marcelo.schmitt@analog.com?part=3