| Message ID | 20250304173555.2496-3-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial Nova Core series | expand |
On Tue, Mar 04, 2025 at 06:34:49PM +0100, 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. Ditto. > > Co-developed-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Danilo Krummrich <dakr@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()."); > + } > + > + 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. > + pub const fn prepare(self) -> Self { > + self.push_internal(b"\0").prepare_module_name() > + } > + > + /// Build the byte array. > + pub const fn build(self) -> [u8; N] { > + // Add the final NULL terminator. > + let this = self.push_internal(b"\0"); > + > + if this.n == N { > + this.buf > + } else { > + crate::build_error!("Length mismatch."); > + } > + } > +} > + > +impl ModInfoBuilder<0> { > + /// Return the length of the byte array to build. > + pub const fn build_length(self) -> usize { > + // Compensate for the NULL terminator added by `build`. > + self.n + 1 > + } > +} > -- > 2.48.1 > > BR, Jarkko
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@google.com> > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > Signed-off-by: Danilo Krummrich <dakr@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? > + } > + > + 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? --- Cheers, Benno > + pub const fn prepare(self) -> Self { > + self.push_internal(b"\0").prepare_module_name() > + } > + > + /// Build the byte array. > + pub const fn build(self) -> [u8; N] { > + // Add the final NULL terminator. > + let this = self.push_internal(b"\0"); > + > + if this.n == N { > + this.buf > + } else { > + crate::build_error!("Length mismatch."); > + } > + } > +} > + > +impl ModInfoBuilder<0> { > + /// Return the length of the byte array to build. > + pub const fn build_length(self) -> usize { > + // Compensate for the NULL terminator added by `build`. > + self.n + 1 > + } > +} > -- > 2.48.1
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@google.com> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > Signed-off-by: Danilo Krummrich <dakr@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 } }
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 > } > }
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.
On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote: > 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? Abstractly: what `ModInfoBuilder` *does*, concretely: - why the generic constant `N` exists, - what `prepare()` does, - what happens with the `module_name` parameter of `new` - answer the question "I want that the builder outputs the string `???` can it do that? If yes, how do I do it?" >> 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. Sounds good. --- Cheers, Benno
On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote: > On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote: > > 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? > > Abstractly: what `ModInfoBuilder` *does*, concretely: > - why the generic constant `N` exists, It doesn't really matter to the user, since the user never needs to supply it. That happens in the module_firmware! macro. I agree it not good to not mention anything about it at all, but I wouldn't want to bother the user with all implemention details. We can probably just mention that it's used internally and is supplied by module_firmware!. (That module_firmware! does that by doing a dry run of the builder itself, isn't necessary to know for the user I think.) > - what `prepare()` does, Same here, it's an implementation detail not relevant to the user. All the user needs to know is that prepare() acts as a separator to be able to supply the next firmware path. > - what happens with the `module_name` parameter of `new` Should probably just mention it's supplied by module_firmware! and used internally. > - answer the question "I want that the builder outputs the string `???` > can it do that? If yes, how do I do it?" All it does is concatenating multiple &str in const context, which I thought is clear since there are only push() and prepare() as public methods. May it be that your request is more about can we add more hints on the implementation details rather than user focused documentation?
On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote: > On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote: >> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote: >> > 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? >> >> Abstractly: what `ModInfoBuilder` *does*, concretely: >> - why the generic constant `N` exists, > > It doesn't really matter to the user, since the user never needs to supply it. > That happens in the module_firmware! macro. > > I agree it not good to not mention anything about it at all, but I wouldn't want > to bother the user with all implemention details. > > We can probably just mention that it's used internally and is supplied by > module_firmware!. (That module_firmware! does that by doing a dry run of the > builder itself, isn't necessary to know for the user I think.) > >> - what `prepare()` does, > > Same here, it's an implementation detail not relevant to the user. All the user > needs to know is that prepare() acts as a separator to be able to supply the > next firmware path. How about calling it `new_path`/`new_entry` or similar? >> - what happens with the `module_name` parameter of `new` > > Should probably just mention it's supplied by module_firmware! and used > internally. IIUC, that's not the case, the `module_firmware!` macro will call the `create` function with the name and you're supposed to just pass it onto the builder. >> - answer the question "I want that the builder outputs the string `???` >> can it do that? If yes, how do I do it?" > > All it does is concatenating multiple &str in const context, which I thought is > clear since there are only push() and prepare() as public methods. > > May it be that your request is more about can we add more hints on the > implementation details rather than user focused documentation? I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone that uses this API would know what to put into the `.modinfo` section, so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add `firmware` or whatever is needed to make it make sense). And then the question would be how to translate that into the builder. I wouldn't be able to piece it together without looking at the implementation. --- Cheers, Benno
On Thu, Mar 06, 2025 at 01:35:52AM +0000, Benno Lossin wrote: > On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote: > > On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote: > >> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote: > >> > 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? > >> > >> Abstractly: what `ModInfoBuilder` *does*, concretely: > >> - why the generic constant `N` exists, > > > > It doesn't really matter to the user, since the user never needs to supply it. > > That happens in the module_firmware! macro. > > > > I agree it not good to not mention anything about it at all, but I wouldn't want > > to bother the user with all implemention details. > > > > We can probably just mention that it's used internally and is supplied by > > module_firmware!. (That module_firmware! does that by doing a dry run of the > > builder itself, isn't necessary to know for the user I think.) > > > >> - what `prepare()` does, > > > > Same here, it's an implementation detail not relevant to the user. All the user > > needs to know is that prepare() acts as a separator to be able to supply the > > next firmware path. > > How about calling it `new_path`/`new_entry` or similar? Sure, new_entry() sounds good! > > >> - what happens with the `module_name` parameter of `new` > > > > Should probably just mention it's supplied by module_firmware! and used > > internally. > > IIUC, that's not the case, the `module_firmware!` macro will call the > `create` function with the name and you're supposed to just pass it onto > the builder. Yes, but this part is documented by module_firmware!, which I think is the correct place. > > >> - answer the question "I want that the builder outputs the string `???` > >> can it do that? If yes, how do I do it?" > > > > All it does is concatenating multiple &str in const context, which I thought is > > clear since there are only push() and prepare() as public methods. > > > > May it be that your request is more about can we add more hints on the > > implementation details rather than user focused documentation? > > I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone > that uses this API would know what to put into the `.modinfo` section, > so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add > `firmware` or whatever is needed to make it make sense). And then the > question would be how to translate that into the builder. > > I wouldn't be able to piece it together without looking at the > implementation. I believe if you come from the perspective of writing a driver, you reach module_firmware! first and then the subsequent stuff makes sense. But I recognize your feedback and will try to make things a bit more obvious by expanding the example of module_firmware! and expanding a few comments here and there. I also think that s/prepare/new_entry/ will help a lot.
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()."); + } + + 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. + pub const fn prepare(self) -> Self { + self.push_internal(b"\0").prepare_module_name() + } + + /// Build the byte array. + pub const fn build(self) -> [u8; N] { + // Add the final NULL terminator. + let this = self.push_internal(b"\0"); + + if this.n == N { + this.buf + } else { + crate::build_error!("Length mismatch."); + } + } +} + +impl ModInfoBuilder<0> { + /// Return the length of the byte array to build. + pub const fn build_length(self) -> usize { + // Compensate for the NULL terminator added by `build`. + self.n + 1 + } +}