Re: [PATCH RFC v5 4/6] iio: osf: add stream parser
From: Andy Shevchenko
Date: Tue Jun 16 2026 - 07:16:57 EST
On Tue, Jun 16, 2026 at 04:22:40PM +0900, Jinseob Kim wrote:
> Add a byte-stream parser that resynchronizes on OSF frame magic, validates
> complete frames, and forwards decoded frames to the OSF core.
...
> +static const u8 osf_stream_magic[OSF_STREAM_MAGIC_LEN] = {
> + 'O', 'S', 'F', '0',
> +};
Why?! You have already a definition, use it instead.
...
> +static size_t osf_stream_discard_to_magic(struct osf_stream *stream)
> +{
> + size_t old_len = stream->len;
> + size_t match_len;
> + size_t i;
> +
> + for (i = 0; i < stream->len; i++) {
In current form it's as simple as
for (size_t i = 0; i < stream->len; i++) {
> + match_len = stream->len - i;
> + if (match_len > OSF_STREAM_MAGIC_LEN)
> + match_len = OSF_STREAM_MAGIC_LEN;
> +
> + if (osf_stream_magic_match(stream->buf + i, match_len)) {
> + if (i)
> + osf_stream_discard(stream, i);
> + return i;
> + }
> + }
> +
> + stream->len = 0;
> + return old_len;
> +}
...
> +static int osf_stream_process(struct osf_stream *stream)
> +{
> + size_t discarded;
> + size_t frame_len;
> + u32 payload_len;
> + int first_err = 0;
> + int ret;
> +
> + while (stream->len) {
> + discarded = osf_stream_discard_to_magic(stream);
> + if (discarded) {
> + stream->stats.bad_magic_resyncs++;
> + stream->stats.dropped_bytes += discarded;
> + if (!first_err)
> + first_err = -EPROTO;
> + }
> +
> + if (!stream->len)
> + break;
> +
> + if (stream->len < OSF_FRAME_HEADER_LEN)
> + break;
> + if (get_unaligned_le16(stream->buf + 6) !=
> + OSF_FRAME_HEADER_LEN) {
Make it a single line for readability.
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EPROTO;
> + continue;
> + }
> +
> + payload_len = get_unaligned_le32(stream->buf + 10);
> + if (payload_len > OSF_STREAM_MAX_PAYLOAD_LEN) {
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + if (!first_err)
> + first_err = -EMSGSIZE;
> + continue;
> + }
> +
> + frame_len = OSF_FRAME_HEADER_LEN + payload_len + OSF_FRAME_CRC_LEN;
> + if (stream->len < frame_len)
> + break;
> +
> + ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);
> + if (ret) {
> + if (ret == -EBADMSG) {
> + stream->stats.bad_crc_frames++;
> + stream->stats.dropped_bytes++;
> + osf_stream_drop_invalid_head(stream);
> + } else {
> + osf_stream_discard(stream, frame_len);
> + }
> + if (!first_err)
> + first_err = ret;
> + continue;
> + }
> +
> + stream->stats.valid_frames++;
> + osf_stream_discard(stream, frame_len);
> + }
> + return first_err;
Why do we continue on the error and then still return an error?
Same Q for the receive part.
> +}
...
> +void osf_stream_init(struct osf_stream *stream, struct osf_device *osf)
> +{
> + if (!stream)
> + return;
> +
> + stream->osf = osf;
> + stream->len = 0;
> + memset(&stream->stats, 0, sizeof(stream->stats));
> +}
> +
> +void osf_stream_reset(struct osf_stream *stream)
> +{
> + if (stream) {
> + stream->len = 0;
> + memset(&stream->stats, 0, sizeof(stream->stats));
> + }
As per above
if (!stream)
return;
> +}
...
> +struct osf_stream_stats {
> + u64 valid_frames;
> + u64 bad_magic_resyncs;
> + u64 bad_crc_frames;
> + u64 partial_frames;
> + u64 dropped_bytes;
> +};
Don't you want to use linux/u64_stats_sync.h APIs?
--
With Best Regards,
Andy Shevchenko