Re: [PATCH] iio: hid-sensor: als: scale each channel individually

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch fails to expose the individual scale for the CHROMATICITY_Y channel because it still uses `info_mask_shared_by_type`.

Pre-existing issues:
- [High] The driver's IIO buffer layout uses a fixed structure, but `available_scan_masks` is dynamically populated, causing misaligned buffer reads and data corruption.
--

--- Patch [1]: [PATCH] iio: hid-sensor: als: scale each channel individually ---
commit 6d49cfd4cf380845a584116e06dcf23fc8c25ad6
Author: Daniel Schaefer <dhs@frame.work>

iio: hid-sensor: als: scale each channel individually

This commit updates the hid-sensor-als driver to support individual scaling
factors for each channel, instead of using a single global scale. It stores
the scale attributes in arrays indexed by channel.

Link: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy/-/merge_requests/413

> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 384572844162c..232cb58e0a9ff 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -33,9 +33,9 @@ struct als_state {
>  		u32 illum[CHANNEL_SCAN_INDEX_MAX];
>  		aligned_s64 timestamp;
>  	} scan;

[Severity: High]
This is a pre-existing issue, but does this fixed struct buffer layout
cause data corruption?

The driver captures samples in als_capture_sample() into a static sparse
array (als_state->scan.illum) based on fixed channel indices. However,
indio_dev->available_scan_masks is dynamically constructed based on the
attributes present on the sensor.

The IIO core calculates scan_bytes and channel offsets assuming the buffer is
tightly packed.

When iio_push_to_buffers_with_ts() is called in als_proc_event(), it writes
the timestamp at the end of the tightly packed length, which overwrites valid
channel data in the sparse array.

The IIO demuxer then reads from incorrect offsets, scrambling the data sent to
userspace.

Should the driver pack the samples sequentially instead of using fixed index
offsets?

> -	int scale_pre_decml;
> -	int scale_post_decml;
> -	int scale_precision;
> +	int scale_pre_decml[CHANNEL_SCAN_INDEX_MAX];
> +	int scale_post_decml[CHANNEL_SCAN_INDEX_MAX];
> +	int scale_precision[CHANNEL_SCAN_INDEX_MAX];
>  	int value_offset;
>  	int num_channels;
>  	s64 timestamp;

[ ... ]

> @@ -186,9 +186,11 @@ static int als_read_raw(struct iio_dev *indio_dev,
>  		ret_type = IIO_VAL_INT;
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = als_state->scale_pre_decml;
> -		*val2 = als_state->scale_post_decml;
> -		ret_type = als_state->scale_precision;
> +		if (chan->scan_index >= CHANNEL_SCAN_INDEX_MAX)
> +			return -EINVAL;
> +		*val = als_state->scale_pre_decml[chan->scan_index];
> +		*val2 = als_state->scale_post_decml[chan->scan_index];
> +		ret_type = als_state->scale_precision[chan->scan_index];
>  		break;

[Severity: Medium]
Since the patch introduces per-channel scaling factors, can userspace actually
read the scale for the CHROMATICITY_Y channel?

The als_channels array defines both CHANNEL_SCAN_INDEX_CHROMATICITY_X and Y
with .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE).

Because of this, the IIO core will create a single in_chromaticity_scale sysfs
attribute for both.

When read, IIO queries the first registered channel of that type (X), leaving
the Y scale factor completely inaccessible.

Does the info_mask_shared_by_type for these channels need to be changed to
info_mask_separate so userspace can read the newly added scale for
CHROMATICITY_Y?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-chromaticity-v1-1-5ef5746ace2a@frame.work?part=1