| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZSOqWMqm/JQOieAd@nanopsycho>
Date: Mon, 9 Oct 2023 09:23:04 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
miguel.ojeda.sandonis@...il.com, greg@...ah.com, tmgross@...ch.edu
Subject: Re: [PATCH net-next v3 3/3] net: phy: add Rust Asix PHY driver
Mon, Oct 09, 2023 at 03:39:12AM CEST, fujita.tomonori@...il.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@...il.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@...il.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@...il.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
>
>
Powered by blists - more mailing lists