Nouveau - Mar 2025 - [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

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.