Re: [PATCH v2 2/7] gpu: nova-core: add TLV parser for firmware files

On Tue Jun 30, 2026 at 9:47 PM CEST, Timur Tabi wrote:
> +Tags and Length
> +===============
> +TLV tags are always four-character words, with all letters being upper case.
> +Duplicate tags are not allowed.

Technically this isn't enforced by the constructor.

Maybe a good reason to implement a HashMap? :) Just kidding, I think it should
be good enough to document for the constructor that it doesn't check for
duplicates and that if duplicate tags are present, the first occurrence is used.

> +/// /// Iterator over the [`TlvBlock`]s of a [`Tlv`].

Stray '///'.

> +///
> +/// # Invariants
> +///
> +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the sense
> +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
> +/// `tlv.data.len()` (end of iteration).
> +struct TlvIter<'tlv, 'a> {
> +    tlv: &'tlv Tlv<'a>,
> +    pos: usize,
> +}

[...]

> +#[allow(dead_code)]
> +pub(crate) struct Tlv<'a> {
> +    data: &'a [u8],
> +}
> +
> +#[allow(dead_code)]

Please use expect.

> +impl<'a> Tlv<'a> {

[...]

> +    pub(crate) fn get_string(&self, tag: &[u8; 4]) -> Result<&'a str> {
> +        let tlv = self.find(tag)?;
> +
> +        let bytes = tlv.value;
> +
> +        // To make sure the value actually is a string, make sure it's all ASCII.
> +        if !bytes.is_ascii() {
> +            return Err(EINVAL);
> +        }
> +
> +        core::str::from_utf8(bytes).map_err(|_| EINVAL)

Technically, the error path is unreachable after the is_ascii() check, but I'd
keep it as is anyway.