Re: [RFC PATCH v2 3/3] net: phy: add Rust Asix PHY driver

[Benno Lossin <benno.lossin@proton.me>]

On 24.09.23 08:49, FUJITA Tomonori wrote:
> This is the Rust implementation of drivers/net/phy/ax88796b.c.  The
> features are equivalent. You can choose C or Rust on kernel
> configuration.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>   drivers/net/phy/Kconfig          |  16 ++++
>   drivers/net/phy/Makefile         |   8 +-
>   drivers/net/phy/ax88796b_rust.rs | 147 +++++++++++++++++++++++++++++++
>   rust/uapi/uapi_helper.h          |   1 +
>   4 files changed, 171 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/phy/ax88796b_rust.rs
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 107880d13d21..47576c905837 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -107,6 +107,22 @@ config AX88796B_PHY
>   	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>   	  AX88796B package.
> 
> +choice
> +	prompt "Implementation options"
> +	depends on AX88796B_PHY
> +	help
> +	  There are two implementations for a driver for Asix PHYs; C and Rust.
> +	  If not sure, choose C.
> +
> +config AX88796B_C_PHY
> +	bool "C driver for Asix PHYs"
> +
> +config AX88796B_RUST_PHY
> +	bool "Rust driver for Asix PHYs"
> +	depends on RUST
> +
> +endchoice
> +
>   config BROADCOM_PHY
>   	tristate "Broadcom 54XX PHYs"
>   	select BCM_NET_PHYLIB
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index c945ed9bd14b..4bdefa90e4dc 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -41,7 +41,13 @@ aquantia-objs			+= aquantia_hwmon.o
>   endif
>   obj-$(CONFIG_AQUANTIA_PHY)	+= aquantia.o
>   obj-$(CONFIG_AT803X_PHY)	+= at803x.o
> -obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
> +ifdef CONFIG_AX88796B_PHY
> +  ifdef CONFIG_AX88796B_C_PHY
> +    obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
> +  else
> +    obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
> +  endif
> +endif
>   obj-$(CONFIG_BCM54140_PHY)	+= bcm54140.o
>   obj-$(CONFIG_BCM63XX_PHY)	+= bcm63xx.o
>   obj-$(CONFIG_BCM7XXX_PHY)	+= bcm7xxx.o
> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
> new file mode 100644
> index 000000000000..88386b8196e9
> --- /dev/null
> +++ b/drivers/net/phy/ax88796b_rust.rs
> @@ -0,0 +1,147 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust Asix PHYs driver
> +use kernel::c_str;
> +use kernel::net::phy::{self, DeviceId, Driver};
> +use kernel::prelude::*;
> +use kernel::uapi;
> +
> +kernel::module_phy_driver! {
> +    drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> +    device_table: [
> +        DeviceId::new_with_driver::<PhyAX88772A>(),
> +        DeviceId::new_with_driver::<PhyAX88772C>(),
> +        DeviceId::new_with_driver::<PhyAX88796B>()
> +    ],
> +    type: RustAsixPhy,
> +    name: "rust_asix_phy",
> +    author: "FUJITA Tomonori <fujita.tomonori@gmail.com>",
> +    description: "Rust Asix PHYs driver",
> +    license: "GPL",
> +}
> +
> +struct PhyAX88772A;
> +
> +impl PhyAX88772A {
> +    const PHY_ID: u32 = 0x003b1861;

Why are you specifying this constant twice?
Doing this solely in the `impl Driver` block
should be sufficient.

> +}
> +
> +#[vtable]
> +impl phy::Driver for PhyAX88772A {
> +    const FLAGS: u32 = phy::PHY_IS_INTERNAL;
> +    const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
> +    const PHY_ID: u32 = Self::PHY_ID;
> +    const PHY_ID_MASK: u32 = !0;

Why is this some magic number? It should be
a constant with a nice name.

> +
> +    // AX88772A is not working properly with some old switches (NETGEAR EN 108TP):
> +    // after autoneg is done and the link status is reported as active, the MII_LPA
> +    // register is 0. This issue is not reproducible on AX88772C.
> +    fn read_status(dev: &mut phy::Device) -> Result<u16> {
> +        dev.genphy_update_link()?;
> +        if !dev.get_link() {
> +            return Ok(0);
> +        }
> +        // If MII_LPA is 0, phy_resolve_aneg_linkmode() will fail to resolve
> +        // linkmode so use MII_BMCR as default values.
> +        let ret = dev.read(uapi::MII_BMCR as u16)?;
> +
> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> +            dev.set_speed(100);
> +        } else {
> +            dev.set_speed(10);
> +        }
> +
> +        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
> +            phy::DuplexMode::Full
> +        } else {
> +            phy::DuplexMode::Half
> +        };
> +        dev.set_duplex(duplex);
> +
> +        dev.genphy_read_lpa()?;
> +
> +        if dev.is_autoneg_enabled() && dev.is_autoneg_completed() {
> +            dev.resolve_aneg_linkmode();
> +        }
> +
> +        Ok(0)
> +    }
> +
> +    fn suspend(dev: &mut phy::Device) -> Result {
> +        dev.genphy_suspend()
> +    }
> +
> +    fn resume(dev: &mut phy::Device) -> Result {
> +        dev.genphy_resume()
> +    }
> +
> +    fn soft_reset(dev: &mut phy::Device) -> Result {
> +        RustAsixPhy::soft_reset(dev)
> +    }
> +
> +    fn link_change_notify(dev: &mut phy::Device) {
> +        // Reset PHY, otherwise MII_LPA will provide outdated information.
> +        // This issue is reproducible only with some link partner PHYs.
> +        if dev.state() == phy::DeviceState::NoLink {
> +            let _ = dev.init_hw();
> +            let _ = dev.start_aneg();
> +        }
> +    }
> +}
> +
> +struct PhyAX88772C;
> +
> +impl PhyAX88772C {
> +    const PHY_ID: u32 = 0x003b1881;
> +}
> +
> +#[vtable]
> +impl Driver for PhyAX88772C {
> +    const FLAGS: u32 = phy::PHY_IS_INTERNAL;
> +    const NAME: &'static CStr = c_str!("Asix Electronics AX88772C");
> +    const PHY_ID: u32 = Self::PHY_ID;
> +    const PHY_ID_MASK: u32 = !0;
> +
> +    fn suspend(dev: &mut phy::Device) -> Result {
> +        dev.genphy_suspend()
> +    }
> +
> +    fn resume(dev: &mut phy::Device) -> Result {
> +        dev.genphy_resume()
> +    }
> +
> +    fn soft_reset(dev: &mut phy::Device) -> Result {
> +        RustAsixPhy::soft_reset(dev)
> +    }
> +}
> +
> +struct PhyAX88796B;
> +
> +impl PhyAX88796B {
> +    const PHY_ID: u32 = 0x003b1841;
> +    const PHY_ID_MASK: u32 = 0xfffffff0;
> +}
> +
> +#[vtable]
> +impl Driver for PhyAX88796B {
> +    const NAME: &'static CStr = c_str!("Asix Electronics AX88796B");
> +    const PHY_ID: u32 = Self::PHY_ID;
> +    const PHY_ID_MASK: u32 = Self::PHY_ID_MASK;
> +
> +    fn soft_reset(dev: &mut phy::Device) -> Result {
> +        RustAsixPhy::soft_reset(dev)
> +    }
> +}
> +
> +struct RustAsixPhy;
> +
> +impl RustAsixPhy {
> +    // Performs a software PHY reset using the standard
> +    // BMCR_RESET bit and poll for the reset bit to be cleared.
> +    // Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation
> +    // such as used on the Individual Computers' X-Surf 100 Zorro card.
> +    fn soft_reset(dev: &mut phy::Device) -> Result {
> +        dev.write(uapi::MII_BMCR as u16, 0)?;
> +        dev.genphy_soft_reset()
> +    }
> +}
> diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
> index 301f5207f023..d92abe9064c2 100644
> --- a/rust/uapi/uapi_helper.h
> +++ b/rust/uapi/uapi_helper.h
> @@ -7,3 +7,4 @@
>    */
> 
>   #include <uapi/asm-generic/ioctl.h>
> +#include <uapi/linux/mii.h>
> --
> 2.34.1
> 
>