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.
<...>
> +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`?
<...>
> +/// 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)
