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

@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] =3D b"NVFW";
> +
> +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if t=
he 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 =3D Self::MAGIC.len();
> +        if data
> +            .get(..magic_len)
> +            .is_none_or(|magic| magic !=3D Self::MAGIC)
> +        {
> +            return Err(EINVAL);
> +        }
> +
> +        // The payload is the contiguous sequence of TLV blocks after th=
e magic.
> +        let payload =3D 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 a=
nd
the !has_vers check would return EINVAL. Which seems consistent, since empt=
y
data is kinda invalid.

> +
> +        // The spec says every TLV must have a VERS tag.
> +        let mut has_vers =3D false;
> +
> +        let mut rest =3D payload;
> +        while !rest.is_empty() {
> +            // Validate and extract the header (type, length).
> +            let Some(header): Option<TlvBlockHeader> =3D rest
> +                .get(..TlvBlockHeader::SIZE)
> +                .and_then(TlvBlockHeader::parse)
> +            else {
> +                return Err(EINVAL);
> +            };
> +
> +            has_vers |=3D header.tag =3D=3D *b"VERS";
> +
> +            // The `length` field of a TLV block contains the actual byt=
e length of the
> +            // value, but each TLV block is aligned to a 4-byte boundary=
.
> +            let Some(stored_size) =3D header.length.checked_next_multipl=
e_of(4) else {
> +                return Err(EINVAL);
> +            };
> +
> +            let length =3D TlvBlockHeader::SIZE
> +                .checked_add(stored_size)
> +                .ok_or(EINVAL)?;
> +
> +            rest =3D 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 =3D=3D *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 =3D self.find(tag)?;
> +
> +        // Treat empty value as an error, to avoid trying to parse nothi=
ng.
> +        if tlv.value.is_empty() {
> +            return Err(ENODATA);
> +        }

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

Either we just add this single error code in a separate patch or Miguel pro=
vides
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