Re: [PATCH v10 3/8] iio: osf: add protocol decoding

[Jonathan Cameron]

On Sat, 19 Sep 2026 03:24:41 +0900
Jinseob Kim <kimjinseob88@xxxxxxxxx> wrote:

> Add helpers for decoding Open Sensor Fusion frame headers and supported
> message payloads.
> 
> Validate the fixed OSF0 envelope, payload bounds and CRC before exposing
> decoded frame contents. Require exact known payload lengths and decode
> capability entries structurally so the core can apply support policy.
> Tolerate reserved padding as required by the fixed protocol contract.
> 
> Use explicit little-endian wire storage sizes and designated
> initializers for decoded output structures.
> 
> Assisted-by: LLM
> Signed-off-by: Jinseob Kim <kimjinseob88@xxxxxxxxx>
Hi Jinseob

A few things inline. Some are about what I'd kind of expect from
how specs are often defined to leave a bit of flexibility and reduce
the need to update drivers for new stuff being added.

If you don't want to go that way I don't really mind.

Otherwise main thing here is why carry reserved data to next layer
given it is meant to be ignored. Drop it down here in the protocol decode.

Thanks,

Jonathan

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

> +
> +int osf_protocol_decode_frame(const u8 *buf, size_t len,
> +			      struct osf_frame *frame, size_t *frame_len)
> +{
> +	u32 expected_crc;
> +	u32 actual_crc;
> +	u32 payload_len;
> +	size_t total_len;
> +
> +	if (!buf || !frame || !frame_len)
> +		return -EINVAL;
> +
> +	if (len < OSF_FRAME_MIN_LEN)
> +		return -EMSGSIZE;
> +
> +	if (get_unaligned_le32(buf) != OSF_FRAME_MAGIC)
> +		return -EPROTO;
> +
> +	if (get_unaligned_le16(buf + 6) != OSF_FRAME_HEADER_LEN)
> +		return -EPROTO;
> +
> +	payload_len = get_unaligned_le32(buf + 10);
> +	if (payload_len > len - OSF_FRAME_MIN_LEN)
> +		return -EMSGSIZE;
> +
> +	total_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
> +	expected_crc = osf_crc32_ieee(buf, OSF_FRAME_HEADER_LEN + payload_len);
> +	actual_crc = get_unaligned_le32(buf + OSF_FRAME_HEADER_LEN + payload_len);
> +
> +	if (actual_crc != expected_crc)
> +		return -EBADMSG;
> +
> +	frame->protocol_major = buf[4];
> +	frame->protocol_minor = buf[5];
> +	frame->message_type = get_unaligned_le16(buf + 8);
> +	frame->payload_len = payload_len;
> +	frame->sequence = get_unaligned_le64(buf + 14);
> +	frame->timestamp_us = get_unaligned_le64(buf + 22);
> +	frame->flags = get_unaligned_le32(buf + 30);
> +	frame->reserved = get_unaligned_le32(buf + 34);

As below. I'm not sure what benefit of keeping reserved around is.

> +	frame->payload = buf + OSF_FRAME_HEADER_LEN;
> +	frame->crc = actual_crc;

The above fully assigns frame so I'd do similar to you have elsewhere
	*frame = (struct osf_frame) {
		.protocol_major = ...
...
	};
Both makes it more readable and makes it clear you aren't leaving
any existing data in place.


> +	*frame_len = total_len;
> +
> +	return 0;
> +}

> +
> +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);
> +
> +	expected_len = OSF_CAP_REPORT_BASE_LEN +
> +		       capability_count * OSF_CAP_SENSOR_ENTRY_LEN;
> +	if (frame->payload_len != expected_len)

I don't hugely mind as it's your code + spec to maintain but generally
for a spec with records like this I'd expect it to be possible to extend
the structure without it being a breaking spec change.   As such
I'd kind of expect the check to be that the payload_len was at least
as big as expected len. Larger would be fine but we'd ignore anything
there.

> +		return -EMSGSIZE;
> +
> +	*report = (struct osf_capability_report) {
> +		.capability_count = capability_count,
> +		.entries = payload + OSF_CAP_REPORT_BASE_LEN,
> +	};
> +
> +	return 0;
> +}
> +
> +int osf_protocol_decode_capability_entry(const struct osf_capability_report
> +					 *report, u16 index,
> +					 struct osf_capability_entry *entry)
> +{
> +	const u8 *payload;
> +
> +	if (!report || !report->entries || !entry)
> +		return -EINVAL;
> +
> +	if (index >= report->capability_count)
> +		return -ERANGE;
> +
> +	payload = report->entries + index * OSF_CAP_SENSOR_ENTRY_LEN;
> +	*entry = (struct osf_capability_entry) {
> +		.sensor_type = get_unaligned_le16(payload),
> +		.sensor_index = get_unaligned_le16(payload + 2),
> +		.channel_count = get_unaligned_le16(payload + 4),
> +		.sample_format = get_unaligned_le16(payload + 6),
> +		.scale_nano = get_unaligned_le32(payload + 8),
> +		.flags = get_unaligned_le32(payload + 12),
> +		.reserved = get_unaligned_le32(payload + 16),

Entirely correctly I think you don't do any checks or reads of reserved values.
As such why copy them around?  I'd just drop that field from your decoded
structures.  Not a huge saving but why carry irrelevant data around!

Jonathan

> +	};
> +
> +	return 0;
> +}