linux-kernel - Re: [PATCH v3 2/5] iio: add processed write API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQzHtqFEIA5E0ikO@smile.fi.intel.com>
Date: Thu, 6 Nov 2025 18:07:18 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Hans de Goede <hansg@...nel.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH v3 2/5] iio: add processed write API

On Thu, Nov 06, 2025 at 03:11:47PM +0100, Romain Gantois wrote:
> Add a function to allow IIO consumers to write a processed value to a
> channel.

...

> +int iio_divide_by_value(int *result, s64 numerator,
> +			unsigned int type, int val, int val2)
> +{
> +	s64 tmp_num, tmp_den;
> +
> +	switch (type) {
> +	case IIO_VAL_INT:
> +		tmp_num = numerator;
> +		tmp_den = val;
> +		break;
> +	case IIO_VAL_INT_PLUS_MICRO:
> +	case IIO_VAL_INT_PLUS_NANO:
> +		switch (type) {
> +		case IIO_VAL_INT_PLUS_MICRO:
> +			tmp_num = MICRO;
> +			tmp_den = MICRO;
> +			break;
> +
> +		case IIO_VAL_INT_PLUS_NANO:
> +			tmp_num = NANO;
> +			tmp_den = NANO;
> +			break;
> +		}

> +		tmp_num *= numerator;
> +		tmp_den = (s64)abs(val) * tmp_den + (s64)abs(val2);

Here is a subtle bug. The problematic piece is abs(). See
https://lore.kernel.org/r/20251106152051.2361551-1-andriy.shevchenko@linux.intel.com
for the answer.

> +		if (val < 0 || val2 < 0)
> +			tmp_num *= -1;

Drop that duplication of the switches above and split the calculations. Note,
with the split done, the confusing assignments of tmp_den will gone as well.

> +		break;
> +	case IIO_VAL_FRACTIONAL:
> +		tmp_num = (s64)numerator * (s64)val2;
> +		tmp_den = val;
> +		break;
> +	case IIO_VAL_FRACTIONAL_LOG2:
> +		tmp_num = (s64)numerator << val2;
> +		tmp_den = val;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!tmp_den)
> +		return -ERANGE;
> +
> +	*result = div64_s64(tmp_num, tmp_den);
> +
> +	return IIO_VAL_INT;
> +}

...

> +	offset_type = iio_channel_read(chan, &offset_val, &offset_val2,

> +	if (offset_type >= 0) {

Why?

> +		switch (offset_type) {
> +		case IIO_VAL_INT:
> +		case IIO_VAL_INT_PLUS_MICRO:
> +			half_step = MICRO / 2;
> +			break;
> +		case IIO_VAL_INT_PLUS_NANO:
> +			half_step = NANO / 2;
> +			break;
> +		case IIO_VAL_FRACTIONAL:
> +			offset_val = DIV_ROUND_CLOSEST(offset_val, offset_val2);
> +			break;
> +		case IIO_VAL_FRACTIONAL_LOG2:
> +			offset_val >>= offset_val2;
> +			break;

> +		default:

You probably wanted to check it here.

> +			return -EINVAL;



> +		}
> +
> +		/* Round fractional part to closest to reduce rounding bias. */
> +		if (half_step) {
> +			if (offset_val2 >= half_step)
> +				*raw -= 1;
> +			else if (offset_val2 <= -half_step)
> +				*raw += 1;
> +		}
> +
> +		*raw -= offset_val;
> +	}

...

> +EXPORT_SYMBOL_GPL(iio_write_channel_processed_scale);

Can we start using namespaced exports?

...

> +/**
> + * iio_divide_by_value() - Divide by an IIO value
> + * @result:	Destination pointer for the division result
> + * @numerator:	Numerator.
> + * @type:	One of the IIO_VAL_* constants. This decides how the @val
> + *		and @val2 parameters are interpreted.
> + * @val:	Denominator.
> + * @val2:	Denominator. @val2 use depends on type.
> + *
> + * Divide an s64 number by an IIO value, storing the result as

s64 number --> @numerator

> + * IIO_VAL_INT. This is typically used for scaling.
> + *
> + * Returns:
> + * IIO_VAL_INT on success or a negative error-number on failure.

Use % for the constants. It will be rendered differently (font) when
applicable. Same for other constants in all of the kernel-doc you add.

> + */

...

> +/**
> + * iio_write_channel_processed_scale() - scale and write processed value to a given channel
> + * @chan:		The channel being queried.
> + * @val:		Value to write.
> + * @scale:		Processed value is divided by this scale factor during the conversion.
> + *
> + * This function writes a processed value to a channel. A processed value means
> + * that this value will have the correct unit and not some device internal
> + * representation. If the device does not support writing a processed value, the
> + * function will query the channel's scale and offset and write an appropriately
> + * transformed raw value.

> + * Context: May sleep.

The above kernel-doc doesn't have this!

> + * Return: an error code or 0.

Be consistent with the existing code, and even in your own change.

("Return" section name, "Context" section presence, etc.)

Use Perl (original) kernel-doc for now, the Python has a significant regression
(the fix is pending to go to Linus' branch).

-- 
With Best Regards,
Andy Shevchenko