Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Benno Lossin <benno.lossin@proton.me>]

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
> 	    }
> 	}