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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Data race and state corruption due to function-static `scale_avail` array.
- [High] Stack-allocated buffers are passed to `i2c_transfer`, violating DMA safety rules.
- [Medium] Writing to temperature scale incorrectly modifies magnetometer hardware gain.
- [Medium] Temperature oversampling ratio exposes raw register values instead of actual multipliers.
- [Medium] IIO scale factors are incorrectly calculated, violating the expected IIO ABI units.
- [Low] Use of floating-point literal in delay calculation causes build failures.
--

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

iio: magnetometer: add support for Melexis MLX90393

This patch adds Industrial I/O subsystem support for the Melexis MLX90393
3-axis magnetometer and temperature sensor. The driver supports raw magnetic
field and temperature measurements, scale and oversampling selection, and
currently uses the I2C interface.

> diff --git a/drivers/iio/magnetometer/mlx90393_core.c b/drivers/iio/magnetometer/mlx90393_core.c
> new file mode 100644
> index 0000000000000..78fcab8a2b566
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_core.c
> @@ -0,0 +1,701 @@

[ ... ]

> +static const int mlx90393_osr2_avail[MLX90393_OSR2_MAX] = {
> +	0, 1, 2, 3,
> +};

[Severity: Medium]
Does this expose raw register values for the temperature oversampling ratio
instead of the actual multipliers?

The IIO ABI requires exposing the actual ratio (1, 2, 4, 8) rather than the
raw hardware indices. Users reading or writing the in_temp_oversampling_ratio
sysfs attribute might be confused by these hardware-specific indices.

[ ... ]

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

[Severity: High]
Can passing these stack-allocated buffers to the I2C transport cause memory
corruption or DMA mapping errors?

Since CONFIG_VMAP_STACK is enabled by default in modern kernels, stack memory
is not physically contiguous and cannot be safely mapped for DMA. When sysfs
operations trigger communication with the device, the I2C controller might use
DMA to transfer data, which expects DMA-safe memory.

This pattern is also present in mlx90393_write_reg() and
mlx90393_read_measurement().

[ ... ]

> +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;
> +	}
> +
> +	*val = 0;
> +	*val2 = mlx90393_scale_table[axis][data->gain_sel][res];
> +
> +	return IIO_VAL_INT_PLUS_NANO;
> +}

[Severity: Medium]
Does this calculate the scale factor correctly for the IIO ABI?

The magnetometer scale values in mlx90393_scale_table are returned using
IIO_VAL_INT_PLUS_NANO, yielding a value like 0.000000751. However, since the
value is 0.751 uT/LSB, this corresponds to 0.00751 Gauss/LSB. Returning it
as nanounits without adjusting the multiplier makes the reported scale off by
a factor of 10,000.

[ ... ]

> +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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE: {
> +		guard(mutex)(&data->lock);
> +		return mlx90393_set_scale(data, chan, val, val2);
> +	}

[Severity: Medium]
Does this allow writing to the temperature scale?

Since IIO_CHAN_INFO_SCALE is checked unconditionally without validating
chan->type, writes to the in_temp_scale sysfs attribute fall through to
mlx90393_set_scale(). Because chan->channel2 is 0 for the temperature channel,
this unintentionally modifies the magnetometer's GAIN_SEL configuration instead
of returning -EINVAL.

[ ... ]

> +static int mlx90393_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan,
> +			     int *val, int *val2, long mask)
> +{

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_MAGN:
> +			return mlx90393_get_scale(data, chan, val, val2);
> +
> +		case IIO_TEMP:
> +			/*
> +			 * Datasheet Table 7: Thermal Specification
> +			 */
> +			*val = 0;
> +			*val2 = 22124;
> +			return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
Does this return the temperature scale in degrees Celsius instead of
milli-Celsius?

The IIO ABI specifies that temperature should be exposed in milli-Celsius, but
returning 0.022124 yields degrees Celsius.

[ ... ]

> +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]
Can concurrent reads of this function corrupt the scale_available sysfs output?

Because scale_avail is declared as a function-static array, it is shared
across all channels and devices. When the IIO core processes the returned
pointer outside of the driver's lock, concurrent reads (e.g., from multiple
axes or probed devices) will overwrite the array and return interleaved or
incorrect values to userspace.

[ ... ]

> +static int mlx90393_init(struct mlx90393_data *data)
> +{
> +	int ret;
> +	u16 reg;
> +
> +	/* Exit mode */
> +	ret = mlx90393_write_cmd(data, MLX90393_CMD_EX);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
> +	 * Wait 1 ms after EX command before issuing RT.
> +	 */
> +	fsleep(1 * USEC_PER_MSEC);
> +
> +	/* Reset device */
> +	ret = mlx90393_write_cmd(data, MLX90393_CMD_RT);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
> +	 * Wait 1.5 ms for the start-up sequence to complete.
> +	 */
> +	fsleep(1.5 * USEC_PER_MSEC);

[Severity: Low]
Will this floating-point literal cause build failures?

The expression uses 1.5, which is a double-precision floating-point literal.
The Linux kernel does not support floating-point arithmetic, which leads to
compilation errors on most architectures that do not support soft-float or
have strict compiler flags against floating-point usage.

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