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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 07/02, sashiko-bot@kernel.org wrote:
> 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?
The available HDL only supports pushing 32-bit data elements to DMA buffers in
CPU endianness. For 16-bit precision parts, 32-bit elements (in CPU endianness)
containing the 16-bit data are pushed to DMA. For the offload use case, the IIO
buffer is configured for 32 storage bits in CPU endianness so data is correctly
aligned in user space (despite 2 out of the 4 bytes being always zeroed). I'll
add a comment with the above explanation.

> 
> [ ... ]
> > @@ -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?
No.

> 
> 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?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/units.h?h=v7.2-rc1#n50

> 
> 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?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/spi/offload/types.h?h=v7.2-rc1#n70

> 
> 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?
Makes no difference. The PWM configurations are recalculated and re-validated
every time the buffer is enabled. There's very little to no benefit in rolling
back PWM configurations.

> 
> > +		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?
That's what the offset_ns field is for.

> 
> > +	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?
Correct, offload support assumes a controller capable of supporting fine grained
number of bits_per_word.

> 
> > +	}
> >  
> >  	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