Re: [PATCH 2/2] iio: light: Add Capella CM3217 ambient light sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iio: light: cm3217: data race on scale_avail in cm3217_read_avail()
--

commit 3a1806ec170fd01b8d751c110690623e96b608ae
Author: Patrick Fischer <admin@tsv-stahl-riesa.de>

iio: light: Add Capella CM3217 ambient light sensor driver

This commit adds an IIO ambient light sensor driver for the Capella
CM3217, communicating over two fixed I2C addresses (0x10 and 0x11) to
configure integration time and range and read the 16-bit light count.

> diff --git a/drivers/iio/light/cm3217.c b/drivers/iio/light/cm3217.c
> new file mode 100644
> index 0000000000000..9e0e9afee0925
> --- /dev/null
> +++ b/drivers/iio/light/cm3217.c
> @@ -0,0 +1,383 @@

[ ... ]

> +struct cm3217_data {
> +	struct i2c_client *client;
> +	struct i2c_client *client_lsb;
> +	/* protects the command bytes, the scale table and ready */
> +	struct mutex lock;
> +	u8 cmd1;
> +	u8 cmd2;
> +	ktime_t ready;
> +	int scale_avail[4][2];
> +};

[ ... ]

> +static void cm3217_update_scale_avail(struct cm3217_data *data)
> +	__must_hold(&data->lock)
> +{
> +	unsigned int fd_it = FIELD_GET(CM3217_CMD2_FD_IT, data->cmd2);
> +	unsigned int i;
> +	u32 rem;
> +	u64 nlux;
> +
> +	for (i = 0; i < ARRAY_SIZE(data->scale_avail); i++) {
> +		nlux = div_u64(CM3217_NLUX_MS << (3 - i), cm3217_fd_it_ms[fd_it]);
> +		data->scale_avail[i][0] = div_u64_rem(nlux, NANO, &rem);
> +		data->scale_avail[i][1] = rem;
> +	}
> +}

[ ... ]

> +static int cm3217_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	struct cm3217_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (const int *)data->scale_avail;
> +		*length = 2 * ARRAY_SIZE(data->scale_avail);
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		return IIO_AVAIL_LIST;

[Severity: Medium]
Can concurrent sysfs reads of in_illuminance_scale_available and writes to
in_illuminance_integration_time race on data->scale_avail?

In cm3217_read_avail(), a raw pointer to data->scale_avail is returned to the
caller without holding data->lock, and the caller in
drivers/iio/industrialio-core.c reads and formats the array elements after
cm3217_read_avail() returns:

iio_read_channel_info_avail()
  indio_dev->info->read_avail() -> cm3217_read_avail()
  iio_format_avail_list(buf, vals, type, length)

At the same time, cm3217_write_raw() or cm3217_resume() can call
cm3217_write_cmd() -> cm3217_update_scale_avail(), overwriting
data->scale_avail[i][0] and data->scale_avail[i][1] in place while
iio_format_avail_list() is iterating over vals. That can produce torn integer
and fractional scale pairs or a list mixing scales from two integration
times.

Since there are only 8 fixed integration times in cm3217_fd_it_ms[] and 4
ranges per integration time, could the scale tables be stored in a static
const table indexed by the integration time selector so cm3217_read_avail()
can return a pointer to an immutable row under data->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-upstream-iio-cm3217-v1-0-5e1724cae7b7@tsv-stahl-riesa.de?part=2