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

[Benno Lossin]

On Thu Mar 6, 2025 at 2:38 AM CET, Danilo Krummrich wrote:
> 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.

Aw well, can't have em all... Probably would be useful if you add a
comment saying that `expr` and `ty` can't be used.

>> > `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.

Sounds good.

---
Cheers,
Benno