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

[Timur Tabi <ttabi@nvidia.com>]

On Mon, 2026-08-03 at 23:36 +0200, Danilo Krummrich wrote:
> @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.

I frequently struggle to find the right error code in kernel code, especially for situations
where some data structure is not quite right.

> > +        // 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?

Well, maybe this is where we should return ENODATA?

I am expecting future Nova code to handle tags that may be legitimately absent, so maybe having
it return ENODATA if the tag is missing, and EINVAL in all other situations is better.

> > +    /// 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.

Can't you just pick it up from the Rust tree and put it in drm-rust-next?

> 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.

The latter seems simpler to me, and would avoid an annoying merge conflict.