Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

Andrew Lunn
2023-11-17T22:54:12Z

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