Message-ID: <Z8j-5HDl3loFNXSn@pollux>
Date: Thu, 6 Mar 2025 02:48:20 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: airlied@...il.com, simona@...ll.ch, corbet@....net,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, ajanulgu@...hat.com, lyude@...hat.com,
	pstanner@...hat.com, zhiw@...dia.com, cjia@...dia.com,
	jhubbard@...dia.com, bskeggs@...dia.com, acurrid@...dia.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...nel.org,
	aliceryhl@...gle.com, tmgross@...ch.edu, gregkh@...uxfoundation.org,
	mcgrof@...nel.org, russ.weight@...ux.dev,
	dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v5 2/5] rust: firmware: introduce
 `firmware::ModInfoBuilder`

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.

Powered by blists - more mailing lists