Danilo Krummrich
2025-Mar-05 22:38 UTC
[PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`
On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote: > > The `firmware` field of the `module!` only accepts literal strings, > > which is due to the fact that it is implemented as a proc macro. > > > > Some drivers require a lot of firmware files (such as nova-core) and > > hence benefit from more flexibility composing firmware path strings. > > > > The `firmware::ModInfoBuilder` is a helper component to flexibly compose > > firmware path strings for the .modinfo section in const context. > > > > It is meant to be used in combination with `kernel::module_firmware!`, > > which is introduced in a subsequent patch. > > > > Co-developed-by: Alice Ryhl <aliceryhl at google.com> > > Signed-off-by: Alice Ryhl <aliceryhl at google.com> > > Signed-off-by: Danilo Krummrich <dakr at kernel.org> > > --- > > rust/kernel/firmware.rs | 98 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 98 insertions(+) > > > > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs > > index c5162fdc95ff..6e6972d94597 100644 > > --- a/rust/kernel/firmware.rs > > +++ b/rust/kernel/firmware.rs > > @@ -115,3 +115,101 @@ unsafe impl Send for Firmware {} > > // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to > > // be used from any thread. > > unsafe impl Sync for Firmware {} > > + > > +/// Builder for firmware module info. > > +/// > > +/// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the > > +/// .modinfo section in const context. > > +/// > > +/// It is meant to be used in combination with [`kernel::module_firmware!`]. > > +/// > > +/// For more details and an example, see [`kernel::module_firmware!`]. > > +pub struct ModInfoBuilder<const N: usize> { > > + buf: [u8; N], > > + n: usize, > > + module_name: &'static CStr, > > +} > > + > > +impl<const N: usize> ModInfoBuilder<N> { > > + /// Create an empty builder instance. > > + pub const fn new(module_name: &'static CStr) -> Self { > > + Self { > > + buf: [0; N], > > + n: 0, > > + module_name, > > + } > > + } > > + > > + const fn push_internal(mut self, bytes: &[u8]) -> Self { > > + let mut j = 0; > > + > > + if N == 0 { > > + self.n += bytes.len(); > > + return self; > > + } > > + > > + while j < bytes.len() { > > + if self.n < N { > > + self.buf[self.n] = bytes[j]; > > + } > > + self.n += 1; > > + j += 1; > > + } > > + self > > + } > > + > > + /// Push an additional path component. > > + /// > > + /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must > > + /// be called before adding path components. > > + pub const fn push(self, s: &str) -> Self { > > + if N != 0 && self.n == 0 { > > + crate::build_error!("Must call prepare() before push()."); > > This will only prevent the first `prepare` call being missed, right?Correct, unfortunately there's no way to detect subsequent ones.> > > + } > > + > > + self.push_internal(s.as_bytes()) > > + } > > + > > + const fn prepare_module_name(self) -> Self { > > + let mut this = self; > > + let module_name = this.module_name; > > + > > + if !this.module_name.is_empty() { > > + this = this.push_internal(module_name.as_bytes_with_nul()); > > + > > + if N != 0 { > > + // Re-use the space taken by the NULL terminator and swap it with the '.' separator. > > + this.buf[this.n - 1] = b'.'; > > + } > > + } > > + > > + this.push_internal(b"firmware=") > > + } > > + > > + /// Prepare for the next module info entry. > > + /// > > + /// Must be called before [`ModInfoBuilder::push`] can be called. > > If you always have to call this before `push`, why not inline it there?You can push() multiple times to compose the firmware path string (which is the whole purpose :). Example from nova-core: pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>); impl<const N: usize> ModInfoBuilder<N> { const fn make_entry_file(self, chipset: &str, fw: &str) -> Self { let version = "535.113.01"; ModInfoBuilder( self.0 .prepare() .push("nvidia/") .push(chipset) .push("/gsp/") .push(fw) .push("-") .push(version) .push(".bin"), ) } const fn make_entry_chipset(self, chipset: &str) -> Self { self.make_entry_file(chipset, "booter_load") .make_entry_file(chipset, "booter_unload") .make_entry_file(chipset, "bootloader") .make_entry_file(chipset, "gsp") } pub(crate) const fn create( module_name: &'static kernel::str::CStr, ) -> firmware::ModInfoBuilder<N> { let mut this = Self(firmware::ModInfoBuilder::new(module_name)); let mut i = 0; while i < gpu::Chipset::NAMES.len() { this = this.make_entry_chipset(gpu::Chipset::NAMES[i]); i += 1; } this.0 } }
Benno Lossin
2025-Mar-05 23:37 UTC
[PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`
On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:> On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote: >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote: >> > + /// Push an additional path component. >> > + /// >> > + /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must >> > + /// be called before adding path components. >> > + pub const fn push(self, s: &str) -> Self { >> > + if N != 0 && self.n == 0 { >> > + crate::build_error!("Must call prepare() before push()."); >> >> This will only prevent the first `prepare` call being missed, right? > > Correct, unfortunately there's no way to detect subsequent ones.Does it make sense to do that one in the constructor? (After looking at the example below) Ah maybe you can't do that, since then you would have two `prepare()` calls for the example below...?>> > + } >> > + >> > + self.push_internal(s.as_bytes()) >> > + } >> > + >> > + const fn prepare_module_name(self) -> Self { >> > + let mut this = self; >> > + let module_name = this.module_name; >> > + >> > + if !this.module_name.is_empty() { >> > + this = this.push_internal(module_name.as_bytes_with_nul()); >> > + >> > + if N != 0 { >> > + // Re-use the space taken by the NULL terminator and swap it with the '.' separator. >> > + this.buf[this.n - 1] = b'.'; >> > + } >> > + } >> > + >> > + this.push_internal(b"firmware=") >> > + } >> > + >> > + /// Prepare for the next module info entry. >> > + /// >> > + /// Must be called before [`ModInfoBuilder::push`] can be called. >> >> If you always have to call this before `push`, why not inline it there? > > You can push() multiple times to compose the firmware path string (which is the > whole purpose :).Ah I see, I only looked at the example you have in the next patch. All in all, I think this patch could use some better documentation, since I had to read a lot of the code to understand what everything is supposed to do... It might also make sense to make this more generic, since one probably also needs this in other places? Or do you think this will only be required for modinfo? --- Cheers, Benno> Example from nova-core: > > pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>); > > impl<const N: usize> ModInfoBuilder<N> { > const fn make_entry_file(self, chipset: &str, fw: &str) -> Self { > let version = "535.113.01"; > > ModInfoBuilder( > self.0 > .prepare() > .push("nvidia/") > .push(chipset) > .push("/gsp/") > .push(fw) > .push("-") > .push(version) > .push(".bin"), > ) > } > > const fn make_entry_chipset(self, chipset: &str) -> Self { > self.make_entry_file(chipset, "booter_load") > .make_entry_file(chipset, "booter_unload") > .make_entry_file(chipset, "bootloader") > .make_entry_file(chipset, "gsp") > } > > pub(crate) const fn create( > module_name: &'static kernel::str::CStr, > ) -> firmware::ModInfoBuilder<N> { > let mut this = Self(firmware::ModInfoBuilder::new(module_name)); > let mut i = 0; > > while i < gpu::Chipset::NAMES.len() { > this = this.make_entry_chipset(gpu::Chipset::NAMES[i]); > i += 1; > } > > this.0 > } > }
Danilo Krummrich
2025-Mar-05 23:57 UTC
[PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`
On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote: > > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote: > >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote: > >> > + /// Push an additional path component. > >> > + /// > >> > + /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must > >> > + /// be called before adding path components. > >> > + pub const fn push(self, s: &str) -> Self { > >> > + if N != 0 && self.n == 0 { > >> > + crate::build_error!("Must call prepare() before push()."); > >> > >> This will only prevent the first `prepare` call being missed, right? > > > > Correct, unfortunately there's no way to detect subsequent ones. > > Does it make sense to do that one in the constructor? > > (After looking at the example below) Ah maybe you can't do that, since > then you would have two `prepare()` calls for the example below...?Exactly.> >> If you always have to call this before `push`, why not inline it there? > > > > You can push() multiple times to compose the firmware path string (which is the > > whole purpose :). > > Ah I see, I only looked at the example you have in the next patch. All > in all, I think this patch could use some better documentation, since I > had to read a lot of the code to understand what everything is supposed > to do...I can expand the example in module_firmware! to make things a bit more obvious. Otherwise, what information do you think is missing?> > It might also make sense to make this more generic, since one probably > also needs this in other places? Or do you think this will only be > required for modinfo?Currently, I don't think there's any more need for a generic const string builder. For now, I think we're good. Let's factor it out, once we have actual need for that.