Re: [PATCH 2/2] iio: pressure: add Sensirion SDP31 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iio: pressure: data race on global sdp31_crc8_table during concurrent probes
- [Low] i2c: sdp31_id array uses positional instead of named initializers
--

commit 64f407e74ab363c060c44155c0fe0d881043f834
Author: Muhammad Abu Bakar <m.abubakar365@yahoo.com>

    iio: pressure: add Sensirion SDP31 driver
    
    Add an IIO driver for the Sensirion SDP31 differential pressure sensor.
    The device is accessed over I2C and reports differential pressure and
    temperature. Each measurement is validated using the sensor's CRC-8
    checksum.

> diff --git a/drivers/iio/pressure/sdp31.c b/drivers/iio/pressure/sdp31.c
> new file mode 100644
> index 0000000000000..934a3afcde7f3
> --- /dev/null
> +++ b/drivers/iio/pressure/sdp31.c
[ ... ]
> +DECLARE_CRC8_TABLE(sdp31_crc8_table);
[ ... ]
> +static int sdp31_check_crc(const u8 *word)
> +{
> +	if (crc8(sdp31_crc8_table, word, 2, SDP31_CRC8_INIT) != word[2])
> +		return -EIO;
> +	return 0;
> +}
[ ... ]
> +static int sdp31_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct sdp31_data *data;
> +	struct sdp31_reading r;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	mutex_init(&data->lock);
> +
> +	crc8_populate_msb(sdp31_crc8_table, SDP31_CRC8_POLY);

[Severity: High]
Is it possible for this crc8_populate_msb() call to race with concurrent reads
of the same table?

DECLARE_CRC8_TABLE(sdp31_crc8_table) creates a global static array. If one
SDP31 device is actively queried from userspace, it reads from
sdp31_crc8_table in sdp31_check_crc().

If a second SDP31 device is probed simultaneously (for example, via a bus
rescan or hotplug event), sdp31_probe() will unconditionally write to this
global array without synchronization.

Could this initialization be protected with something like DO_ONCE(), or moved
to the module initialization phase?

[ ... ]
> +static const struct i2c_device_id sdp31_id[] = {
> +	{ "sdp31" },

[Severity: Low]
Should this array use named initializers to align with I2C subsystem
guidelines?

The I2C subsystem requires arrays of type struct i2c_device_id to explicitly
use named initializers.

Could this be updated to use explicit initialization, such as
{ .name = "sdp31" }?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, sdp31_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919223854.13437-1-m.abubakar365@yahoo.com?part=2