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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `partial_frames` statistic incorrectly increments multiple times for a single valid frame during normal stream reception.
- [Medium] Frame is fully decoded into a local structure that is subsequently discarded without being used, causing duplicate CRC and parsing overhead.
--

commit 8eb00bef4725c351a5a3267078dfac541b5f3a1b
Author: Jinseob Kim <kimjinseob88@gmail.com>

iio: osf: add stream parser

Add the byte stream assembler and resync handling for OSF0 UART frames.

> diff --git a/drivers/iio/opensensorfusion/osf_stream.c b/drivers/iio/opensensorfusion/osf_stream.c
> new file mode 100644
> index 0000000000000..a2739c987da3f
> --- /dev/null
> +++ b/drivers/iio/opensensorfusion/osf_stream.c

[ ... ]

> +static int osf_stream_process(struct osf_stream *stream)
> +{
> +	struct osf_frame frame;
> +	size_t decoded_len;
> +	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) {
> +			stream->stats.partial_frames++;

[Severity: Medium]
Since UART receives data in small chunks, will osf_stream_receive_bytes()
repeatedly hit this break condition while accumulating a single valid frame?

It looks like this might inflate partial_frames into a count of receive wait
states rather than actual truncated frames.

> +			break;
> +		}
> +
> +		if (get_unaligned_le16(stream->buf + 6) !=
> +		    OSF_FRAME_HEADER_LEN) {
> +			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) {
> +			stream->stats.partial_frames++;

[Severity: Medium]
Similar to the check above, does this increment partial_frames incorrectly
when simply waiting for the rest of the payload and CRC to arrive over UART?

> +			break;
> +		}
> +
> +		ret = osf_protocol_decode_frame(stream->buf, frame_len, &frame,
> +						&decoded_len);
> +		if (ret) {
> +			if (ret == -EBADMSG)
> +				stream->stats.bad_crc_frames++;
> +			stream->stats.dropped_bytes++;
> +			osf_stream_drop_invalid_head(stream);
> +			if (!first_err)
> +				first_err = ret;
> +			continue;
> +		}
> +
> +		if (decoded_len != frame_len) {
> +			stream->stats.dropped_bytes++;
> +			osf_stream_drop_invalid_head(stream);
> +			if (!first_err)
> +				first_err = -EMSGSIZE;
> +			continue;
> +		}
> +
> +		ret = osf_core_receive_frame(stream->osf, stream->buf, frame_len);

[Severity: Medium]
The frame structure is fully decoded and its CRC calculated by
osf_protocol_decode_frame() above, but then the local frame struct is
discarded.

Does passing stream->buf directly to osf_core_receive_frame() cause the
frame to be decoded and the CRC to be calculated a second time?

> +		if (ret) {
> +			osf_stream_discard(stream, frame_len);
> +			if (!first_err)
> +				first_err = ret;
> +			continue;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529121005.1470-1-kimjinseob88@gmail.com?part=4