Re: [PATCH v2 09/10] iio: adc: ti-ads112c14: add settlingtime attribute

[Jonathan Cameron]

> Add per-channel sysfs attributes for settlingtime and
> settlingtime_available. These allow adjusting the total settling time
> for each channel. The value consists of a fixed t_latency time (based
> on the selected filter_type, oversampling_ratio and sampling_frequency)
> plus a user-configurable t_delay that determines the value to write to
> the DELAY field in the registers.
> 
> The allowable values are non-linear integer multiples, so the step size
> is just the smallest step size. Writing the attribute will match the
> closest matching value for the DELAY field with a time equal to or
> greater than the requested settling time (unless the requested time is
> larger than the maximum allowable settling time).
> 
> Signed-off-by: David Lechner (TI) <dlechner@xxxxxxxxxxxx>

A few things in here - mostly from sashiko rather than me.

>
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 85e926184b6e..cefbb7afe729 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c


...

> +static int ads112c14_get_settling_time_us(struct ads112c14_data *data,
> +					  struct ads112c14_channel_state *channel_state,
> +					  u8 delay, u32 *settling_time_us)
> +{
> +	int fmod_Hz, latency_tmod;
> +	u64 total_tmod;
> +
> +	fmod_Hz = ads112c14_get_fmod_Hz(data, channel_state);
> +	if (fmod_Hz <= 0)
> +		return -EINVAL;

[Severity: Medium]
Is it possible for negative inputs to bypass this validation check on 32-bit
architectures? MICRO is typically defined as 1000000UL. If integer is
negative (e.g., -1), the calculation integer * MICRO evaluates as an
unsigned 32-bit operation, producing a huge unsigned 32-bit value.

When this is assigned to the 64-bit signed integer settling_time_us, it is
zero-extended to a large positive value, rendering the < 0 check useless
and resulting in incorrect timing delay configurations.

-
I'm not sure on this one (and too lazy to check properly).
My guess is not a problem due to bounds on values from elsewhere but
please take a look.

> +
> +	latency_tmod = ads112c14_get_latency_tmod(channel_state);
> +	if (latency_tmod < 0)
> +		return latency_tmod;
> +
> +	total_tmod = latency_tmod + ads112c14_delay_to_tmod(delay);
> +	*settling_time_us = div64_u64(total_tmod * USEC_PER_SEC, fmod_Hz);
> +
> +	return 0;
> +}
> +
> +static int ads112c14_find_delay_for_settling_time_us(struct ads112c14_data *data,
> +						     struct ads112c14_channel_state *channel_state,
> +						     s64 settling_time_us, u8 *delay)
> +{
> +	u32 fixed_latency_us, delay_us;
> +	u64 delay_tmod_needed;
> +	int ret, fmod_Hz;
> +	u8 i;
> +
> +	ret = ads112c14_get_settling_time_us(data, channel_state, 0, &fixed_latency_us);
> +	if (ret)
> +		return ret;
> +
> +	if (settling_time_us <= fixed_latency_us) {
> +		*delay = 0;
> +		return 0;
> +	}
> +
> +	fmod_Hz = ads112c14_get_fmod_Hz(data, channel_state);
> +	if (fmod_Hz <= 0)
> +		return -EINVAL;
> +
> +	delay_us = settling_time_us - fixed_latency_us;

Sashiko:

[Severity: Medium]
Can this assignment truncate large settling times? delay_us is declared as
a 32-bit unsigned integer, while settling_time_us is a 64-bit signed
integer. If userspace writes a sufficiently large time, the subtraction will
wrap around the 32-bit boundary, resulting in a short, unintended delay
instead of saturating to the hardware's maximum delay limit.

-

The input comes from val * MICRO + val2 so seems like overflow is
plausible.

> +	delay_tmod_needed = DIV_ROUND_UP_ULL((u64)delay_us * fmod_Hz,
> +					     USEC_PER_SEC);
> +
> +	for (i = 1; i < ADS112C14_DELAY_MAX; i++) {
> +		if (ads112c14_delay_to_tmod(i) >= delay_tmod_needed)
> +			break;
> +	}
> +
> +	*delay = i;
> +
> +	return 0;
> +}
> +




> @@ -814,10 +1050,12 @@ static int ads112c14_wait_for_conversion_poll(struct ads112c14_data *data)
>  	if (ret)
>  		return ret;
>  
> +	/* Give it 1ms more than calculated settling time. */
>  	return regmap_read_poll_timeout(data->regmap,
>  					ADS112C14_REG_STATUS_MSB, reg_val,
>  					FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> -					1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> +					1 * USEC_PER_MSEC, settle_time_us +
> +							   1 * USEC_PER_MSEC);
Rewrap this:
					1 * USEC_PER_MSEC,
					settle_time_us + 1 * USEC_PER_MSEC);
Is nicer.
>  }
>  


> @@ -1961,6 +2239,52 @@ static void ads112c14_populate_odr_tables(struct ads112c14_data *data)
>  	available[0] = div_u64_rem(odr_uHz, MICRO, &available[1]);
>  }
>  
> +static void ads112c14_populate_settling_range_tables(struct ads112c14_data *data)
> +{
> +	s32 (*avail)[2];
> +	u32 i, j;
> +
> +	for (i = 0; i < ARRAY_SIZE(ads112c14_sinc_latency_tmod); i++) {
> +		for (j = 0; j < ARRAY_SIZE(ads112c14_fmod_div); j++) {
> +			u64 fmod_Hz = data->fclk_Hz / ads112c14_fmod_div[j];
> +			u64 start_tmod = ads112c14_sinc_latency_tmod[i][j];
> +			u64 step_tmod = ads112c14_delay_to_tmod(1);
> +			u64 stop_tmod = start_tmod + ads112c14_delay_to_tmod(ADS112C14_DELAY_MAX);
> +			s64 start_us, step_us, stop_us;
> +
> +			start_us = DIV_ROUND_CLOSEST_ULL(start_tmod * USEC_PER_SEC, fmod_Hz);

Your friendly neighbourhood Sashiko:
[Severity: High]
Does this code risk a division by zero crash during probe?

The external clock rate data->fclk_Hz is fetched without asserting that the
rate is nonzero or sufficiently large. If clk_get_rate() returns 0 (e.g. from
a faulty, missing, or zero-initialized clock in the device tree), fmod_Hz
becomes 0.

The DIV_ROUND_CLOSEST_ULL macro will then invoke do_div with a zero divisor,
triggering a hardware exception and crashing the kernel.
-

Probably should defend against this but just checking it isn't zero
when we first get it.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>