Re: [PATCH RFC 2/3] iio: osf: add Open Sensor Fusion UART IIO driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The IIO channels are declared as little-endian (IIO_LE), but the driver pushes CPU-native endian data to the IIO kfifo buffer.
- [High] The driver pushes unpacked channel data into the IIO buffer but fails to set `available_scan_masks`, causing incorrect data demuxing and timestamp overwriting when a subset of channels is active.
- [Medium] Unbounded memory leak via `devm_` allocations in the error path of dynamic IIO device registration, triggered by repeated capability reports.
- [Medium] The driver hardcodes `ktime_get_ns()` for sample timestamps, breaking the IIO subsystem's clock selection mechanism.
--

commit 07b7754f2b44c1840fd85c5de6bb124482d998fd
Author: Jinseob Kim <kimjinseob88@gmail.com>

iio: osf: add Open Sensor Fusion UART IIO driver

Add the initial Open Sensor Fusion UART IIO driver. This includes the
protocol v0 parser, OSF0 stream assembler, core capability/sample cache,
serdev UART transport, IIO read_raw path, and software kfifo buffer support.

> diff --git a/drivers/iio/opensensorfusion/osf_core.c b/drivers/iio/opensensorfusion/osf_core.c
> new file mode 100644
> index 0000000000000..fd2eccefc47d6
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_core.c
> @@ -0,0 +1,334 @@

[ ... ]

> +static int osf_core_handle_sensor_sample(struct osf_device *osf,
> +					 const struct osf_frame *frame)
> +{

[ ... ]

> +	event.sensor_type = sample.sensor_type;
> +	event.sensor_index = sample.sensor_index;
> +	event.channel_count = sample.channel_count;
> +	event.sample_format = sample.sample_format;
> +	event.scale_nano = sample.scale_nano;
> +	event.sequence = frame->sequence;
> +	event.timestamp_us = frame->timestamp_us;
> +	event.host_timestamp_ns = ktime_get_ns();

[Severity: Medium]
Does this hardcode the timestamp clock to CLOCK_MONOTONIC? 

The IIO subsystem supports user-configurable clocks via the
current_timestamp_clock sysfs attribute. Should this use iio_get_time_ns()
instead to honor the user's clock selection?

> +	for (i = 0; i < sample.channel_count; i++)
> +		event.values[i] = values[i];

[ ... ]

> diff --git a/drivers/iio/opensensorfusion/osf_iio.c b/drivers/iio/opensensorfusion/osf_iio.c
> new file mode 100644
> index 0000000000000..3d2674e43fbb0
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_iio.c
> @@ -0,0 +1,268 @@

[ ... ]

> +#define OSF_SCAN_TYPE_S32						\
> +	{								\
> +		.sign = 's',						\
> +		.realbits = 32,					\
> +		.storagebits = 32,					\
> +		.endianness = IIO_LE,					\
> +	}

[ ... ]

> +int osf_iio_push_sample(struct iio_dev *indio_dev,
> +			const struct osf_sample_event *event)
> +{
> +	struct osf_iio_state *state;
> +	u8 scan[OSF_IIO_SCAN_BYTES] __aligned(8) = { };
> +	s32 *scan_values = (s32 *)scan;
> +	unsigned int i;

[ ... ]

> +	for (i = 0; i < event->channel_count; i++)
> +		scan_values[i] = event->values[i];
> +
> +	return iio_push_to_buffers_with_timestamp(indio_dev, scan,
> +						  event->host_timestamp_ns);

[Severity: High]
Does this code push CPU-native endian data to the IIO buffer?

The channels are declared as IIO_LE, so userspace on big-endian architectures
will byte-swap this data again. Should the values be converted back using
cpu_to_le32(), or should the channel be declared as IIO_CPU?

> +}
> +
> +int osf_iio_register_sensor(struct device *dev,
> +			    const struct osf_capability_entry *entry,
> +			    void *driver_data, struct iio_dev **indio_dev)
> +{

[ ... ]

> +	iio_dev->name = spec->name;
> +	iio_dev->info = &osf_iio_info;
> +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	iio_dev->channels = spec->channels;
> +	iio_dev->num_channels = spec->num_channels;

[Severity: High]
Does this code omit setting available_scan_masks while pushing unpacked data
into the buffer?

If userspace enables only a subset of channels, the IIO core will expect the
data to be tightly packed. The dynamic timestamp placement by
iio_push_to_buffers_with_timestamp() could then overwrite active channel
data. Providing available_scan_masks would enable the software demuxing
fallback to correct the data alignment.

> +
> +	ret = devm_iio_kfifo_buffer_setup(dev, iio_dev, NULL);

[ ... ]

> diff --git a/drivers/iio/opensensorfusion/osf_serdev.c b/drivers/iio/opensensorfusion/osf_serdev.c
> new file mode 100644
> index 0000000000000..1827760c27f24
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_serdev.c
> @@ -0,0 +1,354 @@

[ ... ]

> +static int osf_serdev_register_iio(struct osf_serdev *osf_uart,
> +				   const struct osf_capability_entry *entry)
> +{
> +	struct device *dev = &osf_uart->serdev->dev;
> +	struct iio_dev *indio_dev;
> +	int ret;

[ ... ]

> +	ret = osf_iio_register_sensor(dev, entry, &osf_uart->osf, &indio_dev);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Does this error path leak the devm_ allocated resources if device
registration fails?

Since the sensor isn't added to the iio array on failure, a new capability
report with a different sequence number will trigger registration again. This
could potentially lead to an unbounded memory leak until out-of-memory.

Should this explicitly free the devm_ resources on failure?

> +
> +	osf_uart->iio[osf_uart->iio_count].sensor_type = entry->sensor_type;

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