Re: [PATCH v7 4/5] iio: osf: add authenticated stream parser

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

On Tue, Jul 07, 2026 at 10:45:24AM +0900, Jinseob Kim wrote:
> Add a UART byte-stream parser for Open Sensor Fusion frames.
> 
> The parser searches for the OSF0 wire magic, keeps partial frames
> buffered, checks header length and payload bounds, and passes complete
> candidate frames to the core decoder.
> 
> Rejected candidate frames drop only the current head byte before
> resynchronizing, so a corrupted unauthenticated payload length cannot
> make the parser skip later valid frames.

...

> +static bool osf_stream_frame_prefix_match(const u8 *buf, size_t len)
> +{
> +	for (size_t i = 0; i < len; i++) {
> +		if (buf[i] != (u8)(OSF_FRAME_MAGIC >> (i * 8)))
> +			return false;
> +	}
> +
> +	return true;

Why so complicated? le32_to_cpup() + just integer comparison should work, no?
Alternatively get_unaligned_le32() if the buffer is unaligned.

> +}

...

> +static size_t osf_stream_discard_to_magic(struct osf_stream *stream)
> +{
> +	size_t old_len = stream->len;
> +	size_t match_len;
> +
> +	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;

Seems like reinvention of min() from minmax.h.

> +		if (osf_stream_frame_prefix_match(stream->buf + i, match_len)) {
> +			if (i)
> +				osf_stream_discard(stream, i);
> +			return i;
> +		}
> +	}
> +
> +	stream->len = 0;
> +	return old_len;
> +}

...

> +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) {

I believe I have commented on this already. Please, go and double check all comments.

> +		stream->len = 0;
> +		memset(&stream->stats, 0, sizeof(stream->stats));
> +	}

My comment was to use the same pattern as in _init() above.

> +}

...

> +int osf_stream_receive_bytes(struct osf_stream *stream, const u8 *buf,
> +			     size_t len)

Not sure, but likely also commented on this, id est to wrap on logical split:

int osf_stream_receive_bytes(struct osf_stream *stream,
			     const u8 *buf, size_t len)

OR simply do in a single line (it's only 82 characters with increased
readability):

int osf_stream_receive_bytes(struct osf_stream *stream, const u8 *buf, size_t len)

> +{
> +	size_t copy_len;
> +	size_t space;
> +	int first_err = 0;
> +	int ret;
> +
> +	if (!stream || !stream->osf || (!buf && len))
> +		return -EINVAL;
> +
> +	if (!len) {
> +		ret = osf_stream_process(stream);
> +		if (ret && !first_err)
> +			first_err = ret;
> +		return first_err;

Why all this dances? first_err is 0, we all know this.

> +	}
> +
> +	while (len) {
> +		space = OSF_STREAM_MAX_FRAME_LEN - stream->len;
> +		if (!space) {
> +			stream->stats.dropped_bytes++;
> +			osf_stream_discard(stream, 1);
> +			if (!first_err)
> +				first_err = -EMSGSIZE;
> +			continue;
> +		}
> +
> +		copy_len = len < space ? len : space;
> +		memcpy(stream->buf + stream->len, buf, copy_len);
> +		stream->len += copy_len;
> +		buf += copy_len;
> +		len -= copy_len;
> +
> +		ret = osf_stream_process(stream);
> +		if (ret && !first_err)
> +			first_err = ret;

This error checking and handling is unusual. It requires a good comment
explaining what's going on.

> +	}
> +
> +	return first_err;
> +}

-- 
With Best Regards,
Andy Shevchenko