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

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

On 07/09, sashiko-bot@kernel.org wrote:
> > +	/*
> > +	 * 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 = 100;
> > +	do {
> > +		config->periodic.offset_ns = offload_offset_ns;
> > +		ret = spi_offload_trigger_validate(st->offload_trigger, config);
> > +		if (ret)
> > +			return ret;
> 
> [Severity: High]
> Does modifying config directly leave the device state out of sync if
> validation fails?
> 
> Here config aliases to &st->offload_trigger_config, and its frequency_hz and
> offset_ns fields are mutated. If spi_offload_trigger_validate() returns an
> error, the function returns without restoring the old configuration.
> 
> However, st->cnv_wf is only updated at the end of the function. If the
> buffer is later enabled, could this result in the PWM running with the old
> frequency while the SPI offload engine tries to use the new, rejected
> frequency?
> 
Ok, v7 will come with a local variable to hold offload trigger configuration
until all checks pass and both st->offload_trigger_config and st->cnv_wf can
be updated.

> > +		offload_offset_ns += 10;
> > +	} while (count-- && config->periodic.offset_ns < min_read_offset);
> > +
> > +	/* Check the minimum CNV to SCLK delay is met */
> > +	if (config->periodic.offset_ns < min_read_offset)
> > +		return -EDOM;
> > +
> > +	/* Check the PWM periods remain the same */
> > +	offload_period_ns = div64_u64(HZ_PER_GHZ, config->periodic.frequency_hz);
> > +	if (cnv_wf.period_length_ns != offload_period_ns)
> > +		return -EDOM;
> 
> [Severity: Medium]
> Could this strict equality check unintentionally reject valid sample
> frequencies due to integer math rounding?
> 
> If a requested frequency produces a period (cnv_wf.period_length_ns) that
> does not perfectly divide 1,000,000,000, DIV_ROUND_UP() might round the
> frequency up. Converting that frequency back to a period via div64_u64()
> would yield a slightly smaller period.
> 
> For example, 133,333 Hz yields a 7500 ns period. 10^9 / 7500 = 133333.33,
> which rounds up to 133334. Then 10^9 / 133334 = 7499 ns. Since 7500 != 7499,
> this check would fail and return -EDOM.

The setup for SPI offload support uses two PWM sources. One PWM drives CNV to
periodically start new conversions, the other signals to the SPI controller when
it can start fetching data. CNV and trigger PWMs must be offset by some ns. In
addition to that, the PWM sources must be synchronized such that the PWM
connected to the controller doesn't fire too early nor too late. And, finally,
the user might want to run the data capture for an arbitrarily long period of
time, requiring the PWM sources to remain synchronized to be able to
consistently read valid samples.

Those requirements pretty much imply the PWM sources to have the same period.
Though, our peripheral device driver only knows about the SPI controller
PWM through the offload_trigger abstraction. After spi_offload_trigger_validate(),
the client driver has no way to know whether the new frequency value is a result
of integer rounding or PWM hardware not supporting the requested period/frequency.
Even though the divergence in the provided example seems to be caused by integer
rounding, it might not always be the case.

> 
> > +
> > +	st->cnv_wf = cnv_wf;
> > +	st->cnv_Hz = DIV_ROUND_CLOSEST_ULL(HZ_PER_GHZ, cnv_wf.period_length_ns);
> > +
> > +	return 0;
> > +}
The other comments seem to make sense.