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

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.
>>=20
>> On Fri Jul 31, 2026 at 10:10 PM CEST, Timur Tabi wrote:
>> > +=C2=A0=C2=A0=C2=A0 fn find(&self, tag: &[u8; 4]) -> Result<TlvBlock<'=
a>> {
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 self.iter().find(|b| b.tag=
 =3D=3D *tag).ok_or(EINVAL)
>> > +=C2=A0=C2=A0=C2=A0 }
>>=20
>> 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 a=
bsent, so maybe having
> it return ENODATA if the tag is missing, and EINVAL in all other situatio=
ns is better.

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

>> > +=C2=A0=C2=A0=C2=A0 /// Return a slice of bytes.=C2=A0 Returns ENODATA=
 if the value is empty.
>> > +=C2=A0=C2=A0=C2=A0 pub(crate) fn get_bytes(&self, tag: &[u8; 4]) -> R=
esult<&'a [u8]> {
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 let tlv =3D self.find(tag)=
?;
>> > +
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Treat empty value as an=
 error, to avoid trying to parse nothing.
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if tlv.value.is_empty() {
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 re=
turn Err(ENODATA);
>> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>>=20
>> 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 t=
o
Linus. So in order for me to pull from the Rust tree, it has to guarantee t=
o 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 merge=
d
into both (or multiple) trees, so the commit only exists exactly once.

However, this kind of logistics typically only makes sense for features, et=
c. In
order to avoid a trivial conflict, it is usually overkill. Which is also wh=
y 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].
>>=20
>> 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 conflic=
t.