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

[Kim Jinseob]

> 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.

Agreed. I'll change this to a designated initializer in the next revision.

> 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.

I'd prefer to keep the exact-length check for the existing OSF0
message IDs. The fixed OSF0 specification deliberately keeps the
baseline message layouts exact, with optional extensions using newly
allocated message IDs rather than trailing data on an existing message
ID.

So I don't plan to change this to an at-least-length check in the next revision.

> 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!

Agreed. I'll remove the reserved fields from the decoded structures.
The reserved bytes will still be covered by the CRC and tolerated on
receive, but they don't need to be propagated past the protocol
decoder.

Thanks,
Jinseob

2026년 9월 20일 (일) 오전 10:27, Jonathan Cameron <jic23@xxxxxxxxxx>님이 작성:
>
> 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;
> > +}