Re: [PATCH RFC v3 3/6] iio: osf: add protocol v0 decoding

[Jonathan Cameron]

On Fri, 29 May 2026 21:10:02 +0900
Jinseob Kim <kimjinseob88@xxxxxxxxx> wrote:

> Add OSF0 frame validation and payload decoders.
> 
> Extend MAINTAINERS to cover the protocol decoder.
> 
> Signed-off-by: Jinseob Kim <kimjinseob88@xxxxxxxxx>

I've commented on a few things inline. Some of them apply in multiple
places. Biggest thing is asking for rules on how this 'specification'
handled backwards compatibility.

Jonathan

> diff --git a/drivers/iio/opensensorfusion/osf_protocol.c b/drivers/iio/opensensorfusion/osf_protocol.c
> new file mode 100644
> index 000000000..ed91d3dd5
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.c

> +
> +int osf_protocol_decode_capability_report(const struct osf_frame *frame,
> +					  struct osf_capability_report *report)
> +{
> +	u16 capability_count;
> +	size_t expected_len;
> +	const u8 *payload;
> +
> +	if (!frame || !report || !frame->payload)
> +		return -EINVAL;
> +
> +	if (frame->message_type != OSF_MSG_CAPABILITY_REPORT)
> +		return -EPROTO;
> +
> +	if (frame->payload_len < OSF_CAP_REPORT_BASE_LEN)
> +		return -EMSGSIZE;
> +
> +	payload = frame->payload;
> +	capability_count = get_unaligned_le16(payload);
> +
> +	if (get_unaligned_le16(payload + 2))

As below - these zero checks to me look like a protocol design bug.
If you can I'd encourage adding something on rules for backward compatibility
to your initial spec docs.

> +		return -EPROTO;
> +
> +	if (capability_count > (SIZE_MAX - OSF_CAP_REPORT_BASE_LEN) /
> +	    OSF_CAP_SENSOR_ENTRY_LEN)

Alignment can be easier to read.

	if (capability_count >
	    (SIZE_MAX - OSF_CAP_REPORT_BASE_LEN) / OSF_CAP_SENSOR_ENTRY_LEN)

> +		return -EOVERFLOW;
> +
> +	expected_len = OSF_CAP_REPORT_BASE_LEN +
> +		       capability_count * OSF_CAP_SENSOR_ENTRY_LEN;
> +	if (frame->payload_len != expected_len)
> +		return -EMSGSIZE;
> +
> +	report->capability_count = capability_count;
> +	report->entries = payload + OSF_CAP_REPORT_BASE_LEN;
> +
> +	return 0;
> +}
> +
> +int osf_protocol_decode_capability_entry(const struct osf_capability_report *report,
> +					 unsigned int index,
> +					 struct osf_capability_entry *entry)
> +{
> +	u16 sample_format;
> +	u16 sensor_type;
> +	u32 flags;
> +	const u8 *payload;

If no other reason for order, use reverse xmas tree. Also fine to combine entries of
same type as long as non assign values. Up to you if you'd prefer not to.

	u16 sample_format, sensor_type;
	const u8 *payload;
	u32 flags;
> +
> +	if (!report || !report->entries || !entry)
> +		return -EINVAL;
> +
> +	if (index >= report->capability_count)
> +		return -ERANGE;
> +
> +	payload = report->entries + index * OSF_CAP_SENSOR_ENTRY_LEN;
> +	sensor_type = get_unaligned_le16(payload);
> +	sample_format = get_unaligned_le16(payload + 6);
> +	flags = get_unaligned_le32(payload + 12);
> +
> +	if (!osf_sensor_type_valid(sensor_type))
> +		return -EPROTO;
> +
> +	if (sample_format != OSF_SAMPLE_FORMAT_S32)
> +		return -EPROTO;
> +
> +	if (flags & ~OSF_CAPABILITY_FLAGS_MASK)
> +		return -EPROTO;
> +
> +	if (get_unaligned_le32(payload + 16))

That needs more info. I guess it's check reserved is 0?
In an extensible protocol design there should be no need to check
that.  If a future version adds stuff in there then there are rules
on how it is done.

1) Not by default, needs to be enabled be aware software.
2) If by default, can be ignored, but must be discoverable - i.e.
   we must know if 0 means the value 0 or reserved0
3) If we can't ignore it then 0 must be natural default.  I.e.
   future capability bits, 0 means not supported.

Upshot, unless the protocol design for future versions is broken
there should never be a reason to check reserved values are 0
other than debug.

If you really need to keep this because the protocol design is
not backwards compatible, then add a comment on what that check is.


> +		return -EPROTO;
> +
> +	entry->sensor_type = sensor_type;
> +	entry->sensor_index = get_unaligned_le16(payload + 2);
> +	entry->channel_count = get_unaligned_le16(payload + 4);
> +	entry->sample_format = sample_format;
> +	entry->scale_nano = get_unaligned_le32(payload + 8);
> +	entry->flags = flags;
> +
> +	return 0;
> +}
> diff --git a/drivers/iio/opensensorfusion/osf_protocol.h b/drivers/iio/opensensorfusion/osf_protocol.h
> new file mode 100644
> index 000000000..4b6fb131a
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_protocol.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _OSF_PROTOCOL_H
> +#define _OSF_PROTOCOL_H
> +
> +#include <linux/types.h>
> +
> +#define OSF_PROTOCOL_MAJOR		0
> +#define OSF_PROTOCOL_MINOR		0
> +#define OSF_FRAME_HEADER_LEN		38
> +#define OSF_FRAME_CRC_LEN		4
> +#define OSF_FRAME_MIN_LEN		(OSF_FRAME_HEADER_LEN + OSF_FRAME_CRC_LEN)
> +
> +#define OSF_SENSOR_SAMPLE_BASE_LEN	16
> +#define OSF_DEVICE_STATUS_LEN		20
> +#define OSF_CAP_REPORT_BASE_LEN		4
> +#define OSF_CAP_SENSOR_ENTRY_LEN		20
> +#define OSF_CAPABILITY_FLAGS_MASK	0x00000003U
> +
> +enum osf_message_type {

Ideally each of these blocks would have a reference to a spec document section.
From website I guess that doc is a work in progress?

> +	OSF_MSG_SENSOR_SAMPLE		= 0x0001,
> +	OSF_MSG_DEVICE_STATUS		= 0x0002,
> +	OSF_MSG_CAPABILITY_REPORT	= 0x0003,
> +};
> +
> +enum osf_sensor_type {
> +	OSF_SENSOR_ACCELEROMETER		= 0x0001,
> +	OSF_SENSOR_GYROSCOPE		= 0x0002,
> +	OSF_SENSOR_MAGNETOMETER		= 0x0003,
> +	OSF_SENSOR_BAROMETER		= 0x0004,
> +	OSF_SENSOR_TEMPERATURE		= 0x0005,
> +	OSF_SENSOR_HUMIDITY		= 0x0006,
> +	OSF_SENSOR_AMBIENT_LIGHT		= 0x0007,
> +	OSF_SENSOR_PROXIMITY		= 0x0008,
> +};
> +
> +enum osf_sample_format {
> +	OSF_SAMPLE_FORMAT_S32		= 0x0001,
> +};
> +

> +struct osf_frame {
> +	u8 protocol_minor;
> +	u16 message_type;
> +	u32 payload_len;
> +	u64 sequence;
> +	u64 timestamp_us;
> +	u32 flags;
> +	const u8 *payload;
#
__counted_by_ptr(payload_len); ?

> +	u32 crc;
> +};
> +
> +struct osf_sensor_sample {
> +	u16 sensor_type;
> +	u16 sensor_index;
> +	u16 channel_count;
> +	u16 sample_format;
> +	u32 scale_nano;
> +	const u8 *samples;

I guess we don't know the alignement so can't
type this and that also stops use doing __counted_by_ptr() which
is annoying.

> +};
> +
> +struct osf_device_status {
> +	u32 uptime_s;
> +	u32 status_flags;
> +	u32 error_flags;
> +	u32 dropped_frames;
> +};
> +
> +struct osf_capability_report {
> +	u16 capability_count;
> +	const u8 *entries;
> +};
> +
> +struct osf_capability_entry {
> +	u16 sensor_type;
> +	u16 sensor_index;
> +	u16 channel_count;
> +	u16 sample_format;
> +	u32 scale_nano;
> +	u32 flags;
> +};