| 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 |
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/
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 > >
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
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
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.
> 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
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 > >
> > + 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
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?
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
> > 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
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
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
> 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
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 --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>
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