Re: [PATCH v2 2/2] iio: magnetometer: add support for Melexis MLX90393

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Data race on statically allocated `scale_avail` array.
- [High] DMA safety violation due to stack-allocated transfer buffers.
- [High] Writing to the temperature scale attribute corrupts the magnetometer gain configuration.
- [Medium] IIO scale values violate ABI standard (Tesla and Celsius instead of Gauss and milli-Celsius).
- [Medium] Missing `id_table` assignment in the I2C driver structure.
- [Medium] Temperature channel exposes raw register exponent values instead of true oversampling ratios.
- [Medium] Lockless state reads in `mlx90393_read_raw` lead to data races.
--

commit bdec68e57f45f6e35e5c0c76d37b8ee5efa9691b
Author: Nikhil Gautam <nikhilgtr@gmail.com>

iio: magnetometer: add support for Melexis MLX90393

Add Industrial I/O subsystem support for the Melexis
MLX90393 3-axis magnetometer and temperature sensor.

> diff --git a/drivers/iio/magnetometer/mlx90393_core.c b/drivers/iio/magnetometer/mlx90393_core.c
> new file mode 100644
> index 0000000000000..0ad4a30c0be96
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_core.c

[ ... ]

> +static int mlx90393_write_reg(struct mlx90393_data *data, u8 reg, u16 val)
> +{
> +	u8 tx[4];
> +	u8 status;
> +	int ret;
> +
> +	tx[0] = MLX90393_CMD_WR;
> +	put_unaligned_be16(val, &tx[1]);
> +	/* Register address is encoded in bits [7:2] */
> +	tx[3] = reg << 2;
> +
> +	ret = mlx90393_xfer(data, tx, sizeof(tx), &status, 1);

[Severity: High]
Is it safe to allocate the transfer buffers on the stack here and in
mlx90393_read_measurement()? 

If the I2C host adapter uses DMA and CONFIG_VMAP_STACK is enabled, mapping
stack memory for DMA operations will fail. Should these buffers be
dynamically allocated to ensure DMA safety?

[ ... ]

> +static int mlx90393_get_scale(struct mlx90393_data *data,
> +			      const struct iio_chan_spec *chan,
> +			      int *val, int *val2)
> +{
> +	enum mlx90393_axis_type axis;
> +	u8 res;
> +
> +	if (chan->channel2 == IIO_MOD_Z) {
> +		axis = MLX90393_AXIS_TYPE_Z;
> +		res = data->res_z;
> +	} else {
> +		axis = MLX90393_AXIS_TYPE_XY;
> +		res = data->res_xy;
> +	}
> +
> +	/*
> +	 * Convert:
> +	 * µT × 1000 → nT
> +	 */
> +	*val = 0;
> +	*val2 = mlx90393_scale_table[axis][data->gain_sel][res];
> +
> +	return IIO_VAL_INT_PLUS_NANO;
> +}

[Severity: Medium]
Does this return the scale in Gauss as required by the IIO ABI for
in_magn_scale? Using IIO_VAL_INT_PLUS_NANO with the current scale table yields
Tesla, which is off by a factor of 10,000 compared to Gauss.

[ ... ]

> +static int mlx90393_write_raw(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      int val, int val2,
> +			      long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE: {
> +		guard(mutex)(&data->lock);
> +		ret = mlx90393_set_scale(data, chan, val, val2);
> +		return ret;
> +	}

[Severity: High]
Can writing to the temperature scale attribute corrupt the magnetometer gain
configuration?

The IIO_TEMP channel exposes a writable scale attribute, but this case does
not check chan->type before calling mlx90393_set_scale(). Because the
temperature channel's channel2 field is 0, it evaluates as an XY magnetometer
channel and will reconfigure the global gain_sel register.

[ ... ]

> +static int mlx90393_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&data->lock);
> +		ret = mlx90393_read_measurement(data, chan->address, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_MAGN:
> +			return mlx90393_get_scale(data, chan, val, val2);

[Severity: Medium]
Is a lock needed here?

Configuration variables like gain_sel, osr, and osr2 are updated while
holding data->lock in mlx90393_write_raw(), but mlx90393_read_raw() reads
these properties without acquiring the lock.

Can concurrent updates result in stale or inconsistently-read configurations?

> +
> +		case IIO_TEMP:
> +			/* Datasheet: 22124 millidegC/LSB */
> +			*val = 0;
> +			*val2 = 22124;
> +			return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
Does this return the temperature scale in milli-Celsius as required by the IIO
ABI? Returning 22124 with IIO_VAL_INT_PLUS_MICRO yields Celsius, which is off
by a factor of 1,000.

[ ... ]

> +static int mlx90393_read_avail(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       const int **vals,
> +			       int *type,
> +			       int *length,
> +			       long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +	static int scale_avail[MLX90393_GAIN_MAX][MLX90393_AXIS_MAX];

[Severity: High]
Is it safe to use a statically allocated local array for scale_avail?

The lock is released before the IIO core iterates over the returned pointer.
Can concurrent sysfs reads across different channels or multiple device
instances clobber the shared array and return corrupted lists to userspace?

> +	enum mlx90393_axis_type axis;
> +	u8 res;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE: {
> +		guard(mutex)(&data->lock);
> +		axis = chan->channel2 == IIO_MOD_Z;
> +		res = axis ? data->res_z : data->res_xy;
> +
> +		for (unsigned int i = 0; i < MLX90393_GAIN_MAX; i++) {
> +			scale_avail[i][0] = 0;
> +			scale_avail[i][1] = mlx90393_scale_table[axis][i][res];
> +		}
> +
> +		*vals = &scale_avail[0][0];
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*length = MLX90393_GAIN_MAX * MLX90393_AXIS_MAX;
> +		return IIO_AVAIL_LIST;
> +	}
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		if (chan->type == IIO_TEMP) {
> +			*vals = mlx90393_osr2_avail;
> +			*type = IIO_VAL_INT;
> +			*length = MLX90393_OSR2_MAX;

[Severity: Medium]
Does this expose raw register exponent values instead of true oversampling
ratios for the temperature channel?

Returning the register exponents (0, 1, 2, 3) directly violates the IIO ABI
which dictates reflecting the actual number of hardware samples applied.

[ ... ]

> diff --git a/drivers/iio/magnetometer/mlx90393_i2c.c b/drivers/iio/magnetometer/mlx90393_i2c.c
> new file mode 100644
> index 0000000000000..52233b6295c2b
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_i2c.c

[ ... ]

> +static struct i2c_driver mlx90393_i2c_driver = {
> +	.driver = {
> +		.name = "mlx90393",
> +		.of_match_table = mlx90393_of_match,
> +	},
> +	.probe = mlx90393_i2c_probe,
> +};

[Severity: Medium]
Is the id_table assignment missing here?

The driver defines the mlx90393_id table and exports it, but it is not
assigned to the id_table field of this structure. Could this prevent the
driver from binding on systems relying on legacy board-file setups or sysfs
new_device instantiation?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618160141.11409-1-nikhilgtr@gmail.com?part=2