Re: [PATCH v4 5/6] iio: core: use kstrtodec64() to parse fixed-point values

[Rodrigo Alencar]

On 26/10/26 16:54, Andy Shevchenko wrote:
> On Mon, Oct 26, 2026 at 08:54:09AM -0300, Rodrigo Alencar via B4 Relay wrote:
> 
> > Replace the open-coded __iio_str_to_fixpoint() parser with kstrtodec64(),
> > so sysfs writes of fixed-point values also accept E notation.
> > 
> > The fractional part only keeps the sign when the integer part is zero,
> > as before. The dB suffix for scale attributes is now handled by a separate
> > __iio_str_to_fixpoint_units() helper. It strips an optional trailing
> > newline and the "dB" or " dB" suffix before parsing.
> > 
> > When there is no fractional part (fract_mult == 0, or dec_scale == 0 for
> > 64-bit values), try base-autodetecting kstrtoll() first so hexadecimal
> > and octal input keeps its meaning ("010" is still 8). Fall back to
> > kstrtodec64() on -EINVAL to accept E notation.
> 
> > As a side effect, inputs that used to be rejected for integer-only
> > attributes are now accepted. An invalid octal number such as "08" is
> > parsed as decimal 8, and a fractional value such as "1.5" is rounded
> > to 2.
> 
> The main question here is: Do we want these side effects?
> And follow up question is: Where is it going to be documented?
> I consider this paragraph is very important for the users.

I see that those side effects turn the sysfs interface more robust to user input,
not an interface change, but indeed a behavior change on edge cases. To answer
the first question, yes, I would want those...

Userspace can do floating point math on physical values and scales and write the
result to raw integer attributes as is. Rounding to nearest acceptable values is
something that drivers often do themselves.

I am not really concerned about the invalid octal case input.

Also, no valid input being used so far is being invalidated with this.

As for the follow up question, I can add some details to iio_str_to_fixpoint() doc
header, which is the exported symbol here.

> 
> ...
> 
> > +	/*
> > +	 * Keep hex/octal support for integers. E notation and decimal values
> > +	 * result in -EINVAL from kstrtoll, due to unexpected termination on '.'
> 
> kstrtoll()
> 
> > +	 * or 'e'. It falls back to kstrtodec64 for decimal fixed-point numbers
> > +	 * with zero scale.
> > +	 */
> > +	ret = kstrtoll(str, 0, val);
> > +	if (ret == -EINVAL)
> 
> > +		ret = kstrtodec64(str, 0, val);
> 
> 		return kstrtodec64(str, 0, val);
> 
> ?
> 
> > +	return ret;
> 
> ...
> 
> >  int iio_str_to_fixpoint(const char *str, int fract_mult,
> >  			int *integer, int *fract)
> >  {
> > -	return __iio_str_to_fixpoint(str, fract_mult, integer, fract, false);
> > +	s64 dec64;
> > +	int ret;
> > +
> > +	if (fract_mult) {
> > +		/* scale = ffs(fract_mult) = ffs(10^n) = ffs(2^n * 5^n) = n + 1 */
> > +		ret = kstrtodec64(str, ffs(fract_mult), &dec64);
> > +		if (ret)
> > +			return ret;
> > +
> > +		dec64 = div_s64_rem(dec64, fract_mult * 10, fract);
> > +		/* only carry the sign in fract if the integer is zero */
> > +		if (dec64)
> > +			*fract = abs(*fract);
> 
> Should we actually switch to UB-free abs() implementation?

abs(fract) here is always less than 10^9 (largest power of 10 that fits 32-bits),
I'd say that this is not a concern. 

> 
> > +	} else {
> > +		ret = iio_str_to_s64(str, &dec64);
> > +		if (ret)
> > +			return ret;
> > +
> > +		*fract = 0;
> > +	}
> > +
> > +	if (dec64 > INT_MAX || dec64 < INT_MIN)
> > +		return -ERANGE;
> > +
> > +	*integer = dec64;
> > +
> > +	return 0;
> >  }

...

-- 
Kind regards,

Rodrigo Alencar