Re: [PATCH 1/2] soc: qcom: qmi: Fix "invalid data length" in encoder

[Bjorn Andersson]

On Thu, Feb 19, 2026 at 01:25:36PM -0800, Jeff Johnson wrote:
> On 2/19/2026 11:18 AM, Bjorn Andersson wrote:
> > I reviewed the downstream code generator source and documentation.
> > 
> > We do generate tables matching the ath12k c-structures, i.e. variable
> > length arrays are always prefixed with an uint32_t field - not a
> > uint8_t or uint16_t based on elem_size.
> > 
> > Looking back at the original implementation of the in-kernel
> > qmi_encode(), we only read elem_size bytes from the c-structure, but we
> > do so into the (little-endian) uint32_t on the stack, from which we
> > encode the message and act upon the result.
> > 
> > In qmi_decode() we decode elem_size bytes from the message into the
> > (little endian) uint32_t and then write 4 bytes to the c-structure.
> > 
> > 
> > The fix would as such seem to be to just update the length fields to be
> > all uint32_t. The problem I see with this is that qmic [1] is the only
> > publicly available code generator, and if we change it to always
> > generate uint32_t length members, we also need to fix the
> > encoder/decoder in libqrtr [2] - which will be an ABI breaking change.
> 
> And IMO that is a deal breaker since it would break the interface with all
> existing legacy firmware.
> 

No, the firwmare-facing encoded length in the messages are currently all
little-endian elem_sized, and this would be unchanged. It's merely a
question about the ABI between code generator, encoder/decoder, and the
client code.

> > 
> > If we go the other way around, the drawback is that we no longer support
> > the c-structures generated by the proprietary code generator.
> > 
> > Worth pointing out is that the structure of the c-code is an ABI between
> > the encoder/decoder, the code generator and the client - it does not
> > affect the wire format.
> > 
> > [1] https://github.com/linux-msm/qmic
> > [2] https://github.com/linux-msm/qrtr
> 
> Going back to the original implementation that reads and writes a u32 on the
> stack, can we stick with that but add endian logic that correctly converts
> between u32 host endian on the stack and either u8 or u16 little endian in the
> messages? Is this specific to QMI_DATA_LEN TLVs?
> 

I gave it some more thought, and discussed a bit with Chris Lew.

If we change qmic to produce uint32_t length entries and align the
in-kernel interfaces to use 32-bit lengths the kernel works fine - and
thanks to Alexander's work, should support both endianes (will double
check).

For the userspace library, the decoder already writes 32-bit fields
(into the u8/u16...) so the situation for currently generated c-structs
will be unchanged and it will be correct for 32-bit fields.

The encoder is reading u8/u16 from the c-struct and encodes this. Just
as with the structs in athNNk, we can change them to 32-bit without
impacting the encoder; as long as we don't change the encoder...
The only problem I think we have left is that we can't fix the userspace
encoder - as this would be incompatible with current clients (u8/u16
can't be read as u32).


I'll take another pass tomorrow, to review this, review Alexander's work
once more, and prepare some patches.

Regards,
Bjorn