| Message ID | 20231024005842.1059620-3-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
On 24.10.23 02:58, 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 <fujita.tomonori@gmail.com> > --- > rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > index 2d821c2475e1..f346b2b4d3cb 100644 > --- a/rust/kernel/net/phy.rs > +++ b/rust/kernel/net/phy.rs > @@ -706,3 +706,132 @@ 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 > +/// > +/// ```ignore Is this example ignored, because it does not compile? I think Wedson was wrapping his example with `module!` inside of a module, so maybe try that? > +/// use kernel::c_str; > +/// use kernel::net::phy::{self, DeviceId}; > +/// use kernel::prelude::*; > +/// > +/// kernel::module_phy_driver! { > +/// drivers: [PhyAX88772A], > +/// device_table: [ > +/// DeviceId::new_with_driver::<PhyAX88772A>(), > +/// ], > +/// name: "rust_asix_phy", > +/// author: "Rust for Linux Contributors", > +/// description: "Rust Asix PHYs driver", > +/// license: "GPL", > +/// } > +/// > +/// struct PhyAX88772A; > +/// > +/// impl phy::Driver for PhyAX88772A { > +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); > +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); > +/// } > +/// ``` > +/// > +/// 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_asix_phy", > +/// author: "Rust for Linux Contributors", > +/// description: "Rust Asix PHYs driver", > +/// license: "GPL", > +/// } > +/// > +/// const _: () = { > +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ > +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), > +/// ]; > +/// > +/// impl ::kernel::Module for Module { > +/// fn init(module: &'static ThisModule) -> Result<Self> { > +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; Can you please format this as well? > +/// Ok(Module { _reg: reg }) > +/// } > +/// } > +/// } > +/// > +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ > +/// ::kernel::bindings::mdio_device_id { > +/// phy_id: 0x003b1861, > +/// 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),+]) => { > + #[no_mangle] > + static __mod_mdio__phydev_device_table: [ > + ::kernel::bindings::mdio_device_id; > + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 > + ] = [ > + $($dev.mdio_device_id()),+, > + ::kernel::bindings::mdio_device_id { > + phy_id: 0, > + phy_id_mask: 0 > + } > + ]; > + }; > + > + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { > + struct Module { > + _reg: ::kernel::net::phy::Registration, > + } > + > + $crate::prelude::module! { > + type: Module, > + $($f)* > + } > + > + const _: () = { > + static mut DRIVERS: [ > + ::kernel::net::phy::DriverVTable; > + $crate::module_phy_driver!(@count_devices $($driver),+) > + ] = [ > + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ > + ]; > + > + impl ::kernel::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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; Can you put the safe operations outside of the `unsafe` block? Since `rustfmt` does not work well within `macro_rules`, you have to manually format this code. One trick I use to do this is to replace all meta variables with normal variables and just put it in `rustfmt` and then revert the replacement in the output.
On Tue, 24 Oct 2023 16:28:02 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 24.10.23 02:58, 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 <fujita.tomonori@gmail.com> >> --- >> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 129 insertions(+) >> >> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >> index 2d821c2475e1..f346b2b4d3cb 100644 >> --- a/rust/kernel/net/phy.rs >> +++ b/rust/kernel/net/phy.rs >> @@ -706,3 +706,132 @@ 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 >> +/// >> +/// ```ignore > > Is this example ignored, because it does not compile? The old version can't be compiled but the current version can so I'll drop ignore. > I think Wedson was wrapping his example with `module!` inside > of a module, so maybe try that? I'm not sure what you mean. >> +/// use kernel::c_str; >> +/// use kernel::net::phy::{self, DeviceId}; >> +/// use kernel::prelude::*; >> +/// >> +/// kernel::module_phy_driver! { >> +/// drivers: [PhyAX88772A], >> +/// device_table: [ >> +/// DeviceId::new_with_driver::<PhyAX88772A>(), >> +/// ], >> +/// name: "rust_asix_phy", >> +/// author: "Rust for Linux Contributors", >> +/// description: "Rust Asix PHYs driver", >> +/// license: "GPL", >> +/// } >> +/// >> +/// struct PhyAX88772A; >> +/// >> +/// impl phy::Driver for PhyAX88772A { >> +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); >> +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); >> +/// } >> +/// ``` >> +/// >> +/// 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_asix_phy", >> +/// author: "Rust for Linux Contributors", >> +/// description: "Rust Asix PHYs driver", >> +/// license: "GPL", >> +/// } >> +/// >> +/// const _: () = { >> +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ >> +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), >> +/// ]; >> +/// >> +/// impl ::kernel::Module for Module { >> +/// fn init(module: &'static ThisModule) -> Result<Self> { >> +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; > > Can you please format this as well? Done. >> +/// Ok(Module { _reg: reg }) >> +/// } >> +/// } >> +/// } >> +/// >> +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ >> +/// ::kernel::bindings::mdio_device_id { >> +/// phy_id: 0x003b1861, >> +/// 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),+]) => { >> + #[no_mangle] >> + static __mod_mdio__phydev_device_table: [ >> + ::kernel::bindings::mdio_device_id; >> + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 >> + ] = [ >> + $($dev.mdio_device_id()),+, >> + ::kernel::bindings::mdio_device_id { >> + phy_id: 0, >> + phy_id_mask: 0 >> + } >> + ]; >> + }; >> + >> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { >> + struct Module { >> + _reg: ::kernel::net::phy::Registration, >> + } >> + >> + $crate::prelude::module! { >> + type: Module, >> + $($f)* >> + } >> + >> + const _: () = { >> + static mut DRIVERS: [ >> + ::kernel::net::phy::DriverVTable; >> + $crate::module_phy_driver!(@count_devices $($driver),+) >> + ] = [ >> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >> + ]; >> + >> + impl ::kernel::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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; > > Can you put the safe operations outside of the `unsafe` block? 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 mut reg = ::kernel::net::phy::Registration::register( module, core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), )?; Ok(Module { _reg: reg }) } > Since `rustfmt` does not work well within `macro_rules`, you > have to manually format this code. One trick I use to do this > is to replace all meta variables with normal variables and > just put it in `rustfmt` and then revert the replacement in > the output. Understood, done.
On 25.10.23 02:02, FUJITA Tomonori wrote: > On Tue, 24 Oct 2023 16:28:02 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 24.10.23 02:58, 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 <fujita.tomonori@gmail.com> >>> --- >>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 129 insertions(+) >>> >>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>> index 2d821c2475e1..f346b2b4d3cb 100644 >>> --- a/rust/kernel/net/phy.rs >>> +++ b/rust/kernel/net/phy.rs >>> @@ -706,3 +706,132 @@ 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 >>> +/// >>> +/// ```ignore >> >> Is this example ignored, because it does not compile? > > The old version can't be compiled but the current version can so I'll > drop ignore. > > >> I think Wedson was wrapping his example with `module!` inside >> of a module, so maybe try that? > > I'm not sure what you mean. Wedson did this [1], note the `# mod module_fs_sample`: /// # Examples /// /// ``` /// # mod module_fs_sample { /// use kernel::prelude::*; /// use kernel::{c_str, fs}; /// /// kernel::module_fs! { /// type: MyFs, /// name: "myfs", /// author: "Rust for Linux Contributors", /// description: "My Rust fs", /// license: "GPL", /// } /// /// struct MyFs; /// impl fs::FileSystem for MyFs { /// const NAME: &'static CStr = c_str!("myfs"); /// } /// # } /// ``` [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 >>> +/// use kernel::c_str; >>> +/// use kernel::net::phy::{self, DeviceId}; >>> +/// use kernel::prelude::*; [...] >>> + const _: () = { >>> + static mut DRIVERS: [ >>> + ::kernel::net::phy::DriverVTable; >>> + $crate::module_phy_driver!(@count_devices $($driver),+) >>> + ] = [ >>> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >>> + ]; >>> + >>> + impl ::kernel::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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; >> >> Can you put the safe operations outside of the `unsafe` block? > > 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 mut reg = ::kernel::net::phy::Registration::register( > module, > core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), > )?; > Ok(Module { _reg: reg }) > } Here the `unsafe` block and the `SAFETY` comment are pretty far away. What about this?: 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 = ::kernel::net::phy::Registration::register(module, ::core::pin::Pin::static_mut(drivers))?; Ok(Module { _reg: reg }) } Also note that I added `::` to the front of `core`.
On Wed, 25 Oct 2023 07:29:32 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 25.10.23 02:02, FUJITA Tomonori wrote: >> On Tue, 24 Oct 2023 16:28:02 +0000 >> Benno Lossin <benno.lossin@proton.me> wrote: >> >>> On 24.10.23 02:58, 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 <fujita.tomonori@gmail.com> >>>> --- >>>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 129 insertions(+) >>>> >>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>>> index 2d821c2475e1..f346b2b4d3cb 100644 >>>> --- a/rust/kernel/net/phy.rs >>>> +++ b/rust/kernel/net/phy.rs >>>> @@ -706,3 +706,132 @@ 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 >>>> +/// >>>> +/// ```ignore >>> >>> Is this example ignored, because it does not compile? >> >> The old version can't be compiled but the current version can so I'll >> drop ignore. >> >> >>> I think Wedson was wrapping his example with `module!` inside >>> of a module, so maybe try that? >> >> I'm not sure what you mean. > > Wedson did this [1], note the `# mod module_fs_sample`: > > /// # Examples > /// > /// ``` > /// # mod module_fs_sample { > /// use kernel::prelude::*; > /// use kernel::{c_str, fs}; > /// > /// kernel::module_fs! { > /// type: MyFs, > /// name: "myfs", > /// author: "Rust for Linux Contributors", > /// description: "My Rust fs", > /// license: "GPL", > /// } > /// > /// struct MyFs; > /// impl fs::FileSystem for MyFs { > /// const NAME: &'static CStr = c_str!("myfs"); > /// } > /// # } > /// ``` > > [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 You are suggesting like the following? /// # Examples /// /// ``` /// # mod module_phy_driver_sample { ... /// # } /// ``` What benefits? >>>> + const _: () = { >>>> + static mut DRIVERS: [ >>>> + ::kernel::net::phy::DriverVTable; >>>> + $crate::module_phy_driver!(@count_devices $($driver),+) >>>> + ] = [ >>>> + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ >>>> + ]; >>>> + >>>> + impl ::kernel::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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; >>> >>> Can you put the safe operations outside of the `unsafe` block? >> >> 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 mut reg = ::kernel::net::phy::Registration::register( >> module, >> core::pin::Pin::static_mut(unsafe { &mut DRIVERS }), >> )?; >> Ok(Module { _reg: reg }) >> } > > Here the `unsafe` block and the `SAFETY` comment are pretty far away. > What about this?: > > 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 = > ::kernel::net::phy::Registration::register(module, ::core::pin::Pin::static_mut(drivers))?; > Ok(Module { _reg: reg }) > } > > Also note that I added `::` to the front of `core`. I see.
On 25.10.23 09:57, FUJITA Tomonori wrote: > On Wed, 25 Oct 2023 07:29:32 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 25.10.23 02:02, FUJITA Tomonori wrote: >>> On Tue, 24 Oct 2023 16:28:02 +0000 >>> Benno Lossin <benno.lossin@proton.me> wrote: >>> >>>> On 24.10.23 02:58, 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 <fujita.tomonori@gmail.com> >>>>> --- >>>>> rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 129 insertions(+) >>>>> >>>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs >>>>> index 2d821c2475e1..f346b2b4d3cb 100644 >>>>> --- a/rust/kernel/net/phy.rs >>>>> +++ b/rust/kernel/net/phy.rs >>>>> @@ -706,3 +706,132 @@ 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 >>>>> +/// >>>>> +/// ```ignore >>>> >>>> Is this example ignored, because it does not compile? >>> >>> The old version can't be compiled but the current version can so I'll >>> drop ignore. >>> >>> >>>> I think Wedson was wrapping his example with `module!` inside >>>> of a module, so maybe try that? >>> >>> I'm not sure what you mean. >> >> Wedson did this [1], note the `# mod module_fs_sample`: >> >> /// # Examples >> /// >> /// ``` >> /// # mod module_fs_sample { >> /// use kernel::prelude::*; >> /// use kernel::{c_str, fs}; >> /// >> /// kernel::module_fs! { >> /// type: MyFs, >> /// name: "myfs", >> /// author: "Rust for Linux Contributors", >> /// description: "My Rust fs", >> /// license: "GPL", >> /// } >> /// >> /// struct MyFs; >> /// impl fs::FileSystem for MyFs { >> /// const NAME: &'static CStr = c_str!("myfs"); >> /// } >> /// # } >> /// ``` >> >> [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 > > You are suggesting like the following? > > /// # Examples > /// > /// ``` > /// # mod module_phy_driver_sample { > ... > /// # } > /// ``` > > What benefits? IIRC Wedson mentioned that without that it did not compile. So if it compiles for you without it, I would not add it.
On Wed, 25 Oct 2023 08:08:53 +0000 Benno Lossin <benno.lossin@proton.me> wrote: >>>>>> +/// 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 >>>>>> +/// >>>>>> +/// ```ignore >>>>> >>>>> Is this example ignored, because it does not compile? >>>> >>>> The old version can't be compiled but the current version can so I'll >>>> drop ignore. >>>> >>>> >>>>> I think Wedson was wrapping his example with `module!` inside >>>>> of a module, so maybe try that? >>>> >>>> I'm not sure what you mean. >>> >>> Wedson did this [1], note the `# mod module_fs_sample`: >>> >>> /// # Examples >>> /// >>> /// ``` >>> /// # mod module_fs_sample { >>> /// use kernel::prelude::*; >>> /// use kernel::{c_str, fs}; >>> /// >>> /// kernel::module_fs! { >>> /// type: MyFs, >>> /// name: "myfs", >>> /// author: "Rust for Linux Contributors", >>> /// description: "My Rust fs", >>> /// license: "GPL", >>> /// } >>> /// >>> /// struct MyFs; >>> /// impl fs::FileSystem for MyFs { >>> /// const NAME: &'static CStr = c_str!("myfs"); >>> /// } >>> /// # } >>> /// ``` >>> >>> [1]: https://github.com/wedsonaf/linux/commit/e909f439481cf6a3df00c7064b0d64cee8630fe9#diff-9b893393ed2a537222d79f6e2fceffb7e9d8967791c2016962be3171c446210fR104-R124 >> >> You are suggesting like the following? >> >> /// # Examples >> /// >> /// ``` >> /// # mod module_phy_driver_sample { >> ... >> /// # } >> /// ``` >> >> What benefits? > > IIRC Wedson mentioned that without that it did not compile. So > if it compiles for you without it, I would not add it. Ah, it's necessary.
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 2d821c2475e1..f346b2b4d3cb 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -706,3 +706,132 @@ 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 +/// +/// ```ignore +/// use kernel::c_str; +/// use kernel::net::phy::{self, DeviceId}; +/// use kernel::prelude::*; +/// +/// kernel::module_phy_driver! { +/// drivers: [PhyAX88772A], +/// device_table: [ +/// DeviceId::new_with_driver::<PhyAX88772A>(), +/// ], +/// name: "rust_asix_phy", +/// author: "Rust for Linux Contributors", +/// description: "Rust Asix PHYs driver", +/// license: "GPL", +/// } +/// +/// struct PhyAX88772A; +/// +/// impl phy::Driver for PhyAX88772A { +/// const NAME: &'static CStr = c_str!("Asix Electronics AX88772A"); +/// const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861); +/// } +/// ``` +/// +/// 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_asix_phy", +/// author: "Rust for Linux Contributors", +/// description: "Rust Asix PHYs driver", +/// license: "GPL", +/// } +/// +/// const _: () = { +/// static mut DRIVERS: [::kernel::net::phy::DriverVTable; 1] = [ +/// ::kernel::net::phy::create_phy_driver::<PhyAX88772A>::(), +/// ]; +/// +/// impl ::kernel::Module for Module { +/// fn init(module: &'static ThisModule) -> Result<Self> { +/// let mut reg = unsafe { ::kernel::net::phy::Registration::register(module, Pin::static_mut(&mut DRIVERS)) }?; +/// Ok(Module { _reg: reg }) +/// } +/// } +/// } +/// +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [ +/// ::kernel::bindings::mdio_device_id { +/// phy_id: 0x003b1861, +/// 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),+]) => { + #[no_mangle] + static __mod_mdio__phydev_device_table: [ + ::kernel::bindings::mdio_device_id; + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 + ] = [ + $($dev.mdio_device_id()),+, + ::kernel::bindings::mdio_device_id { + phy_id: 0, + phy_id_mask: 0 + } + ]; + }; + + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { + struct Module { + _reg: ::kernel::net::phy::Registration, + } + + $crate::prelude::module! { + type: Module, + $($f)* + } + + const _: () = { + static mut DRIVERS: [ + ::kernel::net::phy::DriverVTable; + $crate::module_phy_driver!(@count_devices $($driver),+) + ] = [ + $(::kernel::net::phy::create_phy_driver::<$driver>()),+ + ]; + + impl ::kernel::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 mut reg = unsafe { ::kernel::net::phy::Registration::register(module, core::pin::Pin::static_mut(&mut DRIVERS)) }?; + Ok(Module { _reg: reg }) + } + } + }; + + $crate::module_phy_driver!(@device_table [$($dev),+]); + } +}
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 <fujita.tomonori@gmail.com> --- rust/kernel/net/phy.rs | 129 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+)