Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Mar 06, 2025 at 01:27:19AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
> > On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >>
> >> > +#[macro_export]
> >> > +macro_rules! module_firmware {
> >> > +    ($($builder:tt)*) => {
> >>
> >> This should probably be `$builder:expr` instead.
> >
> > That doesn't work, the compiler then complains, since it's not an expression:
> >
> > 193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
> >      |                                                ^^ expected one of `.`, `?`, `]`, or an operator
> 
> Does `<$builder>::create` work (with the `expr` fragment)?

No, the compiler then explicitly complains that it expects a type.

> 
> > `ty` doesn't work either, since then the compiler expects the caller to add the
> > const generic, which we want the macro to figure out instead.
> >
> >>
> >> > +
> >> > +        #[cfg(not(MODULE))]
> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
> >> > +            <LocalModule as kernel::ModuleMetadata>::NAME
> >>
> >> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
> >
> > Good catch, thanks.
> >
> >>
> >> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
> >> the current module for some reason, no idea if there is a rational
> >> argument behind that, but it just doesn't sit right with me.
> >>
> >> Essentially you're doing this for convenience, right? So you don't want
> >> to have to repeat the name of the module type every time?
> >
> > No, it's really that I can't know the type name here, please see the previous
> > patch commit message that introduces `LocalModule` for explanation.
> 
> Gotcha.
> 
> >> > +        }
> >> > +
> >> > +        #[cfg(MODULE)]
> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
> >> > +            kernel::c_str!("")
> >>
> >> Ditto.
> >>
> >> > +        }
> >>
> >> Are these two functions used outside of the `static` below? If no, then
> >> you can just move them into the static? You can also probably use a
> >> `const` instead of a function, that way you only have 4 lines instead
> >> of 8.
> >
> > Is this what you're proposing?
> >
> > 	#[macro_export]
> > 	macro_rules! module_firmware {
> > 	    ($($builder:tt)*) => {
> > 	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> > 	            $crate::c_str!("")
> > 	        } else {
> > 	            <LocalModule as $crate::ModuleMetadata>::NAME
> > 	        };
> >
> > 	        #[link_section = ".modinfo"]
> > 	        #[used]
> > 	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
> > 	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();
> 
> I meant to also move the `const` into the expression, but I guess that
> leads to duplication:
> 
>     #[link_section = ".modinfo"]
>     #[used]
>     static __MODULE_FIRMWARE: [u8; {
>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>             $crate::c_str!("")
>         } else {
>             <LocalModule as $crate::ModuleMetadata>::NAME
>         };
>         <$builder>::create(PREFIX).build_length()
>     }] = {
>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>             $crate::c_str!("")
>         } else {
>             <LocalModule as $crate::ModuleMetadata>::NAME
>         };
>         <$builder>::create(PREFIX)
>     };
> 
> But then the advantage is that only the `__MODULE_FIRMWARE` static will
> be in-scope.
> 
> Do you think that its useful to have the static be accessible? I.e. do
> users need to access it (I would think they don't)? If they don't, then
> we could put all of those things into a `const _: () = { /* ... */ };`.
> But then people can invoke `module_firmware!` multiple times in the same
> module, is that a problem?

Didn't know that's possible (const _; () = { ... };). That's pretty nice, I will
go with my above proposal wrapped into the anonymous const. Thanks.