Re: [PATCH RFC v6 3/5] iio: osf: add protocol decoding

[Kim Jinseob]

I will fix these in the next revision.

- adjust the OSF_FRAME_MAGIC comment as suggested;
- remove or rewrite the overflow checks that are effectively dead for the
  current u16 counts;
- build with W=1 using both GCC and clang before posting.

I will also include these items in the review tracking before posting another
revision.

Thanks,
Jinseob

2026년 6월 29일 (월) 오후 11:05, Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>님이 작성:
>
> On Mon, Jun 29, 2026 at 04:13:35AM +0900, Jinseob Kim wrote:
> > Add helpers for decoding Open Sensor Fusion frame headers and supported
> > message payloads.
> >
> > The decoder validates the OSF0 wire magic, protocol major version,
> > header length, payload bounds, reserved fields and CRC before exposing
> > decoded frame contents to the rest of the driver.
> >
> > Use explicit little-endian wire storage sizes and designated
> > initializers for decoded output structures.
>
> ...
>
> > +#include <linux/bits.h>
> > +#include <linux/crc32.h>
> > +#include <linux/errno.h>
> > +#include <linux/limits.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
>
> ...
>
> > +#define OSF_FRAME_MAGIC              0x3046534f /* "OSF0" little-endian */
>
> #define OSF_FRAME_MAGIC         0x3046534f /* "OSF0", little-endian */
>
> (mind a comma).
>
> ...
>
> > +int osf_protocol_decode_sensor_sample(const struct osf_frame *frame,
> > +                                   struct osf_sensor_sample *sample)
> > +{
> > +     u16 channel_count;
> > +     u16 sample_format;
> > +     u16 sensor_type;
> > +     size_t expected_len;
> > +     const u8 *payload;
> > +
> > +     if (!frame || !sample || !frame->payload)
> > +             return -EINVAL;
> > +
> > +     if (frame->message_type != OSF_MSG_SENSOR_SAMPLE)
> > +             return -EPROTO;
> > +
> > +     if (frame->payload_len < OSF_SENSOR_SAMPLE_BASE_LEN)
> > +             return -EMSGSIZE;
> > +
> > +     payload = frame->payload;
> > +     sensor_type = get_unaligned_le16(payload);
> > +     channel_count = get_unaligned_le16(payload + 4);
> > +     sample_format = get_unaligned_le16(payload + 6);
> > +
> > +     if (!osf_sensor_type_valid(sensor_type))
> > +             return -EPROTO;
> > +
> > +     if (!channel_count)
> > +             return -EPROTO;
> > +
> > +     if (sample_format != OSF_SAMPLE_FORMAT_S32)
> > +             return -EPROTO;
> > +
> > +     if (get_unaligned_le32(payload + 12))
> > +             return -EPROTO;
>
> > +     if (channel_count > (SIZE_MAX - OSF_SENSOR_SAMPLE_BASE_LEN) /
> > +         sizeof(__le32))
> > +             return -EOVERFLOW;
>
> Dead code because it's always 'false'? Hasn't compiler given a warning?
> Always compile your code with `make W=1` using both compilers: clang and GCC.
>
> > +     expected_len = OSF_SENSOR_SAMPLE_BASE_LEN + channel_count * sizeof(__le32);
> > +     if (frame->payload_len != expected_len)
> > +             return -EMSGSIZE;
> > +
> > +     *sample = (struct osf_sensor_sample) {
> > +             .sensor_type = sensor_type,
> > +             .sensor_index = get_unaligned_le16(payload + 2),
> > +             .channel_count = channel_count,
> > +             .sample_format = sample_format,
> > +             .scale_nano = get_unaligned_le32(payload + 8),
> > +             .samples = payload + OSF_SENSOR_SAMPLE_BASE_LEN,
> > +     };
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +     if (capability_count > (SIZE_MAX - OSF_CAP_REPORT_BASE_LEN) /
> > +         OSF_CAP_SENSOR_ENTRY_LEN)
> > +             return -EOVERFLOW;
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>