Re: [PATCH RFC v5 4/6] iio: osf: add stream parser

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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