diff mbox series

[net-next,v3,3/3] net: phy: add Rust Asix PHY driver

Message ID 20231009013912.4048593-4-fujita.tomonori@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Rust abstractions for network PHY drivers | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 16 maintainers not CCed: pabeni@redhat.com lina@asahilina.net linux@armlinux.org.uk benno.lossin@proton.me edumazet@google.com alex.gaynor@gmail.com kuba@kernel.org aliceryhl@google.com bjorn3_gh@protonmail.com gary@garyguo.net davem@davemloft.net yakoyoku@gmail.com a.hindborg@samsung.com hkallweit1@gmail.com boqun.feng@gmail.com wedsonaf@gmail.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 82 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: please write a help paragraph that fully describes the config symbol
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

FUJITA Tomonori Oct. 9, 2023, 1:39 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 <fujita.tomonori@gmail.com>
---
 drivers/net/phy/Kconfig          |   7 ++
 drivers/net/phy/Makefile         |   6 +-
 drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
 rust/uapi/uapi_helper.h          |   2 +
 4 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/ax88796b_rust.rs

Comments

Trevor Gross Oct. 9, 2023, 3:22 a.m. UTC | #1
On Sun, Oct 8, 2023 at 9:41 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> 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 <fujita.tomonori@gmail.com>
> ---

Thanks for adding the speed bindings. The last thing is that I still
think we need to figure out a better way to generate the correct types
for #define values (discussed around [1]) but we can fix that later.

Reviewed-by: Trevor Gross <tmgross@umich.edu>

[1]: https://lore.kernel.org/rust-for-linux/CALNs47sxuGVXBwhXZa5NgHQ8F0MH2OoUzsgthAURE+OuTtJ1wQ@mail.gmail.com/
Jiri Pirko Oct. 9, 2023, 7:23 a.m. UTC | #2
Mon, Oct 09, 2023 at 03:39:12AM CEST, fujita.tomonori@gmail.com wrote:
>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 <fujita.tomonori@gmail.com>
>---
> drivers/net/phy/Kconfig          |   7 ++
> drivers/net/phy/Makefile         |   6 +-
> drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
> rust/uapi/uapi_helper.h          |   2 +
> 4 files changed, 143 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 421d2b62918f..0317be180ac2 100644
>--- a/drivers/net/phy/Kconfig
>+++ b/drivers/net/phy/Kconfig
>@@ -107,6 +107,13 @@ config AX88796B_PHY
> 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
> 	  AX88796B package.
> 
>+config AX88796B_RUST_PHY
>+	bool "Rust version driver for Asix PHYs"
>+	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
>+	help
>+	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
>+	  instead of the C version.
>+
> 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..017f817f6f8d
>--- /dev/null
>+++ b/drivers/net/phy/ax88796b_rust.rs
>@@ -0,0 +1,129 @@
>+// SPDX-License-Identifier: GPL-2.0
>+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
>+
>+//! Rust Asix PHYs driver
>+//!
>+//! C version of this driver: [`drivers/net/phy/ax88796b.c`](./ax88796b.c)

Wait. So you just add rust driver as a duplicate of existing c driver?
What's the point exactly to have 2 drivers for the same thing?



>+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 <fujita.tomonori@gmail.com>",
>+    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.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(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>

What is exactly the reason to change anything in uapi for phy driver?
Should be just kernel api implementation, no?



>-- 
>2.34.1
>
>
Greg Kroah-Hartman Oct. 9, 2023, 10:10 a.m. UTC | #3
On Mon, Oct 09, 2023 at 10:39:12AM +0900, FUJITA Tomonori wrote:
> 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 <fujita.tomonori@gmail.com>
> ---
>  drivers/net/phy/Kconfig          |   7 ++
>  drivers/net/phy/Makefile         |   6 +-
>  drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
>  rust/uapi/uapi_helper.h          |   2 +
>  4 files changed, 143 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 421d2b62918f..0317be180ac2 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -107,6 +107,13 @@ config AX88796B_PHY
>  	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>  	  AX88796B package.
>  
> +config AX88796B_RUST_PHY
> +	bool "Rust version driver for Asix PHYs"
> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
> +	help
> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
> +	  instead of the C version.

This does not properly describe what hardware this driver supports.  And
that's an odd way to describe the module name, but I see none of the
other entries in this file do that either, so maybe the PHY subsystm
doesn't require that?

thanks,

greg k-h
Miguel Ojeda Oct. 9, 2023, 10:58 a.m. UTC | #4
On Mon, Oct 9, 2023 at 9:23 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wait. So you just add rust driver as a duplicate of existing c driver?
> What's the point exactly to have 2 drivers for the same thing?

Please see https://lore.kernel.org/ksummit/CANiq72=99VFE=Ve5MNM9ZuSe9M-JSH1evk6pABNSEnNjK7aXYA@mail.gmail.com/.

Cheers,
Miguel
FUJITA Tomonori Oct. 9, 2023, 11:41 a.m. UTC | #5
On Mon, 9 Oct 2023 09:23:04 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

>>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>
> 
> What is exactly the reason to change anything in uapi for phy driver?
> Should be just kernel api implementation, no?

It doesn't change anything. Like the C PHY drivers do, Rust PHY
drivers use the defined values in header files in uapi like MII_*,
BMCR_*, etc.
Andrew Lunn Oct. 9, 2023, 12:32 p.m. UTC | #6
> Wait. So you just add rust driver as a duplicate of existing c driver?
> What's the point exactly to have 2 drivers for the same thing?

To tell the truth, i don't think the driver itself is very
important. The important thing has been the discussion along the way
to get to a driver.

For me as a Maintainer, the discussion about maintainability, how do
we make the build fail when C and Rust diverge is important.  So it
seems C enum are better than #defines for that. Maybe we will need to
replace some #define lists with enum in core code? But we also have a
lot of #defines which it would be good to be able to use.

It took a while to get the code to actually register the driver
instances. But this is something nearly every driver needs to do, so i
hope the ideas and maybe some of the actual code can be used in other
drivers.

It has become much clearer the Rust build system needs work. It is too
monolithic at the moment, and there are no good examples of kconfig
integration. 

Documentation is still an open question for me. I know Rust can
generate documentation from the code, but how do we integrate that
into the existing kernel documentation?

Within netdev, our own tooling is not ready for Rust. Our patchwork
instance did not recognise the patch was for netdev. That has been
fixed now. But i'm pretty sure the latest version of the code was not
built as part of our build testing. Jakub said the machine did not
have a Rust toolchain.  I also think because of the Rust build system
limitations, even if it did have the tools, i don't think with
allmodconfig it would try to built the rust code.

When i build it on my machine, i get a million warnings from i think
the linker. That is not going to be acceptable to Mainline, so we need
to investigate that.

I hope some sort of lessons learned, best practices and TODO list can
be distilled from the experience, to help guide the Rust Experiment.

	Andrew
Benno Lossin Oct. 9, 2023, 12:42 p.m. UTC | #7
On 09.10.23 03:39, FUJITA Tomonori wrote:
> 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 <fujita.tomonori@gmail.com>
> ---

One small nit below, you can decide what to do with that.

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

>   drivers/net/phy/Kconfig          |   7 ++
>   drivers/net/phy/Makefile         |   6 +-
>   drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
>   rust/uapi/uapi_helper.h          |   2 +
>   4 files changed, 143 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 421d2b62918f..0317be180ac2 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -107,6 +107,13 @@ config AX88796B_PHY
>   	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>   	  AX88796B package.
> 
> +config AX88796B_RUST_PHY
> +	bool "Rust version driver for Asix PHYs"
> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
> +	help
> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
> +	  instead of the C version.
> +
>   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..017f817f6f8d
> --- /dev/null
> +++ b/drivers/net/phy/ax88796b_rust.rs
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
> +
> +//! 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 <fujita.tomonori@gmail.com>",
> +    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.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(uapi::SPEED_100);
> +        } else {
> +            dev.set_speed(uapi::SPEED_10);
> +        }

Maybe refactor to only have one `dev.set_speed` call?

> +
> +        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>
> --
> 2.34.1
> 
>
Andrew Lunn Oct. 9, 2023, 1:15 p.m. UTC | #8
> > +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
> > +            dev.set_speed(uapi::SPEED_100);
> > +        } else {
> > +            dev.set_speed(uapi::SPEED_10);
> > +        }
> 
> Maybe refactor to only have one `dev.set_speed` call?

This is a common pattern in the C code. This is basically a
re-implementation of

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432

because this PHY is broken. Being one of the maintainers of the PHY
subsystem, it helps me review this code if it happens to look like the
existing code it is adding a workaround to.

Is there a Rust reason to only have one call?

   Andrew
Benno Lossin Oct. 9, 2023, 1:45 p.m. UTC | #9
On 09.10.23 15:15, Andrew Lunn wrote:
>>> +        if ret as u32 & uapi::BMCR_SPEED100 != 0 {
>>> +            dev.set_speed(uapi::SPEED_100);
>>> +        } else {
>>> +            dev.set_speed(uapi::SPEED_10);
>>> +        }
>>
>> Maybe refactor to only have one `dev.set_speed` call?
> 
> This is a common pattern in the C code. This is basically a
> re-implementation of
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2432
> 
> because this PHY is broken. Being one of the maintainers of the PHY
> subsystem, it helps me review this code if it happens to look like the
> existing code it is adding a workaround to.
> 
> Is there a Rust reason to only have one call?

My reason was consistency, since the call to `set_duplex`
below that was changed to only have one call:

+        let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 {
+            phy::DuplexMode::Full
+        } else {
+            phy::DuplexMode::Half
+        };
+        dev.set_duplex(duplex);

I think it should be consistent, I chose to reduce the number of
function calls, since it is immediately obvious that only the argument
is depending on the condition. But if you think it should mirror the C
side, then maybe change the duplex back to calling twice?
Miguel Ojeda Oct. 9, 2023, 2:01 p.m. UTC | #10
On Mon, Oct 9, 2023 at 2:32 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> It has become much clearer the Rust build system needs work. It is too
> monolithic at the moment, and there are no good examples of kconfig
> integration.

I am not sure what you mean. As I said elsewhere, this is well-known,
was done on purpose, and we have discussed it publicly for a long
time.

The Rust experiment does not require the new build system -- the goal
is that developers and maintainers can work on abstractions and
drivers and see whether the whole approach is possible or not.

Being able to compile the abstractions themselves as a module is, of
course, a requirement for leaving the experimentation phase.

> Documentation is still an open question for me. I know Rust can
> generate documentation from the code, but how do we integrate that
> into the existing kernel documentation?

I replied to that in the other thread [1]. The integration that we
really need is getting at least linking in the C to Rust direction;
but I suspect what you are asking for is to replace `rustdoc` or
similar from what you are saying. I don't think that would be a good
idea unless someone can get something substantially better than what
`rustdoc` produces.

[1] https://lore.kernel.org/rust-for-linux/CANiq72n3kHrJKXApx2eZ6MFisdoh==4KQi2qHYkxmDi=TYaHew@mail.gmail.com/

> When i build it on my machine, i get a million warnings from i think
> the linker. That is not going to be acceptable to Mainline, so we need
> to investigate that.

Yes, that is (sadly) expected, and I was at a cross-roads between
restricting it in the config or not, as I mentioned in the other
thread [2]. Given your message, let's take it as an opportunity to
change it now.

[2] https://lore.kernel.org/rust-for-linux/CANiq72m849ebmsVbfrPF3=UrjT=vawEyZ1=nSj6XHqAOEEieMQ@mail.gmail.com/

> I hope some sort of lessons learned, best practices and TODO list can
> be distilled from the experience, to help guide the Rust Experiment.

I appreciate that you are taking the time to have a look at the Rust
support, but please note that most things you are mentioning are not
really "lessons learned" -- they were things that were already known
and/or worked on.

On top of that, please note that we are not the ones that decided that
this driver / patch series was ready. In fact, we recommended
iterating it more before submitting it to the Rust for Linux mailing
list, and even less to the networking one. Mostly because there is
still not a driver merged, and things like this can create confusion
as you have seen (and there are others, way more complex, in the
works, but they have had more internal iterations with other Rust
subsystem people, which we appreciated).

Finally, yes, many things are not ready. That is expected, and the
Rust support is still experimental. We are still in the process of
merging things that we were working on in the past years (on the side
of the abstractions) and, on the infrastructure side, there is also a
lot of work to be done.

We never claimed this is ready for production and that we cover every
feature that C has. This includes other kernel subsystems, but also
tooling and external projects: there are things to do in `pahole`
(thanks Arnaldo for working on that), perhaps in `objtool` too (thanks
Josh for being open to work on that), in the Documentation (thanks
Jonathan and Carlos), in Coccinelle (Coccinelle for Rust has just been
publicly published, thanks Julia and Tathagata), in KernelCI (thanks
Guillaume et al. for getting the Rust builds working again last week),
in `rustc_codegen_gcc` (thanks Antoni for having made it work without
source-level patches in the kernel last month), GCC Rust (getting
closer), LKP/Intel 0-day, in the Rust compiler itself (e.g. recent
mitigations), in `bindgen` (e.g. the `enum` thing, the lack of
non-trivial constants support, the always-`u32`-incorrect C type for
`#define`s)...

What I mean to say with all this (and sorry to those I didn't list --
it was just a sample) is: yes, we are getting there, but it will still
take a while. We cannot really do everything at once, and there are
lots of things going on.

I would encourage others to join the weekly meetings to get up to
speed with the status and help getting everything ready. For
"external" projects, I track things in the lists linked at the top of
https://github.com/Rust-for-Linux/linux/issues/2.

Cheers,
Miguel
Andrew Lunn Oct. 9, 2023, 2:31 p.m. UTC | #11
> > I hope some sort of lessons learned, best practices and TODO list can
> > be distilled from the experience, to help guide the Rust Experiment.
> 
> I appreciate that you are taking the time to have a look at the Rust
> support, but please note that most things you are mentioning are not
> really "lessons learned" -- they were things that were already known
> and/or worked on.

We are at the intersect of two worlds here. Maybe these issues are
well known in the linux for rust world, but they are not really known
to the netdev world, and to some extend the kernel developers /
maintainers world. We need to spread knowledge between each world.

So maybe this "lessons learned" is not really for the Rust people, but
for the netdev community, and kernel developers and Maintainers in
general?

	Andrew
Miguel Ojeda Oct. 9, 2023, 3:27 p.m. UTC | #12
On Mon, Oct 9, 2023 at 4:31 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> We are at the intersect of two worlds here. Maybe these issues are
> well known in the linux for rust world, but they are not really known
> to the netdev world, and to some extend the kernel developers /
> maintainers world. We need to spread knowledge between each world.

Indeed!

We are presenting in netdevconf 0x17 in a few weeks a tutorial [1] to
try to cover a bit of that with a tutorial on Rust & networking
(thanks Jamal for inviting us). Hopefully that helps a bit...

[1] https://netdevconf.info/0x17/sessions/tutorial/rust-for-linux-networking-tutorial.html

And, of course, there is LPC & Kangrejos (the latter already happened
3 weeks ago -- we had most of the slides already up at
https://kangrejos.com). We also had some LF Live: Mentorship Series
done in the past.

> So maybe this "lessons learned" is not really for the Rust people, but
> for the netdev community, and kernel developers and Maintainers in
> general?

I apologize -- I didn't mean to say that you should know about those
things. Just that, we are trying to do our best to get things ready in
the best way possible, while letting people "play" meanwhile with the
abstractions and so on.

Cheers,
Miguel
Miguel Ojeda Oct. 9, 2023, 3:35 p.m. UTC | #13
On Mon, Oct 9, 2023 at 5:27 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I apologize -- I didn't mean to say that you should know about those
> things. Just that, we are trying to do our best to get things ready in
> the best way possible, while letting people "play" meanwhile with the
> abstractions and so on.

I forgot to mention: we have been planning a "networking call" for a
while for everybody interested in it in e.g. Google Meet. Initially it
was going to be tomorrow, but we are rescheduling now.

The goal of the call is to get the different parties involved, since
there are quite a few trying to upstream different bits and pieces
around networking. In particular, we want to discuss having
a`rust-net` branch where everybody can work together on the networking
abstractions and iterate there.

So that is another alternative. Of course, the `rust-net` branch could
be in the networking tree instead.

Please feel free to join, you would be very welcome (and anybody else
from netdev, of course).

Cheers,
Miguel
Andrew Lunn Oct. 9, 2023, 4:09 p.m. UTC | #14
> The goal of the call is to get the different parties involved, since
> there are quite a few trying to upstream different bits and pieces
> around networking. In particular, we want to discuss having
> a`rust-net` branch where everybody can work together on the networking
> abstractions and iterate there.
> 
> So that is another alternative. Of course, the `rust-net` branch could
> be in the networking tree instead.

My experience is, you need to use the netdev mailing list. Anything
which is not developed in full few of netdev is very likely to get
ripped to shreds and has no chance of being merged. As an extension to
that, you should be targeting net-next.

The mailing list has multiple purposes, one of which is education. The
netdev community needs to learn about Rust, in the same way the Rust
community needs to learn about netdev. If this experiment is a
success, i expect Rust code to be no different to C code. It gets
posted to netdev, it gets run through the netdev CI, and eventually
merged via net-next.

	Andrew
FUJITA Tomonori Oct. 12, 2023, 11:57 a.m. UTC | #15
On Mon, 9 Oct 2023 12:10:11 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Mon, Oct 09, 2023 at 10:39:12AM +0900, FUJITA Tomonori wrote:
>> 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 <fujita.tomonori@gmail.com>
>> ---
>>  drivers/net/phy/Kconfig          |   7 ++
>>  drivers/net/phy/Makefile         |   6 +-
>>  drivers/net/phy/ax88796b_rust.rs | 129 +++++++++++++++++++++++++++++++
>>  rust/uapi/uapi_helper.h          |   2 +
>>  4 files changed, 143 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 421d2b62918f..0317be180ac2 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -107,6 +107,13 @@ config AX88796B_PHY
>>  	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>>  	  AX88796B package.
>>  
>> +config AX88796B_RUST_PHY
>> +	bool "Rust version driver for Asix PHYs"
>> +	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
>> +	help
>> +	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
>> +	  instead of the C version.
> 
> This does not properly describe what hardware this driver supports.  And

I'll add (by copying the description of the C driver).


> that's an odd way to describe the module name, but I see none of the
> other entries in this file do that either, so maybe the PHY subsystm
> doesn't require that?

I leave it to Andrew. This is the first driver in Rust so I thought
that users had no idea the relationship between the source file name
and the module file name.
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 421d2b62918f..0317be180ac2 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -107,6 +107,13 @@  config AX88796B_PHY
 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
 	  AX88796B package.
 
+config AX88796B_RUST_PHY
+	bool "Rust version driver for Asix PHYs"
+	depends on RUST_PHYLIB_BINDINGS && AX88796B_PHY
+	help
+	  Uses the Rust version driver for Asix PHYs (ax88796b_rust.ko)
+	  instead of the C version.
+
 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..017f817f6f8d
--- /dev/null
+++ b/drivers/net/phy/ax88796b_rust.rs
@@ -0,0 +1,129 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! 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 <fujita.tomonori@gmail.com>",
+    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.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(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>