Re: [PATCH v7 3/8] gpu: nova-core: add TLV parser for firmware files

["Danilo Krummrich" <dakr@kernel.org>]

@Miguel: One consideration for you below.

On Fri Jul 31, 2026 at 10:10 PM CEST, Timur Tabi wrote:
> +impl<'a> Tlv<'a> {
> +    const MAGIC: &'static [u8; 4] = b"NVFW";
> +
> +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is malformed.
> +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
> +        // Verify that the magic bytes exist and are the correct value
> +        let magic_len = Self::MAGIC.len();
> +        if data
> +            .get(..magic_len)
> +            .is_none_or(|magic| magic != Self::MAGIC)
> +        {
> +            return Err(EINVAL);
> +        }
> +
> +        // The payload is the contiguous sequence of TLV blocks after the magic.
> +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
> +
> +        if payload.is_empty() {
> +            // Reject empty TLV files
> +            return Err(ENODATA);
> +        }

This seems redundant, if payload is empty the below loop wouldn't execute and
the !has_vers check would return EINVAL. Which seems consistent, since empty
data is kinda invalid.

> +
> +        // The spec says every TLV must have a VERS tag.
> +        let mut has_vers = false;
> +
> +        let mut rest = payload;
> +        while !rest.is_empty() {
> +            // Validate and extract the header (type, length).
> +            let Some(header): Option<TlvBlockHeader> = rest
> +                .get(..TlvBlockHeader::SIZE)
> +                .and_then(TlvBlockHeader::parse)
> +            else {
> +                return Err(EINVAL);
> +            };
> +
> +            has_vers |= header.tag == *b"VERS";
> +
> +            // The `length` field of a TLV block contains the actual byte length of the
> +            // value, but each TLV block is aligned to a 4-byte boundary.
> +            let Some(stored_size) = header.length.checked_next_multiple_of(4) else {
> +                return Err(EINVAL);
> +            };
> +
> +            let length = TlvBlockHeader::SIZE
> +                .checked_add(stored_size)
> +                .ok_or(EINVAL)?;
> +
> +            rest = rest.split_at_checked(length).ok_or(EINVAL)?.1;
> +        }
> +
> +        if !has_vers {
> +            return Err(EINVAL);
> +        }

[...]

> +    fn find(&self, tag: &[u8; 4]) -> Result<TlvBlock<'a>> {
> +        self.iter().find(|b| b.tag == *tag).ok_or(EINVAL)
> +    }

NIT: Do we really know the tag is invalid just because it wasn't found?

> +    /// Return a slice of bytes.  Returns ENODATA if the value is empty.
> +    pub(crate) fn get_bytes(&self, tag: &[u8; 4]) -> Result<&'a [u8]> {
> +        let tlv = self.find(tag)?;
> +
> +        // Treat empty value as an error, to avoid trying to parse nothing.
> +        if tlv.value.is_empty() {
> +            return Err(ENODATA);
> +        }

This one seems resonable; but we don't have the error code in place. The commit
message says the series depends on [1], but this likely goes through the Rust
tree.

Either we just add this single error code in a separate patch or Miguel provides
a signed tag for [1].

Since this is a perfectly trivial conflict, I'd go for the former.

Miguel, let me know if you want to provide a signed tag, otherwise it would be
good if Timur (or myself) could send a patch for ENODATA only.

@Timur: In any case, no need to resend the series for this or the other two nits
in this reply.

Thanks,
Danilo