Re: [PATCH RFC v2 3/7] iio: osf: add protocol v0 decoding

[Kim Jinseob]

Sorry, resending in plain text.

On Thu, May 28, 2026 at 10:49:00PM +0100, Jonathan Cameron wrote:
> https://sashiko.dev/#/patchset/20260524085312.15369-1-kimjinseob88%40gmail.com
> Has some perhaps useful feedback. Please make sure to either exclude
> them as false positives or address for next version.
>
> Quite a bit of the feedback in here is about which checks are actually
> useful. Normal kernel practice is assume we didn't shoot outselves in the
> foot but hardware and userspace may have done. So we defend only against
> things in their control.

Addressed in RFC v3.

I reworked the decoder to focus validation on device-controlled fields. The
v3 decoder now validates sensor_type, sample_format, channel_count, reserved
fields, and payload length overflow cases.

I also removed unnecessary internal-only caller checks where the call path
already guarantees the arguments, while keeping protocol and stream boundary
checks.

> Similar on whether this defensive code makes sense

Addressed in RFC v3. I removed the remaining internal-only defensive checks
where the caller already guarantees valid arguments.

> I guess it might change in future, but for now why not make samples an s32 *

For the variable sample data I kept the wire-format decoding explicit because
the payload is little-endian data from the device and may not be naturally
aligned. The v3 code validates the format and channel count before the sample
data is consumed.

> Similar sanity check questions.
>
> If that happens seems like something went very wrong elsewhere so
> seems unlikely defense makes sense here.

Addressed in RFC v3. I trimmed the redundant internal consistency checks and
kept the checks that validate frame and payload data received from the device.

> This one does belong in here.

Kept in RFC v3. Payload length validation remains part of the decoder.

> This check is fine as picks up on bad hardware, but why keep
> the value of reserved?

Addressed in RFC v3. Reserved fields are now validate-only. The decoder checks
them for protocol compliance but does not keep them in the decoded driver
structures.

> I'm not sure if there is a way to get here with any of those failing.
> Generally for kernel code we defend against this sort of thing at higher levels.
>
> Same applies to all this parser.
>
> Likewise, I'd expect this to only be called on one that exists. This
> defensive stuff costs us in complexity so only do it if needed. Do however
> defend against values that are coming from the device where possible to
> ensure they are consistent.

Addressed in RFC v3. I removed unnecessary internal-only checks and kept the
validation for values that come from the device.

> Why keep it?

Addressed in RFC v3. Reserved values are no longer stored after validation.

> I think you are setting them all, so would be neater as a designated
> inializer:
>         *entry = (struct osf_capbility_entry) {
>                 .sensor_type = get_unaligned_le16(payload),
>                 .sensor_index = get_unaligned_le16(payload + 2),
>         };
>
> Same applies to some of the other structures filled in here.

Addressed in RFC v3 where it made the code clearer. I also reduced the amount
of field-by-field copying where the decoded data did not need to be retained.

> We don't need to match types with original data so
> nice to use the named enums if possible.  Will need
> to check limits though when decoding.

Addressed in RFC v3 by validating decoded values before accepting them from
the device. The decoder now rejects unsupported sensor_type, sample_format, and
channel_count combinations.

> Compared to capabilities this one seems trickier for
> types given it's variable size. Still nice to use something
> that indicates the type if we can.

For the variable sample data I kept the wire-format decoding explicit because
the payload is little-endian data from the device and may not be naturally
aligned.

> Are these not struct osf_capability_entry?
> If they are then use that not a u8.
>
> I'd not bother with a const marking.
> With that type fixed you should be able to use
> __counted_by_ptr(capability_count)  to make that relationship
> clear.

I reviewed this while preparing RFC v3. The capability payload is still decoded
from the wire format, but v3 now validates the capability entry contents more
strictly and avoids keeping reserved fields after validation.

Thanks,

Jinseob