[net-next,v9,2/4] rust: net::phy add module_phy_driver macro

Commit Message

FUJITA Tomonori Dec. 5, 2023, 1:14 a.m. UTC

This macro creates an array of kernel's `struct phy_driver` and
registers it. This also corresponds to the kernel's
`MODULE_DEVICE_TABLE` macro, which embeds the information for module
loading into the module binary file.

A PHY driver should use this macro.

Signed-off-by: FUJITA Tomonori <[e-mail]>
Reviewed-by: Alice Ryhl <[e-mail]>
---
 rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

Comments

Jarkko Sakkinen Dec. 5, 2023, 1:48 a.m. UTC | #1

On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
> This macro creates an array of kernel's `struct phy_driver` and
> registers it. This also corresponds to the kernel's
> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
> loading into the module binary file.
>
> A PHY driver should use this macro.
>
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> Reviewed-by: Alice Ryhl <[e-mail]>
> ---
>  rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 5d220187eec9..d9cec139324a 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -752,3 +752,149 @@ const fn as_int(&self) -> u32 {
>          }
>      }
>  }
> +
> +/// Declares a kernel module for PHYs drivers.
> +///
> +/// This creates a static array of kernel's `struct phy_driver` and registers it.

s/This creates a static array/Creates a static array/

Suggestion for better formulation:

"Creates a static array of `struct phy driver` instances, and registers them.""

> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.

s/This/`kernel::module_phy_driver`/

Or at least I did not see it introduced earlier in the text.

BR, Jarkko

FUJITA Tomonori Dec. 5, 2023, 3:44 a.m. UTC | #2

On Tue, 05 Dec 2023 03:48:32 +0200
"Jarkko Sakkinen" <[e-mail]> wrote:

> On Tue Dec 5, 2023 at 3:14 AM EET, FUJITA Tomonori wrote:
>> This macro creates an array of kernel's `struct phy_driver` and
>> registers it. This also corresponds to the kernel's
>> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
>> loading into the module binary file.
>>
>> A PHY driver should use this macro.
>>
>> Signed-off-by: FUJITA Tomonori <[e-mail]>
>> Reviewed-by: Alice Ryhl <[e-mail]>
>> ---
>>  rust/kernel/net/phy.rs | 146 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 146 insertions(+)
>>
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 5d220187eec9..d9cec139324a 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> @@ -752,3 +752,149 @@ const fn as_int(&self) -> u32 {
>>          }
>>      }
>>  }
>> +
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of kernel's `struct phy_driver` and registers it.
> 
> s/This creates a static array/Creates a static array/
> 
> Suggestion for better formulation:
> 
> "Creates a static array of `struct phy driver` instances, and registers them.""

I follow Rust comment style here. Maybe I should do:

s/This creates/This macro creates/

>> +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
>> +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
> 
> s/This/`kernel::module_phy_driver`/
> 
> Or at least I did not see it introduced earlier in the text.

s/This/This macro/ works for you?

Likely `kernel::` part would be changed in the future.

Miguel Ojeda Dec. 8, 2023, 2:42 p.m. UTC | #3

On Tue, Dec 5, 2023 at 4:44 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> I follow Rust comment style here. Maybe I should do:
>
> s/This creates/This macro creates/

I think "Creates..." is fine, it is what we do in other files for functions.

> Likely `kernel::` part would be changed in the future.

Similarly, you could say "Corresponds...".

Cheers,
Miguel

Benno Lossin Dec. 8, 2023, 6:11 p.m. UTC | #4

On 12/5/23 02:14, FUJITA Tomonori wrote:
> This macro creates an array of kernel's `struct phy_driver` and
> registers it. This also corresponds to the kernel's
> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
> loading into the module binary file.
> 
> A PHY driver should use this macro.
> 
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> Reviewed-by: Alice Ryhl <[e-mail]>

Reviewed-by: Benno Lossin <[e-mail]>

Patch

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 5d220187eec9..d9cec139324a 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -752,3 +752,149 @@  const fn as_int(&self) -> u32 {
         }
     }
 }
+
+/// Declares a kernel module for PHYs drivers.
+///
+/// This creates a static array of kernel's `struct phy_driver` and registers it.
+/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
+/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
+///
+/// # Examples
+///
+/// ```
+/// # mod module_phy_driver_sample {
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// kernel::module_phy_driver! {
+///     drivers: [PhySample],
+///     device_table: [
+///         DeviceId::new_with_driver::<PhySample>()
+///     ],
+///     name: "rust_sample_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust sample PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhySample;
+///
+/// #[vtable]
+/// impl phy::Driver for PhySample {
+///     const NAME: &'static CStr = c_str!("PhySample");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001);
+/// }
+/// # }
+/// ```
+///
+/// This expands to the following code:
+///
+/// ```ignore
+/// use kernel::c_str;
+/// use kernel::net::phy::{self, DeviceId};
+/// use kernel::prelude::*;
+///
+/// struct Module {
+///     _reg: ::kernel::net::phy::Registration,
+/// }
+///
+/// module! {
+///     type: Module,
+///     name: "rust_sample_phy",
+///     author: "Rust for Linux Contributors",
+///     description: "Rust sample PHYs driver",
+///     license: "GPL",
+/// }
+///
+/// struct PhySample;
+///
+/// #[vtable]
+/// impl phy::Driver for PhySample {
+///     const NAME: &'static CStr = c_str!("PhySample");
+///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x00000001);
+/// }
+///
+/// const _: () = {
+///     static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] =
+///         [::kernel::net::phy::create_phy_driver::<PhySample>()];
+///
+///     impl ::kernel::Module for Module {
+///         fn init(module: &'static ThisModule) -> Result<Self> {
+///             let drivers = unsafe { &mut DRIVERS };
+///             let mut reg = ::kernel::net::phy::Registration::register(
+///                 module,
+///                 ::core::pin::Pin::static_mut(drivers),
+///             )?;
+///             Ok(Module { _reg: reg })
+///         }
+///     }
+/// };
+///
+/// #[cfg(MODULE)]
+/// #[no_mangle]
+/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0x00000001,
+///         phy_id_mask: 0xffffffff,
+///     },
+///     ::kernel::bindings::mdio_device_id {
+///         phy_id: 0,
+///         phy_id_mask: 0,
+///     },
+/// ];
+/// ```
+#[macro_export]
+macro_rules! module_phy_driver {
+    (@replace_expr $_t:tt $sub:expr) => {$sub};
+
+    (@count_devices $($x:expr),*) => {
+        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
+    };
+
+    (@device_table [$($dev:expr),+]) => {
+        // SAFETY: C will not read off the end of this constant since the last element is zero.
+        #[cfg(MODULE)]
+        #[no_mangle]
+        static __mod_mdio__phydev_device_table: [$crate::bindings::mdio_device_id;
+            $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [
+            $($dev.mdio_device_id()),+,
+            $crate::bindings::mdio_device_id {
+                phy_id: 0,
+                phy_id_mask: 0
+            }
+        ];
+    };
+
+    (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {
+        struct Module {
+            _reg: $crate::net::phy::Registration,
+        }
+
+        $crate::prelude::module! {
+            type: Module,
+            $($f)*
+        }
+
+        const _: () = {
+            static mut DRIVERS: [$crate::net::phy::DriverVTable;
+                $crate::module_phy_driver!(@count_devices $($driver),+)] =
+                [$($crate::net::phy::create_phy_driver::<$driver>()),+];
+
+            impl $crate::Module for Module {
+                fn init(module: &'static 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 };
+                    let mut reg = $crate::net::phy::Registration::register(
+                        module,
+                        ::core::pin::Pin::static_mut(drivers),
+                    )?;
+                    Ok(Module { _reg: reg })
+                }
+            }
+        };
+
+        $crate::module_phy_driver!(@device_table [$($dev),+]);
+    }
+}