| Message ID | 20231006094911.3305152-4-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote: > +config AX88796B_RUST_PHY > + bool "Rust reference driver" > + depends on RUST && AX88796B_PHY > + default n Nit, "n" is always the default, there is no need for this line. > + help > + Uses the Rust version driver for Asix PHYs. You need more text here please. Provide a better description of what hardware is supported and the name of the module if it is built aas a module. Also that if you select this one, the C driver will not be built (which is not expressed in the Kconfig language, why not? > + > 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 This can be expressed in Kconfig, no need to put this here, right? > 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..d11c82a9e847 > --- /dev/null > +++ b/drivers/net/phy/ax88796b_rust.rs > @@ -0,0 +1,129 @@ > +// SPDX-License-Identifier: GPL-2.0 No copyright line? Are you sure? thanks, greg k-h
On Fri, 6 Oct 2023 12:31:59 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote: >> +config AX88796B_RUST_PHY >> + bool "Rust reference driver" >> + depends on RUST && AX88796B_PHY >> + default n > > Nit, "n" is always the default, there is no need for this line. Understood, I'll remove this line. >> + help >> + Uses the Rust version driver for Asix PHYs. > > You need more text here please. Provide a better description of what > hardware is supported and the name of the module if it is built aas a > module. > > Also that if you select this one, the C driver will not be built (which > is not expressed in the Kconfig language, why not? Because the way to load a PHY driver module can't handle multiple modules with the same phy id (a NIC driver loads a PHY driver module). There is no machinism to specify which PHY driver module should be loaded when multiple PHY modules have the same phy id (as far as I know). The Kconfig file would be like the following. AX88796B_RUST_PHY depends on AX88796B_PHY so the description of AX88796B_PHY is enough? I'll add the name of the module. config AX88796B_PHY tristate "Asix PHYs" help Currently supports the Asix Electronics PHY found in the X-Surf 100 AX88796B package. config AX88796B_RUST_PHY bool "Rust reference driver" depends on RUST && AX88796B_PHY default n help Uses the Rust version driver for Asix PHYs. >> + >> 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 > > This can be expressed in Kconfig, no need to put this here, right? Not sure. Is it possible? If we allow both modules to be built, I guess it's possible though. >> 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..d11c82a9e847 >> --- /dev/null >> +++ b/drivers/net/phy/ax88796b_rust.rs >> @@ -0,0 +1,129 @@ >> +// SPDX-License-Identifier: GPL-2.0 > > No copyright line? Are you sure? Ah, I'll add.
On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote: > On Fri, 6 Oct 2023 12:31:59 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote: > >> +config AX88796B_RUST_PHY > >> + bool "Rust reference driver" > >> + depends on RUST && AX88796B_PHY > >> + default n > > > > Nit, "n" is always the default, there is no need for this line. > > Understood, I'll remove this line. > > >> + help > >> + Uses the Rust version driver for Asix PHYs. > > > > You need more text here please. Provide a better description of what > > hardware is supported and the name of the module if it is built aas a > > module. > > > > Also that if you select this one, the C driver will not be built (which > > is not expressed in the Kconfig language, why not? > > Because the way to load a PHY driver module can't handle multiple > modules with the same phy id (a NIC driver loads a PHY driver module). > There is no machinism to specify which PHY driver module should be > loaded when multiple PHY modules have the same phy id (as far as I know). Sorry, I know that, I mean I am pretty sure you can express this "one or the other" type of restriction in Kconfig, no need to encode it in the Makefile logic. Try doing "depens on AX88796B_PHY=n" as the dependency for the rust driver. > The Kconfig file would be like the following. AX88796B_RUST_PHY > depends on AX88796B_PHY so the description of AX88796B_PHY is enough? > I'll add the name of the module. > > > config AX88796B_PHY > tristate "Asix PHYs" > help > Currently supports the Asix Electronics PHY found in the X-Surf 100 > AX88796B package. > > config AX88796B_RUST_PHY > bool "Rust reference driver" > depends on RUST && AX88796B_PHY > default n > help > Uses the Rust version driver for Asix PHYs. "This is the rust version of a driver to support... It will be called..." > > >> + > >> 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 > > > > This can be expressed in Kconfig, no need to put this here, right? > > Not sure. Is it possible? If we allow both modules to be built, I > guess it's possible though. see above, this is expressed in the Kconfig language and then no ifdef is needed here at all. thanks, greg k-h
On Fri, 6 Oct 2023 16:12:24 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote: >> On Fri, 6 Oct 2023 12:31:59 +0200 >> Greg KH <gregkh@linuxfoundation.org> wrote: >> >> > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote: >> >> +config AX88796B_RUST_PHY >> >> + bool "Rust reference driver" >> >> + depends on RUST && AX88796B_PHY >> >> + default n >> > >> > Nit, "n" is always the default, there is no need for this line. >> >> Understood, I'll remove this line. >> >> >> + help >> >> + Uses the Rust version driver for Asix PHYs. >> > >> > You need more text here please. Provide a better description of what >> > hardware is supported and the name of the module if it is built aas a >> > module. >> > >> > Also that if you select this one, the C driver will not be built (which >> > is not expressed in the Kconfig language, why not? >> >> Because the way to load a PHY driver module can't handle multiple >> modules with the same phy id (a NIC driver loads a PHY driver module). >> There is no machinism to specify which PHY driver module should be >> loaded when multiple PHY modules have the same phy id (as far as I know). > > Sorry, I know that, I mean I am pretty sure you can express this "one or > the other" type of restriction in Kconfig, no need to encode it in the > Makefile logic. > > Try doing "depens on AX88796B_PHY=n" as the dependency for the rust > driver. You meant the following? config AX88796B_PHY tristate "Asix PHYs" help Currently supports the Asix Electronics PHY found in the X-Surf 100 AX88796B package. config AX88796B_RUST_PHY bool "Rust reference driver" depends on RUST && AX88796B_PHY default n help Uses the Rust version driver for Asix PHYs. The problem is that there are NIC drivers that `select AX88796B_PHY`. the Kconfig language doesn't support something like `select AX88796B_PHY or AX88796B_RUST_PHY`, I guess.
> The Kconfig file would be like the following. AX88796B_RUST_PHY > depends on AX88796B_PHY so the description of AX88796B_PHY is enough? > I'll add the name of the module. > > > config AX88796B_PHY > tristate "Asix PHYs" > help > Currently supports the Asix Electronics PHY found in the X-Surf 100 > AX88796B package. I _think_ you can add depends on !AX88796B_RUST_PHY > config AX88796B_RUST_PHY > bool "Rust reference driver" > depends on RUST && AX88796B_PHY And then this becomes depends on RUST && !AX88796B_PHY > default n > help > Uses the Rust version driver for Asix PHYs. You then express the mutual exclusion in Kconfig, so that only one of AX88796B_PHY and AX88796B_RUST_PHY is ever enabled. I've not actually tried this, so it might not work. Ideally you need to be able disable both, so that you can enable one. There is good documentation in Documentation/kbuild/kconfig-language.rst > >> +ifdef CONFIG_AX88796B_RUST_PHY > >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b_rust.o > >> +else > >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b.o > >> +endif > > > > This can be expressed in Kconfig, no need to put this here, right? > > Not sure. Is it possible? If we allow both modules to be built, I > guess it's possible though. If what i suggested above works, you don't need the ifdef, just list the two drivers are normal and let Kconfig only enable one at most. Or go back to your idea of using choice. Maybe something like choice tristate "AX88796B PHY driver" config CONFIG_AX88796B_PHY bool "C driver" config CONFIG_AX88796B_RUST_PHY bool "Rust driver" depends on RUST endchoice totally untested.... Andrew
On Fri, Oct 06, 2023 at 11:30:54PM +0900, FUJITA Tomonori wrote: > On Fri, 6 Oct 2023 16:12:24 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Fri, Oct 06, 2023 at 10:53:25PM +0900, FUJITA Tomonori wrote: > >> On Fri, 6 Oct 2023 12:31:59 +0200 > >> Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >> > On Fri, Oct 06, 2023 at 06:49:11PM +0900, FUJITA Tomonori wrote: > >> >> +config AX88796B_RUST_PHY > >> >> + bool "Rust reference driver" > >> >> + depends on RUST && AX88796B_PHY > >> >> + default n > >> > > >> > Nit, "n" is always the default, there is no need for this line. > >> > >> Understood, I'll remove this line. > >> > >> >> + help > >> >> + Uses the Rust version driver for Asix PHYs. > >> > > >> > You need more text here please. Provide a better description of what > >> > hardware is supported and the name of the module if it is built aas a > >> > module. > >> > > >> > Also that if you select this one, the C driver will not be built (which > >> > is not expressed in the Kconfig language, why not? > >> > >> Because the way to load a PHY driver module can't handle multiple > >> modules with the same phy id (a NIC driver loads a PHY driver module). > >> There is no machinism to specify which PHY driver module should be > >> loaded when multiple PHY modules have the same phy id (as far as I know). > > > > Sorry, I know that, I mean I am pretty sure you can express this "one or > > the other" type of restriction in Kconfig, no need to encode it in the > > Makefile logic. > > > > Try doing "depens on AX88796B_PHY=n" as the dependency for the rust > > driver. > > You meant the following? > > config AX88796B_PHY > tristate "Asix PHYs" > help > Currently supports the Asix Electronics PHY found in the X-Surf 100 > AX88796B package. > > config AX88796B_RUST_PHY > bool "Rust reference driver" > depends on RUST && AX88796B_PHY > default n > help > Uses the Rust version driver for Asix PHYs. Nope, that's not going to work. And your string is wrong :( > The problem is that there are NIC drivers that `select > AX88796B_PHY`. the Kconfig language doesn't support something like > `select AX88796B_PHY or AX88796B_RUST_PHY`, I guess. You're going to have to figure out which you want to have in the system, so if a NIC driver selects the C version, live with that. This is just the tip of the iceburg of the mess that is "duplicate drivers in the kernel tree" that I keep warning people about. You all are on your own here now, good luck! greg k-h
> config AX88796B_PHY > tristate "Asix PHYs" > help > Currently supports the Asix Electronics PHY found in the X-Surf 100 > AX88796B package. > > config AX88796B_RUST_PHY > bool "Rust reference driver" > depends on RUST && AX88796B_PHY > default n > help > Uses the Rust version driver for Asix PHYs. > > > The problem is that there are NIC drivers that `select > AX88796B_PHY`. the Kconfig language doesn't support something like > `select AX88796B_PHY or AX88796B_RUST_PHY`, I guess. So change AX88796B_PHY to mean any driver for that hardware. And then move the C driver to AX88796B_C_PHY, and add AX88796B_RUST_PHY. All the MAC drivers really cares about is that there is a PHY driver. They don't care if it is written in C, Rust, or SNOBOL. Andrew
On Fri, 6 Oct 2023 16:35:28 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> The Kconfig file would be like the following. AX88796B_RUST_PHY >> depends on AX88796B_PHY so the description of AX88796B_PHY is enough? >> I'll add the name of the module. >> >> >> config AX88796B_PHY >> tristate "Asix PHYs" >> help >> Currently supports the Asix Electronics PHY found in the X-Surf 100 >> AX88796B package. > > I _think_ you can add > > depends on !AX88796B_RUST_PHY > >> config AX88796B_RUST_PHY >> bool "Rust reference driver" >> depends on RUST && AX88796B_PHY > > And then this becomes > > depends on RUST && !AX88796B_PHY > >> default n >> help >> Uses the Rust version driver for Asix PHYs. > > You then express the mutual exclusion in Kconfig, so that only one of > AX88796B_PHY and AX88796B_RUST_PHY is ever enabled. > > I've not actually tried this, so it might not work. Ideally you need > to be able disable both, so that you can enable one. This doesn't work. ubuntu@ip-172-30-47-114:~/git/linux$ make LLVM=1 -j 32 menuconfig drivers/net/phy/Kconfig:111:error: recursive dependency detected! drivers/net/phy/Kconfig:111: symbol AX88796B_RUST_PHY depends on AX88796B_PHY drivers/net/phy/Kconfig:104: symbol AX88796B_PHY depends on AX88796B_RUST_P The following gurantees that only one is built but we hit the `select AX88796B_PHY` problem in my previous mail. config AX88796B_PHY tristate "Asix PHYs" help Currently supports the Asix Electronics PHY found in the X-Surf 100 AX88796B package. config AX88796B_RUST_PHY bool "Rust reference driver" depends on RUST && AX88796B_PHY=n help Uses the Rust version driver for Asix PHYs. Greg, Sorry. I messed up copy-and-paste in the previous mail. I think that you meant the above. > There is good documentation in > > Documentation/kbuild/kconfig-language.rst > >> >> +ifdef CONFIG_AX88796B_RUST_PHY >> >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b_rust.o >> >> +else >> >> + obj-$(CONFIG_AX88796B_PHY) += ax88796b.o >> >> +endif >> > >> > This can be expressed in Kconfig, no need to put this here, right? >> >> Not sure. Is it possible? If we allow both modules to be built, I >> guess it's possible though. > > If what i suggested above works, you don't need the ifdef, just list > the two drivers are normal and let Kconfig only enable one at most. > Or go back to your idea of using choice. Maybe something like > > choice > tristate "AX88796B PHY driver" > > config CONFIG_AX88796B_PHY > bool "C driver" > > config CONFIG_AX88796B_RUST_PHY > bool "Rust driver" > depends on RUST > endchoice > > totally untested.... Now I'm thinking that this is the best option. Kconfig would be the following: config AX88796B_PHY tristate "Asix PHYs" help Currently supports the Asix Electronics PHY found in the X-Surf 100 AX88796B package. choice prompt "Implementation options" depends on AX88796B_PHY help There are two implementations for a driver for Asix PHYs; C and Rust. If not sure, choose C. config AX88796B_C_PHY bool "The C version driver for Asix PHYs" config AX88796B_RUST_PHY bool "The Rust version driver for Asix PHYs" depends on RUST endchoice No hack in Makefile: obj-$(CONFIG_AX88796B_C_PHY) += ax88796b.o obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o
> Now I'm thinking that this is the best option. Kconfig would be the following: > > config AX88796B_PHY > tristate "Asix PHYs" > help > Currently supports the Asix Electronics PHY found in the X-Surf 100 > AX88796B package. > > choice > prompt "Implementation options" > depends on AX88796B_PHY > help > There are two implementations for a driver for Asix PHYs; C and Rust. > If not sure, choose C. > > config AX88796B_C_PHY > bool "The C version driver for Asix PHYs" > > config AX88796B_RUST_PHY > bool "The Rust version driver for Asix PHYs" > depends on RUST > > endchoice > > > No hack in Makefile: > > obj-$(CONFIG_AX88796B_C_PHY) += ax88796b.o > obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o This looks reasonable. Lets use this. But i still think we need some sort of RUST_PHYLIB_BINDING. Andrew
On Fri, 6 Oct 2023 17:57:41 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> Now I'm thinking that this is the best option. Kconfig would be the following: >> >> config AX88796B_PHY >> tristate "Asix PHYs" >> help >> Currently supports the Asix Electronics PHY found in the X-Surf 100 >> AX88796B package. >> >> choice >> prompt "Implementation options" >> depends on AX88796B_PHY >> help >> There are two implementations for a driver for Asix PHYs; C and Rust. >> If not sure, choose C. >> >> config AX88796B_C_PHY >> bool "The C version driver for Asix PHYs" >> >> config AX88796B_RUST_PHY >> bool "The Rust version driver for Asix PHYs" >> depends on RUST >> >> endchoice >> >> >> No hack in Makefile: >> >> obj-$(CONFIG_AX88796B_C_PHY) += ax88796b.o >> obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o > > This looks reasonable. Lets use this. But i still think we need some > sort of RUST_PHYLIB_BINDING. How about adding CONFIG_RUST_PHYLIB to the first patch. Not selectable, it's just a flag for Rust PHYLIB support. diff --git a/init/Kconfig b/init/Kconfig index 4b4e3df1658d..2b6627aeb98c 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1889,7 +1889,7 @@ config RUST depends on !GCC_PLUGINS depends on !RANDSTRUCT depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE depends on PHYLIB=y + select RUST_PHYLIB select CONSTRUCTORS help Enables Rust support in the kernel. @@ -1904,6 +1904,10 @@ config RUST If unsure, say N. +config RUST_PHYLIB + bool + config RUSTC_VERSION_TEXT string depends on RUST Then the driver depends on RUST instead of RUST_PHYLIB. diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 82ecfffc276c..e0d7a19ca774 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -119,7 +119,7 @@ config AX88796B_C_PHY config AX88796B_RUST_PHY bool "The Rust version driver for Asix PHYs" - depends on RUST + depends on RUST_PHYLIB endchoice
> How about adding CONFIG_RUST_PHYLIB to the first patch. Not > selectable, it's just a flag for Rust PHYLIB support. We have to be careful with names. To some extent, CONFIG_PHYLIB means the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the core of phylib written in rust? I doubt that will ever happen, but we are setting a naming scheme here which i expect others will blindly cut/paste. What we actually want is a symbol which represents the Rust binding onto the phylib core. So i think it should have BINDING, or WRAPPER or something like that in the name. > diff --git a/init/Kconfig b/init/Kconfig > index 4b4e3df1658d..2b6627aeb98c 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -1889,7 +1889,7 @@ config RUST > depends on !GCC_PLUGINS > depends on !RANDSTRUCT > depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE > depends on PHYLIB=y > + select RUST_PHYLIB I know the rust build system is rather limited at the moment, but is this required? Is it possible to build the rust code without the phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile target only if it is enabled? > select CONSTRUCTORS > help > Enables Rust support in the kernel. > @@ -1904,6 +1904,10 @@ config RUST > > If unsure, say N. > > +config RUST_PHYLIB > + bool This is where the depends on PHYLIB should be. It is the Rust binding on phylib which has the dependency on phylib, not the core rust code. What i think the end state should be, once the Rust build system is better is that in drivers/net/phy/Kconfig we have: if PHYLIB config RUST_PHYLIB_BINDING bool depends on RUST help Adds support needed for PHY drivers written in Rust. It provides a wrapper around the C phlib core. and the Makefile when uses this to build the binding as a kernel module. Andrew
On Fri, 6 Oct 2023 18:55:23 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> How about adding CONFIG_RUST_PHYLIB to the first patch. Not >> selectable, it's just a flag for Rust PHYLIB support. > > We have to be careful with names. To some extent, CONFIG_PHYLIB means > the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the > core of phylib written in rust? I doubt that will ever happen, but we > are setting a naming scheme here which i expect others will blindly > cut/paste. What we actually want is a symbol which represents the Rust > binding onto the phylib core. So i think it should have BINDING, or > WRAPPER or something like that in the name. Good point. Let's save CONFIG_PHYLIB for someday. >> diff --git a/init/Kconfig b/init/Kconfig >> index 4b4e3df1658d..2b6627aeb98c 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -1889,7 +1889,7 @@ config RUST >> depends on !GCC_PLUGINS >> depends on !RANDSTRUCT >> depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE >> depends on PHYLIB=y >> + select RUST_PHYLIB > > I know the rust build system is rather limited at the moment, but is > this required? Is it possible to build the rust code without the > phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile > target only if it is enabled? A short-term solution could work, I think. config RUST bool "Rust support" depends on HAVE_RUST depends on RUST_IS_AVAILABLE depends on !MODVERSIONS depends on !GCC_PLUGINS depends on !RANDSTRUCT depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE select CONSTRUCTORS help Enables Rust support in the kernel. This allows other Rust-related options, like drivers written in Rust, to be selected. It is also required to be able to load external kernel modules written in Rust. See Documentation/rust/ for more information. If unsure, say N. config RUST_PHYLIB_BINDING bool "PHYLIB bindings support" depends on RUST depends on PHYLIB=y help Adds support needed for PHY drivers written in Rust. It provides a wrapper around the C phlib core. Then we can conditionally build build the PHYLIB bindings. diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs index fbb6d9683012..33fc1531a6c0 100644 --- a/rust/kernel/net.rs +++ b/rust/kernel/net.rs @@ -2,4 +2,5 @@ //! Networking. +#[cfg(CONFIG_RUST_BINDINGS_PHYLIB)] pub mod phy; A long-term solution is under discussion. https://lore.kernel.org/rust-for-linux/20231006155739.246381-1-yakoyoku@gmail.com/
On Sat, Oct 07, 2023 at 08:54:00AM +0900, FUJITA Tomonori wrote: > On Fri, 6 Oct 2023 18:55:23 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > >> How about adding CONFIG_RUST_PHYLIB to the first patch. Not > >> selectable, it's just a flag for Rust PHYLIB support. > > > > We have to be careful with names. To some extent, CONFIG_PHYLIB means > > the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the > > core of phylib written in rust? I doubt that will ever happen, but we > > are setting a naming scheme here which i expect others will blindly > > cut/paste. What we actually want is a symbol which represents the Rust > > binding onto the phylib core. So i think it should have BINDING, or > > WRAPPER or something like that in the name. > > Good point. Let's save CONFIG_PHYLIB for someday. > > > >> diff --git a/init/Kconfig b/init/Kconfig > >> index 4b4e3df1658d..2b6627aeb98c 100644 > >> --- a/init/Kconfig > >> +++ b/init/Kconfig > >> @@ -1889,7 +1889,7 @@ config RUST > >> depends on !GCC_PLUGINS > >> depends on !RANDSTRUCT > >> depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE > >> depends on PHYLIB=y > >> + select RUST_PHYLIB > > > > I know the rust build system is rather limited at the moment, but is > > this required? Is it possible to build the rust code without the > > phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile > > target only if it is enabled? > > A short-term solution could work, I think. > > config RUST > bool "Rust support" > depends on HAVE_RUST > depends on RUST_IS_AVAILABLE > depends on !MODVERSIONS > depends on !GCC_PLUGINS > depends on !RANDSTRUCT > depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE > select CONSTRUCTORS > help > Enables Rust support in the kernel. > > This allows other Rust-related options, like drivers written in Rust, > to be selected. > > It is also required to be able to load external kernel modules > written in Rust. > > See Documentation/rust/ for more information. > > If unsure, say N. > > config RUST_PHYLIB_BINDING > bool "PHYLIB bindings support" > depends on RUST > depends on PHYLIB=y > help > Adds support needed for PHY drivers written in Rust. It provides > a wrapper around the C phlib core. > > > Then we can conditionally build build the PHYLIB bindings. > > diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs > index fbb6d9683012..33fc1531a6c0 100644 > --- a/rust/kernel/net.rs > +++ b/rust/kernel/net.rs > @@ -2,4 +2,5 @@ > > //! Networking. > > +#[cfg(CONFIG_RUST_BINDINGS_PHYLIB)] > pub mod phy; This looks reasonable. If you spin a new version with all these Kconfig changes, i will do some compile testing. Andrew
On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs > new file mode 100644 > index 000000000000..d11c82a9e847 > --- /dev/null > +++ b/drivers/net/phy/ax88796b_rust.rs Maybe want to link to the C version, just for the crossref? > + fn read_status(dev: &mut phy::Device) -> Result<u16> { > + dev.genphy_update_link()?; > + if !dev.get_link() { > + return Ok(0); > + } Looking at this usage, I think `get_link()` should be renamed to just `link()`. `get_link` makes me think that it is performing an action like calling `genphy_update_link`, just `link()` sounds more like a static accessor. Or maybe it's worth replacing `get_link` with a `get_updated_link` that calls `genphy_update_link` and then returns `link`, the user can store it if they need to reuse it. This seems somewhat less accident prone than someone calling `.link()`/`.get_link()` repeatedly and wondering why their phy isn't coming up. In any case, please make the docs clear about what behavior is executed and what the preconditions are, it should be clear what's going to wait for the bus vs. simple field access. > + if ret as u32 & uapi::BMCR_SPEED100 != 0 { > + dev.set_speed(100); > + } else { > + dev.set_speed(10); > + } Speed should probably actually be an enum since it has defined values. Something like #[non_exhaustive] enum Speed { Speed10M, Speed100M, Speed1000M, // 2.5G, 5G, 10G, 25G? } impl Speed { fn as_mb(self) -> u32; } > + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 { > + phy::DuplexMode::Full > + } else { > + phy::DuplexMode::Half > + }; BMCR_x and MII_x are generated as `u32` but that's just a bindgen thing. It seems we should reexport them as the correct types so users don't need to cast all over: pub MII_BMCR: u8 = bindings::MII_BMCR as u8; pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ... // (I'd just make a macro for this) But I'm not sure how to handle that since the uapi crate exposes its bindings directly. We're probably going to run into this issue with other uapi items at some point, any thoughts Miguel? > + dev.genphy_read_lpa()?; Same question as with the `genphy_update_link` > + 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(); > + } > + } > +} Is it worth doing anything with these errors? I know that the C driver doesn't. --- The overall driver looks correct to me, most of these comments are actually about [1/3] - Trevor [1]: https://lore.kernel.org/rust-for-linux/baa4cc4b-bcde-4b02-9286-c923404db972@lunn.ch/
On Fri, 6 Oct 2023 17:57:41 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> Now I'm thinking that this is the best option. Kconfig would be the following: >> >> config AX88796B_PHY >> tristate "Asix PHYs" >> help >> Currently supports the Asix Electronics PHY found in the X-Surf 100 >> AX88796B package. >> >> choice >> prompt "Implementation options" >> depends on AX88796B_PHY >> help >> There are two implementations for a driver for Asix PHYs; C and Rust. >> If not sure, choose C. >> >> config AX88796B_C_PHY >> bool "The C version driver for Asix PHYs" >> >> config AX88796B_RUST_PHY >> bool "The Rust version driver for Asix PHYs" >> depends on RUST >> >> endchoice >> >> >> No hack in Makefile: >> >> obj-$(CONFIG_AX88796B_C_PHY) += ax88796b.o >> obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o > > This looks reasonable. Lets use this. But i still think we need some > sort of RUST_PHYLIB_BINDING. Sorry, I found that it doesn't work well when you try to build AX88796B_PHY as a module. With AX88796B_PHY=m, if you choose the C version, then you got AX88796B_C_PHY=y; the driver will be built-in. Seems that we need a trick in Makefile in any ways, I'll go with the original version of this patch; the simplest, I think.
On Sat, 7 Oct 2023 03:19:20 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > >> diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs >> new file mode 100644 >> index 000000000000..d11c82a9e847 >> --- /dev/null >> +++ b/drivers/net/phy/ax88796b_rust.rs > > Maybe want to link to the C version, just for the crossref? Sure. >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { >> + dev.genphy_update_link()?; >> + if !dev.get_link() { >> + return Ok(0); >> + } > > Looking at this usage, I think `get_link()` should be renamed to just > `link()`. `get_link` makes me think that it is performing an action > like calling `genphy_update_link`, just `link()` sounds more like a > static accessor. Andrew suggested to rename link() to get_link(), I think. Then we discussed again last week: https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/ > Or maybe it's worth replacing `get_link` with a `get_updated_link` > that calls `genphy_update_link` and then returns `link`, the user can > store it if they need to reuse it. This seems somewhat less accident > prone than someone calling `.link()`/`.get_link()` repeatedly and > wondering why their phy isn't coming up. Once this is merged, I'll think about APIs. I need to add more bindings anyway. > In any case, please make the docs clear about what behavior is > executed and what the preconditions are, it should be clear what's > going to wait for the bus vs. simple field access. Sure. >> + if ret as u32 & uapi::BMCR_SPEED100 != 0 { >> + dev.set_speed(100); >> + } else { >> + dev.set_speed(10); >> + } > > Speed should probably actually be an enum since it has defined values. > Something like > > #[non_exhaustive] > enum Speed { > Speed10M, > Speed100M, > Speed1000M, > // 2.5G, 5G, 10G, 25G? > } > > impl Speed { > fn as_mb(self) -> u32; > } > ethtool.h says: /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. * Update drivers/net/phy/phy.c:phy_speed_to_str() and * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values. */ I don't know there are drivers that set such values. >> + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 { >> + phy::DuplexMode::Full >> + } else { >> + phy::DuplexMode::Half >> + }; > > BMCR_x and MII_x are generated as `u32` but that's just a bindgen > thing. It seems we should reexport them as the correct types so users > don't need to cast all over: > > pub MII_BMCR: u8 = bindings::MII_BMCR as u8; > pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ... > // (I'd just make a macro for this) > > But I'm not sure how to handle that since the uapi crate exposes its > bindings directly. We're probably going to run into this issue with > other uapi items at some point, any thoughts Miguel? reexporting all the BMCR_ values by hand doesn't sound fun. Can we automaticall generate such? >> + dev.genphy_read_lpa()?; > > Same question as with the `genphy_update_link` > >> + 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(); >> + } >> + } >> +} > > Is it worth doing anything with these errors? I know that the C driver doesn't. I'll check out what other drivers do in the similar situations. > The overall driver looks correct to me, most of these comments are > actually about [1/3] Thanks!
On Sat, Oct 07, 2023 at 03:19:20AM -0400, Trevor Gross wrote: > On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > diff --git a/drivers/net/phy/ax88796b_rust.rs b/drivers/net/phy/ax88796b_rust.rs > > new file mode 100644 > > index 000000000000..d11c82a9e847 > > --- /dev/null > > +++ b/drivers/net/phy/ax88796b_rust.rs > > Maybe want to link to the C version, just for the crossref? > > > + fn read_status(dev: &mut phy::Device) -> Result<u16> { > > + dev.genphy_update_link()?; > > + if !dev.get_link() { > > + return Ok(0); > > + } > > Looking at this usage, I think `get_link()` should be renamed to just > `link()`. `get_link` makes me think that it is performing an action > like calling `genphy_update_link`, just `link()` sounds more like a > static accessor. Naming is hard, and i had the exact opposite understanding. The rust binding seems to impose getter/setters on members of phydev. So my opinion was, using get_/set_ makes it clear this is just a dumb getter/setter, and nothing more. > Or maybe it's worth replacing `get_link` with a `get_updated_link` > that calls `genphy_update_link` and then returns `link`, the user can > store it if they need to reuse it. This seems somewhat less accident > prone than someone calling `.link()`/`.get_link()` repeatedly and > wondering why their phy isn't coming up. You have to be very careful with reading the link state. It is latched low. Meaning if the link is dropped and then comes back again, the first read of the link will tell you it went away, and the second read will give you current status. The core expects the driver to read the link state only once, when asked what is the state of the link, so it gets informed about this short link down events. > In any case, please make the docs clear about what behavior is > executed and what the preconditions are, it should be clear what's > going to wait for the bus vs. simple field access. > > > + if ret as u32 & uapi::BMCR_SPEED100 != 0 { > > + dev.set_speed(100); > > + } else { > > + dev.set_speed(10); > > + } > > Speed should probably actually be an enum since it has defined values. > Something like > > #[non_exhaustive] > enum Speed { > Speed10M, > Speed100M, > Speed1000M, > // 2.5G, 5G, 10G, 25G? > } This beings us back to how do you make use of C #defines. All the values defined here are theoretically valid: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1887 #define SPEED_10 10 #define SPEED_100 100 #define SPEED_1000 1000 #define SPEED_2500 2500 #define SPEED_5000 5000 #define SPEED_10000 10000 #define SPEED_14000 14000 #define SPEED_20000 20000 #define SPEED_25000 25000 #define SPEED_40000 40000 #define SPEED_50000 50000 #define SPEED_56000 56000 #define SPEED_100000 100000 #define SPEED_200000 200000 #define SPEED_400000 400000 #define SPEED_800000 800000 and more speeds keep getting added. Also, the kAPI actually would allow the value 42, not that any hardware i know of actually supports that. > > + 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(); > > + } > > + } > > +} > > Is it worth doing anything with these errors? I know that the C driver doesn't. You could do a phydev_err(). But if these fail, the hardware is dead, and there is not much you can do about that. Andrew
> reexporting all the BMCR_ values by hand doesn't sound fun. Can we > automaticall generate such? The C22 address space only have a max of 32, and no more are expected. C45 address space can have in theory 32 x 65536, although in practice it is sparsely populated. But new values are added every so often. So generated at build time from the #defines would be better. Andrew
On Sat, Oct 7, 2023 at 8:10 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > >> + fn read_status(dev: &mut phy::Device) -> Result<u16> { > >> + dev.genphy_update_link()?; > >> + if !dev.get_link() { > >> + return Ok(0); > >> + } > > > > Looking at this usage, I think `get_link()` should be renamed to just > > `link()`. `get_link` makes me think that it is performing an action > > like calling `genphy_update_link`, just `link()` sounds more like a > > static accessor. > > Andrew suggested to rename link() to get_link(), I think. > > Then we discussed again last week: > > https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/ Thanks for the link, in that case LGTM > > >> + if ret as u32 & uapi::BMCR_SPEED100 != 0 { > >> + dev.set_speed(100); > >> + } else { > >> + dev.set_speed(10); > >> + } > > > > Speed should probably actually be an enum since it has defined values. > > Something like > > > > #[non_exhaustive] > > enum Speed { > > Speed10M, > > Speed100M, > > Speed1000M, > > // 2.5G, 5G, 10G, 25G? > > } > > > > impl Speed { > > fn as_mb(self) -> u32; > > } > > > > ethtool.h says: > > /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. > * Update drivers/net/phy/phy.c:phy_speed_to_str() and > * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values. > */ > > I don't know there are drivers that set such values. Andrew replied to this too and an enum wouldn't work. Maybe good to add uapi/linux/ethtool.h to the bindings and use the SPEED_X defined there? > >> + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 { > >> + phy::DuplexMode::Full > >> + } else { > >> + phy::DuplexMode::Half > >> + }; > > > > BMCR_x and MII_x are generated as `u32` but that's just a bindgen > > thing. It seems we should reexport them as the correct types so users > > don't need to cast all over: > > > > pub MII_BMCR: u8 = bindings::MII_BMCR as u8; > > pub BMCR_RESV: u16 = bindings::BMCR_RESV as u16; ... > > // (I'd just make a macro for this) > > > > But I'm not sure how to handle that since the uapi crate exposes its > > bindings directly. We're probably going to run into this issue with > > other uapi items at some point, any thoughts Miguel? > > reexporting all the BMCR_ values by hand doesn't sound fun. Can we > automaticall generate such? Definitely not by hand, I don't think bindgen allows finer control over what types are created from `#define` yet. I am not sure what our policy is on build scripts but the below would work: # repeat this with a different prefix (BMCR) and type (u16) as needed perl -ne 'print if s/^#define\s+(BMCR\w+)\s+([0-9xX]+)\s+(?:\/\*(.*)\*\/)?/\/\/\/ \3\npub const \1: u16 = \2;/' include/uapi/linux/mii.h > somefile.rs That creates outputs /// MSB of Speed (1000) pub const BMCR_SPEED1000: u16 = 0x0040; /// Collision test pub const BMCR_CTST: u16 = 0x0080; /// Full duplex pub const BMCR_FULLDPLX: u16 = 0x0100; Miguel, any suggestions here?
On Sat, Oct 7, 2023 at 11:35 AM Andrew Lunn <andrew@lunn.ch> wrote: > > Looking at this usage, I think `get_link()` should be renamed to just > > `link()`. `get_link` makes me think that it is performing an action > > like calling `genphy_update_link`, just `link()` sounds more like a > > static accessor. > > Naming is hard, and i had the exact opposite understanding. > > The rust binding seems to impose getter/setters on members of > phydev. So my opinion was, using get_/set_ makes it clear this is just > a dumb getter/setter, and nothing more. > > > Or maybe it's worth replacing `get_link` with a `get_updated_link` > > that calls `genphy_update_link` and then returns `link`, the user can > > store it if they need to reuse it. This seems somewhat less accident > > prone than someone calling `.link()`/`.get_link()` repeatedly and > > wondering why their phy isn't coming up. > > You have to be very careful with reading the link state. It is latched > low. Meaning if the link is dropped and then comes back again, the > first read of the link will tell you it went away, and the second read > will give you current status. The core expects the driver to read the > link state only once, when asked what is the state of the link, so it > gets informed about this short link down events. Thanks for the clarification, I agree that it makes sense as-is then. > > In any case, please make the docs clear about what behavior is > > executed and what the preconditions are, it should be clear what's > > going to wait for the bus vs. simple field access. > > > > > + if ret as u32 & uapi::BMCR_SPEED100 != 0 { > > > + dev.set_speed(100); > > > + } else { > > > + dev.set_speed(10); > > > + } > > > > Speed should probably actually be an enum since it has defined values. > > Something like > > > > #[non_exhaustive] > > enum Speed { > > Speed10M, > > Speed100M, > > Speed1000M, > > // 2.5G, 5G, 10G, 25G? > > } > > This beings us back to how do you make use of C #defines. All the > values defined here are theoretically valid: > > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1887 > > #define SPEED_10 10 > #define SPEED_100 100 > #define SPEED_1000 1000 > #define SPEED_2500 2500 > #define SPEED_5000 5000 > #define SPEED_10000 10000 > #define SPEED_14000 14000 > #define SPEED_20000 20000 > #define SPEED_25000 25000 > #define SPEED_40000 40000 > #define SPEED_50000 50000 > #define SPEED_56000 56000 > #define SPEED_100000 100000 > #define SPEED_200000 200000 > #define SPEED_400000 400000 > #define SPEED_800000 800000 > > and more speeds keep getting added. > > Also, the kAPI actually would allow the value 42, not that any > hardware i know of actually supports that. The enum doesn't make sense but maybe we should just generate bindings for these defines? I suggested that in my reply to Fujita. - Trevor
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 107880d13d21..e4d941f0ebe4 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 reference driver" + depends on RUST && AX88796B_PHY + default n + help + Uses the Rust version driver for Asix PHYs. + 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..d11c82a9e847 --- /dev/null +++ b/drivers/net/phy/ax88796b_rust.rs @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Rust Asix PHYs driver +use kernel::c_str; +use kernel::net::phy::{self, DeviceId, Driver}; +use kernel::prelude::*; +use kernel::uapi; + +kernel::module_phy_driver! { + drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B], + device_table: [ + DeviceId::new_with_driver::<PhyAX88772A>(), + DeviceId::new_with_driver::<PhyAX88772C>(), + DeviceId::new_with_driver::<PhyAX88796B>() + ], + type: RustAsixPhy, + name: "rust_asix_phy", + author: "FUJITA Tomonori <fujita.tomonori@gmail.com>", + description: "Rust Asix PHYs driver", + license: "GPL", +} + +struct RustAsixPhy; + +// Performs a software PHY reset using the standard +// BMCR_RESET bit and poll for the reset bit to be cleared. +// Toggle BMCR_RESET bit off to accommodate broken AX8796B PHY implementation +// such as used on the Individual Computers' X-Surf 100 Zorro card. +fn 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(100); + } else { + dev.set_speed(10); + } + + let duplex = if ret as u32 & uapi::BMCR_FULLDPLX != 0 { + phy::DuplexMode::Full + } else { + phy::DuplexMode::Half + }; + dev.set_duplex(duplex); + + dev.genphy_read_lpa()?; + + if dev.is_autoneg_enabled() && dev.is_autoneg_completed() { + dev.resolve_aneg_linkmode(); + } + + Ok(0) + } + + fn suspend(dev: &mut phy::Device) -> Result { + dev.genphy_suspend() + } + + fn resume(dev: &mut phy::Device) -> Result { + dev.genphy_resume() + } + + fn soft_reset(dev: &mut phy::Device) -> Result { + 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..d92abe9064c2 100644 --- a/rust/uapi/uapi_helper.h +++ b/rust/uapi/uapi_helper.h @@ -7,3 +7,4 @@ */ #include <uapi/asm-generic/ioctl.h> +#include <uapi/linux/mii.h>
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 | 1 + 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 drivers/net/phy/ax88796b_rust.rs