| Message ID | 20231026001050.1720612-3-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
FUJITA Tomonori <fujita.tomonori@gmail.com> writes: > 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> Reviewed-by: Alice Ryhl <aliceryhl@google.com> A few minor nits: > + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { Here, you can add $(,)? to allow trailing commas when the macro is used. Like this: (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => { > + ::kernel::bindings::mdio_device_id { Here, I recommend `$crate` instead of `::kernel`. > +/// #[no_mangle] > +/// 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, > +/// }, > +/// ]; I'd probably put a safety comment on the `#[no_mangle]` invocation to say that "C will not read off the end of this constant since the last element is zero". Alice
On Thu, Oct 26, 2023 at 09:10:47AM +0900, FUJITA Tomonori 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 > +/// > +/// ``` > +/// # mod module_phy_driver_sample { > +/// 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; > +/// > +/// #[vtable] > +/// 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); > +/// } > +/// # } > +/// ``` When run the following kunit command: ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \ --kconfig_add CONFIG_RUST=y \ --kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \ --kconfig_add CONFIG_PHYLIB=y \ --kconfig_add CONFIG_NETDEVICES=y \ --kconfig_add CONFIG_NET=y \ --kconfig_add CONFIG_AX88796B_RUST_PHY=y \ --kconfig_add CONFIG_AX88796B_PHY=y I got the following errors: ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 >>> drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a ld.lld: error: duplicate symbol: __rust_asix_phy_exit >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 >>> drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 >>> rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 >>> drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a Because kunit will use the above doc test to generate test, and since CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has been called twice, and causes duplicate symbols. For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage in the example. But for __mod_mdio__phydev_device_table, it's hard-coded in `module_phy_driver!`, I don't have a quick fix right now. Also, does it mean `module_phy_driver!` is only supposed to be "called" once for the entire kernel? Regards, Boqun
On Fri, Nov 17, 2023 at 02:21:38PM -0800, Boqun Feng wrote: > On Thu, Oct 26, 2023 at 09:10:47AM +0900, FUJITA Tomonori 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 > > +/// > > +/// ``` > > +/// # mod module_phy_driver_sample { > > +/// 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; > > +/// > > +/// #[vtable] > > +/// 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); > > +/// } > > +/// # } > > +/// ``` > > When run the following kunit command: > > ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \ > --kconfig_add CONFIG_RUST=y \ > --kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \ > --kconfig_add CONFIG_PHYLIB=y \ > --kconfig_add CONFIG_NETDEVICES=y \ > --kconfig_add CONFIG_NET=y \ > --kconfig_add CONFIG_AX88796B_RUST_PHY=y \ > --kconfig_add CONFIG_AX88796B_PHY=y > > I got the following errors: > > ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a > > ld.lld: error: duplicate symbol: __rust_asix_phy_exit > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a > > ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a > > Because kunit will use the above doc test to generate test, and since > CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has > been called twice, and causes duplicate symbols. > > For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage > in the example. But for __mod_mdio__phydev_device_table, it's hard-coded > in `module_phy_driver!`, I don't have a quick fix right now. Also, does > it mean `module_phy_driver!` is only supposed to be "called" once for > the entire kernel? Each kernel module should be in its own symbol name space. The only symbols which are visible outside of the module are those exported using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not export anything, in general. Being built in also does not change this. Neither drivers/net/phy/ax88796b_rust.o nor rust/doctests_kernel_generated.o should have exported this symbol. I've no idea how this actually works, i guess there are multiple passes through the linker? Maybe once to resolve symbols across object files within a module. Normal global symbols are then made local, leaving only those exported with EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL()? A second pass through linker then links all the exported symbols thorough the kernel? Andrew
On 11/17/23 23:54, Andrew Lunn wrote: > Each kernel module should be in its own symbol name space. The only > symbols which are visible outside of the module are those exported > using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not > export anything, in general. > > Being built in also does not change this. > > Neither drivers/net/phy/ax88796b_rust.o nor > rust/doctests_kernel_generated.o should have exported this symbol. > > I've no idea how this actually works, i guess there are multiple > passes through the linker? Maybe once to resolve symbols across object > files within a module. Normal global symbols are then made local, > leaving only those exported with EXPORT_SYMBOL_GPL() or > EXPORT_SYMBOL()? A second pass through linker then links all the > exported symbols thorough the kernel? I brought this issue up in [1], but I was a bit confused by your last reply there, as I have no idea how the `EXPORT_SYMBOL` macros work. IIRC on the Rust side all public items are automatically GPL exported. But `#[no_mangle]` is probably a special case, since in userspace it is usually used to do interop with C (and therefore the symbol is always exported with the name not mangled). One fix would be to make the name unique by using the type name supplied in the macro. [1]: https://lore.kernel.org/rust-for-linux/1aea7ddb-73b7-8228-161e-e2e4ff5bc98d@proton.me/
On Fri, Nov 17, 2023 at 11:01:58PM +0000, Benno Lossin wrote: > On 11/17/23 23:54, Andrew Lunn wrote: > > Each kernel module should be in its own symbol name space. The only > > symbols which are visible outside of the module are those exported > > using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not > > export anything, in general. > > > > Being built in also does not change this. > > > > Neither drivers/net/phy/ax88796b_rust.o nor > > rust/doctests_kernel_generated.o should have exported this symbol. > > > > I've no idea how this actually works, i guess there are multiple > > passes through the linker? Maybe once to resolve symbols across object > > files within a module. Normal global symbols are then made local, > > leaving only those exported with EXPORT_SYMBOL_GPL() or > > EXPORT_SYMBOL()? A second pass through linker then links all the > > exported symbols thorough the kernel? > > I brought this issue up in [1], but I was a bit confused by your last > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work. > > IIRC on the Rust side all public items are automatically GPL exported. So that sounds wrong to me. But as i said, i don't know how this actually works. In kernel C code, you effectively have three levels of symbol visibility. Anything static is not visible outside of the .o file. Anything global within a module is visible across multiple compilation units within that module, but not visible outside of the module. Symbols marked with EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL() are visible through the entire kernel. Device drivers generally don't have any EXPORT_SYMBOL* symbols. They do however need to import symbols which are EXPORT_SYMBOL*. Core code, like phylib, has lots of EXPORT_SYMBOL*. You can also have modules which are just libraries, and they use EXPORT_SYMBOL*. Other modules will be linked to them. > But `#[no_mangle]` is probably a special case, since in userspace it > is usually used to do interop with C (and therefore the symbol is always > exported with the name not mangled). So you might need this for symbols which are EXPORT_SYMBOL*, especially if they are going to be used by C code. If only other Rust modules are going to use them, and the mangled name is predictable, i suppose you could use the mangled name. Andrew
On Fri, 17 Nov 2023 23:01:58 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 11/17/23 23:54, Andrew Lunn wrote: >> Each kernel module should be in its own symbol name space. The only >> symbols which are visible outside of the module are those exported >> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not >> export anything, in general. >> >> Being built in also does not change this. >> >> Neither drivers/net/phy/ax88796b_rust.o nor >> rust/doctests_kernel_generated.o should have exported this symbol. >> >> I've no idea how this actually works, i guess there are multiple >> passes through the linker? Maybe once to resolve symbols across object >> files within a module. Normal global symbols are then made local, >> leaving only those exported with EXPORT_SYMBOL_GPL() or >> EXPORT_SYMBOL()? A second pass through linker then links all the >> exported symbols thorough the kernel? > > I brought this issue up in [1], but I was a bit confused by your last > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work. > > IIRC on the Rust side all public items are automatically GPL exported. Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL(). > But `#[no_mangle]` is probably a special case, since in userspace it > is usually used to do interop with C (and therefore the symbol is always > exported with the name not mangled). > > One fix would be to make the name unique by using the type name supplied > in the macro. > > [1]: https://lore.kernel.org/rust-for-linux/1aea7ddb-73b7-8228-161e-e2e4ff5bc98d@proton.me/ I fixed this in a different way; declaring __mod_mdio__phydev_device_table only when built as a module. (@device_table [$($dev:expr),+]) => { + #[cfg(MODULE)] #[no_mangle] static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [ This is used for dynamic loading so when a phy driver is built-in, we don't need this. When a driver is built as a module, the user-space tool converts this into moduole alias information. This __mod_mdio__phydev_device_table isn't loaded into kernel so no conflict even when multiple phy drivers are loaded.
On Sat, 18 Nov 2023 00:18:10 +0100 Andrew Lunn <andrew@lunn.ch> wrote: >> But `#[no_mangle]` is probably a special case, since in userspace it >> is usually used to do interop with C (and therefore the symbol is always >> exported with the name not mangled). > > So you might need this for symbols which are EXPORT_SYMBOL*, > especially if they are going to be used by C code. If only other Rust > modules are going to use them, and the mangled name is predictable, i > suppose you could use the mangled name. This isn't loaded into the kernel. This is used only by the tool to build a module. When built as a module, this information is converted into module alias information for dynamic loading.
On Fri, 17 Nov 2023 14:21:38 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > When run the following kunit command: > > ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \ > --kconfig_add CONFIG_RUST=y \ > --kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \ > --kconfig_add CONFIG_PHYLIB=y \ > --kconfig_add CONFIG_NETDEVICES=y \ > --kconfig_add CONFIG_NET=y \ > --kconfig_add CONFIG_AX88796B_RUST_PHY=y \ > --kconfig_add CONFIG_AX88796B_PHY=y > > I got the following errors: > > ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a > > ld.lld: error: duplicate symbol: __rust_asix_phy_exit > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a > > ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table > >>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0 > >>> rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a > >>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0 > >>> drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a > > Because kunit will use the above doc test to generate test, and since > CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has > been called twice, and causes duplicate symbols. > > For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage > in the example. Surely, done. > But for __mod_mdio__phydev_device_table, it's hard-coded > in `module_phy_driver!`, I don't have a quick fix right now. Also, does > it mean `module_phy_driver!` is only supposed to be "called" once for > the entire kernel? As I wrote in another mail, I fixed this.
On Fri, 17 Nov 2023 09:39:08 +0000 Alice Ryhl <aliceryhl@google.com> wrote: > FUJITA Tomonori <fujita.tomonori@gmail.com> writes: >> 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> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > A few minor nits: > >> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => { > > Here, you can add $(,)? to allow trailing commas when the macro is used. > Like this: > > (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => { Updated. > >> + ::kernel::bindings::mdio_device_id { > > Here, I recommend `$crate` instead of `::kernel`. I copied the code that Benno wrote, IIRC. Either is fine by me. Why `$crate` is better here? Also better to replace other `::kernel` in this macro? >> +/// #[no_mangle] >> +/// 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, >> +/// }, >> +/// ]; > > I'd probably put a safety comment on the `#[no_mangle]` invocation to say that > "C will not read off the end of this constant since the last element is zero". Added. Thanks a lot!
On 19.11.23 11:50, FUJITA Tomonori wrote: > On Fri, 17 Nov 2023 09:39:08 +0000 > Alice Ryhl <aliceryhl@google.com> wrote: >> FUJITA Tomonori <fujita.tomonori@gmail.com> writes: >>> + ::kernel::bindings::mdio_device_id { >> >> Here, I recommend `$crate` instead of `::kernel`. > > I copied the code that Benno wrote, IIRC. Either is fine by me. Why > `$crate` is better here? When I suggested that, I might have confused the location of the macro being in the `macros` crate. There you cannot use `$crate`, since it is not available for proc macros. But since this is in the `kernel` crate, you can use `$crate`. `$crate` is better, since it unambiguously refers to the current crate and `::kernel` only works, because we named our crate `kernel`. So the code using `$crate` is more portable. > Also better to replace other `::kernel` in this macro? Yes.
On Sun, Nov 19, 2023 at 06:25:44PM +0900, FUJITA Tomonori wrote: > On Fri, 17 Nov 2023 23:01:58 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > > > On 11/17/23 23:54, Andrew Lunn wrote: > >> Each kernel module should be in its own symbol name space. The only > >> symbols which are visible outside of the module are those exported > >> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not > >> export anything, in general. > >> > >> Being built in also does not change this. > >> > >> Neither drivers/net/phy/ax88796b_rust.o nor > >> rust/doctests_kernel_generated.o should have exported this symbol. > >> > >> I've no idea how this actually works, i guess there are multiple > >> passes through the linker? Maybe once to resolve symbols across object > >> files within a module. Normal global symbols are then made local, > >> leaving only those exported with EXPORT_SYMBOL_GPL() or > >> EXPORT_SYMBOL()? A second pass through linker then links all the > >> exported symbols thorough the kernel? > > > > I brought this issue up in [1], but I was a bit confused by your last > > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work. > > > > IIRC on the Rust side all public items are automatically GPL exported. > > Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL() > or EXPORT_SYMBOL_GPL(). Do they need to be public? Generally, a PHY driver does not export anything. So you can probably make them private. We just however need to ensure the compiler/linker does not think they are unused, so throws them away. I would however like to get an understanding how EXPORT_SYMBOL* is supposed to work in rust. Can it really be hidden away? Or should methods be explicitly marked like C code? What is the Rust equivalent of the three levels of symbol scope we have in C? Andrew
On Sun, 19 Nov 2023 16:50:34 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Sun, Nov 19, 2023 at 06:25:44PM +0900, FUJITA Tomonori wrote: >> On Fri, 17 Nov 2023 23:01:58 +0000 >> Benno Lossin <benno.lossin@proton.me> wrote: >> >> > On 11/17/23 23:54, Andrew Lunn wrote: >> >> Each kernel module should be in its own symbol name space. The only >> >> symbols which are visible outside of the module are those exported >> >> using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not >> >> export anything, in general. >> >> >> >> Being built in also does not change this. >> >> >> >> Neither drivers/net/phy/ax88796b_rust.o nor >> >> rust/doctests_kernel_generated.o should have exported this symbol. >> >> >> >> I've no idea how this actually works, i guess there are multiple >> >> passes through the linker? Maybe once to resolve symbols across object >> >> files within a module. Normal global symbols are then made local, >> >> leaving only those exported with EXPORT_SYMBOL_GPL() or >> >> EXPORT_SYMBOL()? A second pass through linker then links all the >> >> exported symbols thorough the kernel? >> > >> > I brought this issue up in [1], but I was a bit confused by your last >> > reply there, as I have no idea how the `EXPORT_SYMBOL` macros work. >> > >> > IIRC on the Rust side all public items are automatically GPL exported. >> >> Hmm, they are public but doesn't look like exported by EXPORT_SYMBOL() >> or EXPORT_SYMBOL_GPL(). > > Do they need to be public? Generally, a PHY driver does not export > anything. So you can probably make them private. We just however need > to ensure the compiler/linker does not think they are unused, so > throws them away. The Rust ax88796b driver doesn't export anything. The Rust and C drivers handle the device_table in the same way when they are built as a module. $ grep __mod_mdio /proc/kallsyms ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust] $ grep __mod_mdio /proc/kallsyms ffffffffa0288010 d __mod_mdio__asix_tbl_device_table [ax88796b] Note that when the Rust ax88796b driver is built-in, __mod_mdio__phydev_device_table is not defined. Sorry about my explanation, which leads to the confusion, I think. > I would however like to get an understanding how EXPORT_SYMBOL* is > supposed to work in rust. Can it really be hidden away? Or should > methods be explicitly marked like C code? What is the Rust equivalent > of the three levels of symbol scope we have in C? If I understand correctly, there is no official way to export symbols in Rust kernel modules; No equivalent to EXPORT_SYMBOL* for Rust code built as a module yet. Note that all core Rust functions are GPL exported so they are available to Rust kernel modules. `static` has a different meaning in Rust but I think that you can use file scope and kernel-module scope in Rust.
> The Rust ax88796b driver doesn't export anything. The Rust and C > drivers handle the device_table in the same way when they are built as > a module. > > $ grep __mod_mdio /proc/kallsyms > ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust] > > $ grep __mod_mdio /proc/kallsyms > ffffffffa0288010 d __mod_mdio__asix_tbl_device_table [ax88796b] I checked what r and d mean. If they are upper case, they are exported. Lower case means they are not exported. My laptop is using the realtek PHY driver: 0000000000000000 r __mod_mdio__realtek_tbl_device_table [realtek] Also lower r. Looking at all the symbols for the realtek driver, all the symbols use lower case. Nothing is exported. Is that what you see for the ax88796b_rust? Andrew
On Mon, 20 Nov 2023 15:13:23 +0100 Andrew Lunn <andrew@lunn.ch> wrote: >> The Rust ax88796b driver doesn't export anything. The Rust and C >> drivers handle the device_table in the same way when they are built as >> a module. >> >> $ grep __mod_mdio /proc/kallsyms >> ffffffffa0358058 r __mod_mdio__phydev_device_table [ax88796b_rust] >> >> $ grep __mod_mdio /proc/kallsyms >> ffffffffa0288010 d __mod_mdio__asix_tbl_device_table [ax88796b] > > I checked what r and d mean. If they are upper case, they are > exported. Lower case means they are not exported. > > My laptop is using the realtek PHY driver: > > 0000000000000000 r __mod_mdio__realtek_tbl_device_table [realtek] > > Also lower r. > > Looking at all the symbols for the realtek driver, all the symbols use > lower case. Nothing is exported. > > Is that what you see for the ax88796b_rust? Yes, all the symbols for ax88796b_rust use lower case.
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 61223f638bc6..145d0407fe31 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -723,3 +723,146 @@ 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: [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; +/// +/// #[vtable] +/// 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", +/// } +/// +/// struct PhyAX88772A; +/// +/// #[vtable] +/// 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); +/// } +/// +/// 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 drivers = unsafe { &mut DRIVERS }; +/// let mut reg = ::kernel::net::phy::Registration::register( +/// module, +/// ::core::pin::Pin::static_mut(drivers), +/// )?; +/// Ok(Module { _reg: reg }) +/// } +/// } +/// }; +/// +/// #[no_mangle] +/// 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 drivers = unsafe { &mut DRIVERS }; + let mut reg = ::kernel::net::phy::Registration::register( + module, + ::core::pin::Pin::static_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 | 143 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)