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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Access to non-existent `offset_ns` field in `struct spi_offload_trigger_periodic` causes a build failure.
- [Critical] The macro `HZ_PER_GHZ` is used but never defined anywhere in the kernel, causing a build failure.
- [High] Mismatch between SPI offload transfer length and IIO channel storage bits for 16-bit devices causes buffer data corruption.
- [High] Unassigned `length` pointer in `ltc2378_read_avail` API callback leaves caller variable uninitialized.
- [High] Unpredictable phase shift between two independent PWM channels risks hardware data corruption.
- [High] Forcing unsupported `bits_per_word` breaks single-shot reads on standard SPI controllers.
- [Medium] `st->offload_trigger_config` is updated in-place and not rolled back on validation failure, corrupting driver state.
--

commit 7c29c089819a4620f86fae01e267747bb7a266ff
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
> index b5cf2e974daca..b25602d91bbe1 100644
> --- a/drivers/iio/adc/ltc2378.c
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> @@ -46,10 +58,19 @@
>  #define LTC2378_UNIPOLAR_DIFF_CHANNEL(_real_bits)				\
>  	LTC2378_DIFF_CHANNEL(0, _real_bits, (((_real_bits) > 16) ? 32 : 16), 0)
>  
> +#define LTC2378_OFFLOAD_BIPOLAR_DIFF_CHANNEL(_real_bits)			\
> +	LTC2378_DIFF_CHANNEL(1, (_real_bits), 32, 1)
> +
> +#define LTC2378_OFFLOAD_UNIPOLAR_DIFF_CHANNEL(_real_bits)			\
> +	LTC2378_DIFF_CHANNEL(0, (_real_bits), 32, 1)
> +

[Severity: High]
Does hardcoding the storagebits to 32 here mismatch with the SPI transfer
length for 16-bit devices?

When ltc2378_prepare_offload_message() configures the transfer, it sets the
length to spi_bpw_to_bytes(16), which is 2 bytes. Since the IIO subsystem
expects 4 bytes per sample based on these channel definitions, could this
cause buffer reads to be misaligned and return garbage data to userspace?

[ ... ]
> @@ -254,7 +344,136 @@ 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;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltc2378_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length, long mask)
> +{
> +	struct ltc2378_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = st->sample_freq_range;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;

[Severity: High]
Should this callback also initialize the length pointer?

Since this returns IIO_AVAIL_RANGE successfully but doesn't assign *length,
callers like iio_read_channel_info_avail() in the core might use an
uninitialized stack variable for the array size. Could this lead to
out-of-bounds array reads?

> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +static int ltc2378_update_conversion_rate(struct ltc2378_state *st, int freq_Hz)
> +{
[ ... ]
> +	/*
> +	 * 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);

[Severity: Critical]
Is HZ_PER_GHZ defined anywhere in the kernel?

Looking through the tree and include/linux/units.h, this macro doesn't appear
to exist. Won't this cause an unconditional build failure?

> +	min_read_offset = LTC2378_TBUSYLH_NS + st->info->tconv_ns + LTC2378_TDSDOBUSYL_NS;
> +	offload_offset_ns = min_read_offset;
> +	count = 100;
> +	do {
> +		config->periodic.offset_ns = offload_offset_ns;

[Severity: Critical]
Does struct spi_offload_trigger_periodic actually have an offset_ns field?

According to include/linux/spi/offload/types.h, struct
spi_offload_trigger_periodic only contains the frequency_hz field. This seems
like it will cause a build error.

> +		ret = spi_offload_trigger_validate(st->offload_trigger, config);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If validation fails here, does the in-place modification of the shared config
struct get rolled back?

Since config points directly to st->offload_trigger_config, returning early
leaves the device state with rejected values. If the IIO buffer is later
enabled, could the driver use these invalid config values to start the
offload trigger?

> +		offload_offset_ns += 10;
> +	} while (count-- && config->periodic.offset_ns < min_read_offset);
[ ... ]
> +static int ltc2378_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ltc2378_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pwm_set_waveform_might_sleep(st->cnv_trigger, &st->cnv_wf, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &st->offload_trigger_config);

[Severity: High]
Does starting these two clock sources sequentially result in an unpredictable
phase shift?

Since st->cnv_trigger and st->offload_trigger are independent and enabled one
after the other in software, CPU execution delays might cause an uncontrolled
phase alignment. The LTC2378 datasheet forbids SCLK toggling during the
conversion phase. Could this random phase shift cause SPI transactions to
overlap with the conversion window and corrupt the sampled data?

> +	if (ret)
> +		goto out_pwm_disable;
> +
> +	return 0;
[ ... ]
> @@ -330,8 +653,53 @@ static int ltc2378_probe(struct spi_device *spi)
[ ... ]
> +		/*
> +		 * Set single-read transfer bits_per_word so the SPI subsystem
> +		 * rearanges data to CPU endianness, enabling us to reuse
> +		 * offload_chan specifications for single-shot reads.
> +		 */
> +		st->xfer.bits_per_word = st->info->offload_chan.scan_type.realbits;

[Severity: High]
Is it safe to force this bits_per_word on the shared transfer struct used for
regular single-shot reads?

Most generic SPI host controllers only natively support standard word widths
(like 8, 16, 32). If bits_per_word is set to an arbitrary resolution like 18,
won't the SPI core's __spi_validate reject the transfer with -EINVAL and break
sysfs single-shot reads?

> +	}
>  
>  	st->xfer.rx_buf = &st->scan.data;
>  	st->xfer.len = spi_bpw_to_bytes(indio_dev->channels[0].scan_type.realbits);

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