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

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

On Tue Aug 4, 2026 at 12:05 AM CEST, Timur Tabi wrote:
> 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:
>> > +    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?

Or maybe just ENOENT.

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

In this case it makes sense to differenciate; AFAIC this can be a follow-up
patch.

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

This is what I refer to below as "signed tag".

Some background on this: We can't have patches as duplicate commits going to
Linus. So in order for me to pull from the Rust tree, it has to guarantee to not
change its histroy (which it does not). Besides that, we don't want all the
commits from another tree, but just a specific one. The normal process for this
is to put the commit on a separate branch based on a commit base, e.g. some -rc,
typically -rc1. Then we can create a signed tag from this that can be merged
into both (or multiple) trees, so the commit only exists exactly once.

However, this kind of logistics typically only makes sense for features, etc. In
order to avoid a trivial conflict, it is usually overkill. Which is also why I
said I'd go for the former.

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