Re: [PATCH v2 3/3] iio: flow: add Sensirion SLF3S liquid flow sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Severe precision loss in the volumetric flow scale factor due to IIO formatting limitations.
- [High] Stack-allocated and .rodata buffers are passed to I2C transfer functions, which is unsafe for DMA.
- [Medium] Missing power-on delay after enabling the VDD regulator before initiating I2C communication.
--

commit 929caccfae7a9335ed97004b15c2acb13710c7bc
Author: Wadim Mueller <wafgo01@gmail.com>

iio: flow: add Sensirion SLF3S liquid flow sensor driver

Add a driver for the Sensirion SLF3S family of digital liquid-flow
sensors (SLF3S-0600F, SLF3S-1300F and SLF3S-4000B) communicating over I2C.

> diff --git a/drivers/iio/flow/slf3s.c b/drivers/iio/flow/slf3s.c
> new file mode 100644
> index 0000000000000..f971a2dc2c0a4
> --- /dev/null
> +++ b/drivers/iio/flow/slf3s.c

[ ... ]

> +static int slf3s_send_cmd(struct i2c_client *client, const u8 cmd[static 2])
> +{
> +	int ret = i2c_master_send(client, cmd, 2);

[Severity: High]
Does passing cmd directly to i2c_master_send() risk a kernel panic?

Since the callers (like slf3s_detect_variant) pass .rodata arrays here
(e.g., slf3s_cmd_prep_pid), if the underlying I2C adapter utilizes DMA,
dma_map_single() will be called on these read-only buffers. 

Using heap-allocated memory (e.g., a buffer embedded in the state struct,
aligned to IIO_DMA_MINALIGN, and protected by a lock) for I2C transfers 
avoids this.

> +
> +	if (ret == 2)
> +		return 0;
> +	return ret < 0 ? ret : -EIO;
> +}

[ ... ]

> +static int slf3s_detect_variant(struct slf3s_data *sf)
> +{
> +	struct i2c_client *client = sf->client;
> +	u8 buf[SLF3S_PRODUCT_ID_LEN];
> +	int ret;
> +
> +	ret = slf3s_send_cmd(client, slf3s_cmd_prep_pid);
> +	if (ret)
> +		return ret;
> +
> +	ret = slf3s_send_cmd(client, slf3s_cmd_read_pid);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_master_recv(client, buf, sizeof(buf));

[Severity: High]
Can using a stack-allocated buffer for I2C transfers cause issues on systems
using DMA? 

On architectures with CONFIG_VMAP_STACK enabled, mapping a stack address for
DMA is illegal and can trigger a kernel panic. This also applies to the local
buffer used in slf3s_read_sample().

[ ... ]

> +static int slf3s_read_raw(struct iio_dev *indio_dev,
> +			  struct iio_chan_spec const *chan, int *val,
> +			  int *val2, long mask)
> +{
> +	struct slf3s_data *sf = iio_priv(indio_dev);
> +	int flow, temp, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLUMEFLOW) {
> +			*val = sf->variant->scale_num;
> +			*val2 = sf->variant->scale_den;
> +			return IIO_VAL_FRACTIONAL;

[Severity: High]
Does using IIO_VAL_FRACTIONAL here lead to severe precision loss for the
volumetric flow scale factor?

The IIO core computes the fractional scale string by evaluating
div_s64(vals[0] * 1000000000LL, vals[1]). For SLF3S-0600F, this evaluates
to 1000000000 / 600000000 = 1, resulting in a sysfs string of 0.000000001
(1.0e-9).

The true mathematical scale is ~1.666666667e-9. Truncating it to 1.0e-9
introduces a ~40% precision loss, which could severely degrade the measurement
accuracy when userspace calculates the actual flow.

> +		}
> +		*val = SLF3S_TEMP_SCALE_MILLIC;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

> +static int slf3s_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;

[ ... ]

> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable vdd supply\n");
> +
> +	ret = slf3s_detect_variant(sf);

[Severity: Medium]
Could the lack of a power-on delay cause the initial I2C communication to
fail?

The driver enables the 'vdd' regulator and immediately issues an I2C command
via slf3s_detect_variant(). While the regulator core ensures the voltage has
reached the required level, it does not account for the sensor IC's internal
boot or reset time.

If the regulator physically toggles power, the sensor might require several
milliseconds to initialize before it can ACK I2C transactions. Could this
cause the sensor to NACK the initial command, leading to an -EIO probe
failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527184257.141635-1-wafgo01@gmail.com?part=3