[PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[Rahul Rameshbabu]

Similar to the use of $crate::Module, ThisModule should be referred to as
$crate::ThisModule in the macro evaluation. The reason the macro previously
did not cause any errors is because all the users of the macro would use
kernel::prelude::*, bringing ThisModule into scope.

Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---

Notes:
    How I came up with this change:
    
    I was working on my own rust bindings and rust driver when I compared my
    macro_rule to the one used for module_phy_driver. I noticed, if I made a
    driver that does not use kernel::prelude::*, that the ThisModule type
    identifier used in the macro would cause an error without being scoped in
    the macro_rule. I believe the correct implementation for the macro is one
    where the types used are correctly expanded with needed scopes.

 rust/kernel/net/phy.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 910ce867480a..80f9f571b88c 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -837,7 +837,7 @@ const fn as_int(&self) -> u32 {
 ///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
 ///
 ///     impl ::kernel::Module for Module {
-///         fn init(module: &'static ThisModule) -> Result<Self> {
+///         fn init(module: &'static ::kernel::ThisModule) -> Result<Self> {
 ///             let drivers = unsafe { &mut DRIVERS };
 ///             let mut reg = ::kernel::net::phy::Registration::register(
 ///                 module,
@@ -899,7 +899,7 @@ struct Module {
                 [$($crate::net::phy::create_phy_driver::<$driver>()),+];
 
             impl $crate::Module for Module {
-                fn init(module: &'static ThisModule) -> Result<Self> {
+                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
                     // SAFETY: The anonymous constant guarantees that nobody else can access
                     // the `DRIVERS` static. The array is used only in the C side.
                     let drivers = unsafe { &mut DRIVERS };

base-commit: 73af53d82076bbe184d9ece9e14b0dc8599e6055
-- 
2.44.1

Re: [PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[Jakub Kicinski]

On Wed, 13 Nov 2024 17:45:22 +0000 Rahul Rameshbabu wrote:
> Similar to the use of $crate::Module, ThisModule should be referred to as
> $crate::ThisModule in the macro evaluation. The reason the macro previously
> did not cause any errors is because all the users of the macro would use
> kernel::prelude::*, bringing ThisModule into scope.

You say "previously", does it mean there are no in-tree users where
this could cause bugs? If so no Fixes tag necessary..

> Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
> 
> Notes:
>     How I came up with this change:
>     
>     I was working on my own rust bindings and rust driver when I compared my
>     macro_rule to the one used for module_phy_driver. I noticed, if I made a
>     driver that does not use kernel::prelude::*, that the ThisModule type
>     identifier used in the macro would cause an error without being scoped in
>     the macro_rule. I believe the correct implementation for the macro is one
>     where the types used are correctly expanded with needed scopes.

Rust experts, does the patch itself make sense?

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 910ce867480a..80f9f571b88c 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -837,7 +837,7 @@ const fn as_int(&self) -> u32 {
>  ///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
>  ///
>  ///     impl ::kernel::Module for Module {
> -///         fn init(module: &'static ThisModule) -> Result<Self> {
> +///         fn init(module: &'static ::kernel::ThisModule) -> Result<Self> {
>  ///             let drivers = unsafe { &mut DRIVERS };
>  ///             let mut reg = ::kernel::net::phy::Registration::register(
>  ///                 module,
> @@ -899,7 +899,7 @@ struct Module {
>                  [$($crate::net::phy::create_phy_driver::<$driver>()),+];
>  
>              impl $crate::Module for Module {
> -                fn init(module: &'static ThisModule) -> Result<Self> {
> +                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
>                      // SAFETY: The anonymous constant guarantees that nobody else can access
>                      // the `DRIVERS` static. The array is used only in the C side.
>                      let drivers = unsafe { &mut DRIVERS };
> 
> base-commit: 73af53d82076bbe184d9ece9e14b0dc8599e6055

Re: [PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[FUJITA Tomonori]

On Sun, 24 Nov 2024 16:27:00 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

>> Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>> ---
>> 
>> Notes:
>>     How I came up with this change:
>>     
>>     I was working on my own rust bindings and rust driver when I compared my
>>     macro_rule to the one used for module_phy_driver. I noticed, if I made a
>>     driver that does not use kernel::prelude::*, that the ThisModule type
>>     identifier used in the macro would cause an error without being scoped in
>>     the macro_rule. I believe the correct implementation for the macro is one
>>     where the types used are correctly expanded with needed scopes.
> 
> Rust experts, does the patch itself make sense?

Looks good to me.

Re: [PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[Alice Ryhl]

On Mon, Nov 25, 2024 at 1:27 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 13 Nov 2024 17:45:22 +0000 Rahul Rameshbabu wrote:
> > Similar to the use of $crate::Module, ThisModule should be referred to as
> > $crate::ThisModule in the macro evaluation. The reason the macro previously
> > did not cause any errors is because all the users of the macro would use
> > kernel::prelude::*, bringing ThisModule into scope.
>
> You say "previously", does it mean there are no in-tree users where
> this could cause bugs? If so no Fixes tag necessary..
>
> > Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
> > Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> > ---
> >
> > Notes:
> >     How I came up with this change:
> >
> >     I was working on my own rust bindings and rust driver when I compared my
> >     macro_rule to the one used for module_phy_driver. I noticed, if I made a
> >     driver that does not use kernel::prelude::*, that the ThisModule type
> >     identifier used in the macro would cause an error without being scoped in
> >     the macro_rule. I believe the correct implementation for the macro is one
> >     where the types used are correctly expanded with needed scopes.
>
> Rust experts, does the patch itself make sense?

Yes, the macro should not rely on the user having random things in
scope when calling the macro. This change is good.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> > index 910ce867480a..80f9f571b88c 100644
> > --- a/rust/kernel/net/phy.rs
> > +++ b/rust/kernel/net/phy.rs
> > @@ -837,7 +837,7 @@ const fn as_int(&self) -> u32 {
> >  ///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
> >  ///
> >  ///     impl ::kernel::Module for Module {
> > -///         fn init(module: &'static ThisModule) -> Result<Self> {
> > +///         fn init(module: &'static ::kernel::ThisModule) -> Result<Self> {
> >  ///             let drivers = unsafe { &mut DRIVERS };
> >  ///             let mut reg = ::kernel::net::phy::Registration::register(
> >  ///                 module,
> > @@ -899,7 +899,7 @@ struct Module {
> >                  [$($crate::net::phy::create_phy_driver::<$driver>()),+];
> >
> >              impl $crate::Module for Module {
> > -                fn init(module: &'static ThisModule) -> Result<Self> {
> > +                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
> >                      // SAFETY: The anonymous constant guarantees that nobody else can access
> >                      // the `DRIVERS` static. The array is used only in the C side.
> >                      let drivers = unsafe { &mut DRIVERS };
> >
> > base-commit: 73af53d82076bbe184d9ece9e14b0dc8599e6055
>

Re: [PATCH net] rust: net::phy scope ThisModule usage in the module_phy_driver macro

[Paolo Abeni]

On 11/13/24 18:45, Rahul Rameshbabu wrote:
> Similar to the use of $crate::Module, ThisModule should be referred to as
> $crate::ThisModule in the macro evaluation. The reason the macro previously
> did not cause any errors is because all the users of the macro would use
> kernel::prelude::*, bringing ThisModule into scope.
> 
> Fixes: 2fe11d5ab35d ("rust: net::phy add module_phy_driver macro")
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
> 
> Notes:
>     How I came up with this change:
>     
>     I was working on my own rust bindings and rust driver when I compared my
>     macro_rule to the one used for module_phy_driver. I noticed, if I made a
>     driver that does not use kernel::prelude::*, that the ThisModule type
>     identifier used in the macro would cause an error without being scoped in
>     the macro_rule. I believe the correct implementation for the macro is one
>     where the types used are correctly expanded with needed scopes.

As noted by Jakub, since this apparently does not address a problem
existing in the current tree, but cleans-up the implementation for
future usage, I suggest to target the net-next without a fixes tag.

Note that net-next is currently closed for the merge window and will
re-open around Dec 2.

Thanks,

Paolo