Re: [PATCH] iio: adc: stm32-adc: fix possible division by zero in processed channel

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 12:23:38PM +0200, Fabrice Gasnier wrote:
> 
> On 9/16/26 09:35, Andy Shevchenko wrote:
> > On Tue, Sep 15, 2026 at 06:10:40PM +0200, Fabrice Gasnier wrote:
> >> In case the conversion has failed or returned zero, processing *val
> >> can lead to a division by zero.
> >> Need to check for errors, or converted value is zero, before processing
> >> the data.
> >> In case converted value is zero, e.g. the Vrefint channel, this should
> >> be considered as invalid in all case.
> > 
> > Something went very wrong with the indentation of the above.
> 
> Hi Andy,
> 
> Euh, sorry but I don't understand what's wrong with indentation in the
> commit message ? Could you clarify ?

Each sentence seems to be a separate paragraph. Paragraphs are delimited
by a blank lines. But are they (sentences) really so independent?

Compare to:

  In case the conversion has failed or returned zero, processing *val
  can lead to a division by zero. Need to check for errors, or converted
  value is zero, before processing the data. In case converted value
  is zero, e.g. the Vrefint channel, this should be considered as invalid
  in all cases.

(also I fixed the typo, should be plural for 'cases' at the end).

...

> >> -		if (mask == IIO_CHAN_INFO_PROCESSED)
> >> -			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> >> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
> >> +			if (ret >= 0 && *val)
> >> +				*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> >> +			else
> >> +				ret = ret < 0 ? ret : -EINVAL;
> >> +		}
> > 
> > Reuse of the *val makes code harder to follow. Add a temporary variable for
> > this and do something like this (also note other simplifications)
> > 
> > 		tmp_choose_good_name = *val;
> 
> In case ret is an error; this lead to evaluate (e.g. read) *val from the
> (maybe uninitialized) stack. Probably not an issue? But would prefer to
> keep reading it only when ret >= 0, as done currently with the if
> condition above ? (e.g. evaluate ret, before *val)

> > 		...
> > 		if (mask == IIO_CHAN_INFO_PROCESSED) {
> > 			if (ret < 0)
> > 				return ret;

Then simply move the assignment here

			tmp_choose_good_name = *val;

> > 			if (tmp == 0)
> > 				return -EINVAL;
> 
> I've started with something similar before, but we can't return directly
> here. Must call iio_device_release_direct() first. This would add more
> lines.
> 
> So I chose to implement above ternary ret = ret < 0 ? ret : -EINVAL, to
> fall-through.
> 
> Please advise on the preferred way,

As per above.

> > 			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / tmp;
> > 		}
> > 
> >>  		iio_device_release_direct(indio_dev);
> >>  		return ret;

-- 
With Best Regards,
Andy Shevchenko