[net-next,v7,5/5] net: phy: add Rust Asix PHY driver

Commit Message

FUJITA Tomonori Oct. 26, 2023, 12:10 a.m. UTC

This is the Rust implementation of drivers/net/phy/ax88796b.c. The
features are equivalent. You can choose C or Rust versionon kernel
configuration.

Signed-off-by: FUJITA Tomonori <[e-mail]>
Reviewed-by: Trevor Gross <[e-mail]>
Reviewed-by: Benno Lossin <[e-mail]>
---
 MAINTAINERS                      |   8 ++
 drivers/net/phy/Kconfig          |   8 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 5 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs

Comments

Alice Ryhl Nov. 17, 2023, 9:39 a.m. UTC | #1

FUJITA Tomonori <[e-mail]> writes:
> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
> features are equivalent. You can choose C or Rust versionon kernel
> configuration.
> 
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> Reviewed-by: Trevor Gross <[e-mail]>
> Reviewed-by: Benno Lossin <[e-mail]>

Overall looks reasonable. I found various nits below:

There's a typo in your commit message: versionon.

> +use kernel::c_str;
> +use kernel::net::phy::{self, DeviceId, Driver};
> +use kernel::prelude::*;
> +use kernel::uapi;

You used the other import style in other patches.

> +        // 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 {

The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
these constants are defined as the wrong type?

It's probably difficult to get bindgen to change the type, but you could
do this at the top of the function or file:

	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

> +            let _ = dev.init_hw();
> +            let _ = dev.start_aneg();

Just to confirm: You want to call `start_aneg` even if `init_hw` returns
failure? And you want to ignore both errors?

Alice

FUJITA Tomonori Nov. 19, 2023, 9:57 a.m. UTC | #2

On Fri, 17 Nov 2023 09:39:15 +0000
Alice Ryhl <[e-mail]> wrote:

> FUJITA Tomonori <[e-mail]> writes:
>> This is the Rust implementation of drivers/net/phy/ax88796b.c. The
>> features are equivalent. You can choose C or Rust versionon kernel
>> configuration.
>> 
>> Signed-off-by: FUJITA Tomonori <[e-mail]>
>> Reviewed-by: Trevor Gross <[e-mail]>
>> Reviewed-by: Benno Lossin <[e-mail]>
> 
> Overall looks reasonable. I found various nits below:
> 
> There's a typo in your commit message: versionon.

Oops, will fix.

> 
>> +use kernel::c_str;
>> +use kernel::net::phy::{self, DeviceId, Driver};
>> +use kernel::prelude::*;
>> +use kernel::uapi;
> 
> You used the other import style in other patches.

use kernel::{
    c_str,
    net::phy::{self, DeviceId, Driver},
    prelude::*,
    uapi,
};

?

>> +        // 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 {
> 
> The `ret as u32` and `uapi::MII_BMCR as u16` casts make me think that
> these constants are defined as the wrong type?
> 
> It's probably difficult to get bindgen to change the type, but you could
> do this at the top of the function or file:

Yes, if we could specfy a type with bindgen, it's easier.

> 	const MII_BMCR: u16 = uapi::MII_BMCR as u16;
> 	const BMCR_SPEED100: u16 = uapi::BMCR_SPEED100 as u16;

I'll.


>> +            let _ = dev.init_hw();
>> +            let _ = dev.start_aneg();
> 
> Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> failure? And you want to ignore both errors?

Yeah, I tried to implement the exact same behavior in the original C driver.


Thanks!

Andrew Lunn Nov. 19, 2023, 4:03 p.m. UTC | #3

> >> +            let _ = dev.init_hw();
> >> +            let _ = dev.start_aneg();
> > 
> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> > failure? And you want to ignore both errors?
> 
> Yeah, I tried to implement the exact same behavior in the original C driver.

You probably could check the return values, and it would not make a
difference. Also, link_change_notify() is a void function, so you
cannot return the error anyway.

These low level functions basically only fail if the hardware is
`dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
recovery. You tend to get such errors during probe and fail the
probe. Or maybe if power management is wrong and it has turned a
critical clock off. But that is unlikely in this case, we are calling
link_change_notify because the PHY has told us something changed
recently, so it probably is alive.

I would say part of not checking the return code is also that C does
not have the nice feature that Rust has of making very simple to check
the return code. That combined with it being mostly pointless for PHY
drivers.

    Andrew

FUJITA Tomonori Nov. 21, 2023, 6:19 a.m. UTC | #4

On Sun, 19 Nov 2023 17:03:30 +0100
Andrew Lunn <[e-mail]> wrote:

>> >> +            let _ = dev.init_hw();
>> >> +            let _ = dev.start_aneg();
>> > 
>> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
>> > failure? And you want to ignore both errors?
>> 
>> Yeah, I tried to implement the exact same behavior in the original C driver.
> 
> You probably could check the return values, and it would not make a
> difference. Also, link_change_notify() is a void function, so you
> cannot return the error anyway.
> 
> These low level functions basically only fail if the hardware is
> `dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
> recovery. You tend to get such errors during probe and fail the
> probe. Or maybe if power management is wrong and it has turned a
> critical clock off. But that is unlikely in this case, we are calling
> link_change_notify because the PHY has told us something changed
> recently, so it probably is alive.
> 
> I would say part of not checking the return code is also that C does
> not have the nice feature that Rust has of making very simple to check
> the return code. That combined with it being mostly pointless for PHY
> drivers.

Understood. I'll check the first return value if you prefer. I might
add WARN_ON_ONCE after Rust supports it.

diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs
index ce6640ff523f..7522070a6acd 100644
--- a/drivers/net/phy/ax88796b_rust.rs
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -95,7 +95,9 @@ 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();
+            if let Err(_) = dev.init_hw() {
+                return;
+            }
             let _ = dev.start_aneg();
         }
     }

Greg KH Nov. 21, 2023, 7:12 a.m. UTC | #5

On Tue, Nov 21, 2023 at 03:19:39PM +0900, FUJITA Tomonori wrote:
> On Sun, 19 Nov 2023 17:03:30 +0100
> Andrew Lunn <[e-mail]> wrote:
> 
> >> >> +            let _ = dev.init_hw();
> >> >> +            let _ = dev.start_aneg();
> >> > 
> >> > Just to confirm: You want to call `start_aneg` even if `init_hw` returns
> >> > failure? And you want to ignore both errors?
> >> 
> >> Yeah, I tried to implement the exact same behavior in the original C driver.
> > 
> > You probably could check the return values, and it would not make a
> > difference. Also, link_change_notify() is a void function, so you
> > cannot return the error anyway.
> > 
> > These low level functions basically only fail if the hardware is
> > `dead`. You get an -EIO or maybe -TIMEDOUT back. And there is no real
> > recovery. You tend to get such errors during probe and fail the
> > probe. Or maybe if power management is wrong and it has turned a
> > critical clock off. But that is unlikely in this case, we are calling
> > link_change_notify because the PHY has told us something changed
> > recently, so it probably is alive.
> > 
> > I would say part of not checking the return code is also that C does
> > not have the nice feature that Rust has of making very simple to check
> > the return code. That combined with it being mostly pointless for PHY
> > drivers.
> 
> Understood. I'll check the first return value if you prefer. I might
> add WARN_ON_ONCE after Rust supports it.

Please don't, it shouldn't support it, handle errors properly and
return, don't panic machines (remember, the majority of the Linux
systems in the world run panic-on-warn).

thanks,
g
reg k-h

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index f0f0610defd6..aad0325121e0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3058,6 +3058,14 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/net/asix,ax88796c.yaml
 F:	drivers/net/ethernet/asix/ax88796c_*
 
+ASIX PHY DRIVER [RUST]
+M:	FUJITA Tomonori <[e-mail]>
+R:	Trevor Gross <[e-mail]>
+L:	[e-mail]
+L:	[e-mail]
+S:	Maintained
+F:	drivers/net/phy/ax88796b_rust.rs
+
 ASPEED CRYPTO DRIVER
 M:	Neal Liu <[e-mail]>
 L:	[e-mail] (moderated for non-subscribers)
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 0faebdb184ca..11b18370a05b 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -115,6 +115,14 @@  config AX88796B_PHY
 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
 	  AX88796B package.
 
+config AX88796B_RUST_PHY
+	bool "Rust reference driver for Asix PHYs"
+	depends on RUST_PHYLIB_ABSTRACTIONS && AX88796B_PHY
+	help
+	  Uses the Rust reference driver for Asix PHYs (ax88796b_rust.ko).
+	  The features are equivalent. It supports the Asix Electronics PHY
+	  found in the X-Surf 100 AX88796B package.
+
 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..58d7dfb095ab 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -41,7 +41,11 @@  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_RUST_PHY
+  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
+else
+  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
+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..6c24c94ab2db
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -0,0 +1,129 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
+
+//! Rust Asix PHYs driver
+//!
+//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)
+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>()
+    ],
+    name: "rust_asix_phy",
+    author: "FUJITA Tomonori <[e-mail]>",
+    description: "Rust Asix PHYs driver",
+    license: "GPL",
+}
+
+// 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 asix_soft_reset(dev: &mut phy::Device) -> Result {
+    dev.write(uapi::MII_BMCR as u16, 0)?;
+    dev.genphy_soft_reset()
+}
+
+struct PhyAX88772A;
+
+#[vtable]
+impl phy::Driver for PhyAX88772A {
+    const FLAGS: u32 = phy::flags::IS_INTERNAL;
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
+
+    // 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.is_link_up() {
+            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(uapi::SPEED_100);
+        } else {
+            dev.set_speed(uapi::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 {
+        asix_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;
+
+#[vtable]
+impl Driver for PhyAX88772C {
+    const FLAGS: u32 = phy::flags::IS_INTERNAL;
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88772C");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1881);
+
+    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 {
+        asix_soft_reset(dev)
+    }
+}
+
+struct PhyAX88796B;
+
+#[vtable]
+impl Driver for PhyAX88796B {
+    const NAME: &'static CStr = c_str!("Asix Electronics AX88796B");
+    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_model_mask(0x003b1841);
+
+    fn soft_reset(dev: &mut phy::Device) -> Result {
+        asix_soft_reset(dev)
+    }
+}
diff --git a/rust/uapi/uapi_helper.h b/rust/uapi/uapi_helper.h
index 301f5207f023..08f5e9334c9e 100644
--- a/rust/uapi/uapi_helper.h
+++ b/rust/uapi/uapi_helper.h
@@ -7,3 +7,5 @@ 
  */
 
 #include <uapi/asm-generic/ioctl.h>
+#include <uapi/linux/mii.h>
+#include <uapi/linux/ethtool.h>