On Wed Oct 7, 2026 at 9:05 PM JST, Alexandre Courbot wrote:
> Sorry, some follow-up comments after my previous review.
>
> On Mon Sep 28, 2026 at 5:42 PM JST, Eliot Courtney wrote:
>> Similar to the typed encoding layer, add some decoding type machinery.
>> Add a simple macro `nvkv_decode!` which implements `Schema` for a struct
>> by composing visit calls to each member. Add some common `Schema` kinds,
>> such as `Array` which collects an array value into a fixed maximum size
>> array, and `Required` which fails a decode if the value is not sent.
>>
>> Signed-off-by: Eliot Courtney <[email protected]>
>> ---
>> drivers/gpu/nova-core/gsp/nvkv.rs | 11 +-
>> drivers/gpu/nova-core/gsp/nvkv/decode.rs | 622
>> ++++++++++++++++++++++++++++++-
>> 2 files changed, 628 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs
>> b/drivers/gpu/nova-core/gsp/nvkv.rs
>> index 7ac3a459a98b..5791df07a7fa 100644
>> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
>> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
>> @@ -9,7 +9,7 @@
>> //! function calls will map to some struct - for example,
>> f(GPU_NAME_STRING_KEY, 0, b"some gpu")
>> //! naturally maps to storing a &str with the GPU name.
>>
>> -#![expect(unused_imports)]
>> +#![cfg_attr(not(CONFIG_KUNIT), expect(unused_imports))]
>> #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>>
>> use core::{
>> @@ -23,7 +23,8 @@
>> use kernel::{
>> alloc::{
>> allocator::KVmalloc,
>> - Allocator, //
>> + Allocator,
>> + ArrayVec, //
>> },
>> bitfield,
>> num::Bounded,
>> @@ -148,6 +149,12 @@ fn default() -> Self {
>> }
>> }
>>
>> +/// A schema field for an array value under the NVKV key `KEY_ID`.
>> +#[repr(transparent)]
>
> I see that `#[repr(transparent)]` is used several times in this series,
> but is there a need for it? Same for the many `#[inline]`s, here I feel
> like letting the compiler arrange things as it wants might be the better
> call. I know I suggested downgrading from always-inline to just inline,
> so maybe we should go all the way here. These are very likely to be
> inlined anyway, and worst case I don't think a function call would
> induce a big cost.
It's not required. I was adding that because I was thinking of these
types semantically as just being type-level only wrappers of the
underlying data. I'll remove them, plus the inlines.
>
> <...>
>> +impl<T: Default, const KEY_ID: KeyId> Schema for Key<T, KEY_ID> {
>> + type Target = T;
>> +
>> + #[inline]
>> + fn init() -> impl Init<Self> {
>> + Self::default()
>> + }
>> +
>> + #[inline]
>> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> + Ok(core::mem::take(&mut self.0))
>> + }
>> +}
>
> Both methods return the built value on the stack, which is fine when we
> only deal with scalars but technically we could also store larger
> values. How about a `const_assert!` ensuring we don't go beyond, say, 32
> bytes for the size of `T` and `Target`?
Will so do!
>
> <...>
>> +/// A schema field for a key that must be present.
>> +///
>> +/// `finish` fails with `EINVAL` if no value arrived for the key.
>> +#[repr(transparent)]
>> +pub(crate) struct Required<T, const KEY_ID: KeyId>(Key<Option<T>, KEY_ID>);
>> +
>> +impl<T, const KEY_ID: KeyId> Schema for Required<T, KEY_ID> {
>> + type Target = T;
>> +
>> + #[inline]
>> + fn init() -> impl Init<Self> {
>> + Self(None.into())
>> + }
>> +
>> + #[inline]
>> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> + (self.0).0.take().ok_or(EINVAL)
>
> I've experimented a bit with my earlier suggestion of having a wrapping
> `Required` type because I wasn't so sure it would work, but it seems to
> be indeed doable! Here is my draft implementation:
>
> pub(crate) struct Required<S: Schema> {
> inner: S,
> parsed: bool,
> }
>
> impl<S: Schema> Schema for Required<S> {
> type Target = S::Target;
>
> fn init() -> impl Init<Self> {
> init!(Self { inner <- S::init(), parsed: false })
> }
>
> fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> // Reset `self.parsed` to make the schema empty again per the method
> contract.
> let parsed = core::mem::take(&mut self.parsed);
> self.inner
> .finish()
> .chain(move |_| if parsed { Ok(()) } else { Err(EINVAL) })
> }
> }
>
> impl<'data, S: Schema + Visit<'data>> Visit<'data> for Required<S> {
> fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'data>)
> -> Result<bool> {
> let consumed = self.inner.visit(key, index, value)?;
> self.parsed |= consumed;
> Ok(consumed)
> }
> }
>
> You need to convert all the `Required<T, ...>` into `Required<Key<T,
> ...>>`, but this reads more logically I think, and now you can combine
> `Required` with more types. I believe you could also implement
> `Optional` in a similar way.
>
> (you will also need to add a `#[derive(Default)]` to `FbRegionFlags` on
> the next patch, but that's not a big deal since it wraps a primitive
> type anyway)
Thanks for prototyping this, will add this in.
