diff mbox series

[net-next,v10,1/4] rust: core abstractions for network PHY drivers

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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 11 maintainers not CCed: a.hindborg@samsung.com yakoyoku@gmail.com aakashsensharma@gmail.com hkallweit1@gmail.com bjorn3_gh@protonmail.com alex.gaynor@gmail.com kuba@kernel.org pabeni@redhat.com linux@armlinux.org.uk gary@garyguo.net edumazet@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 110 exceeds 80 columns WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns WARNING: please write a help paragraph that fully describes the config symbol
netdev/build_clang_rust fail Errors and warnings before: 193 this patch: 193
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

FUJITA Tomonori Dec. 10, 2023, 11:49 p.m. UTC
This patch adds abstractions to implement network PHY drivers; the
driver registration and bindings for some of callback functions in
struct phy_driver and many genphy_ functions.

This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.

This patch enables unstable const_maybe_uninit_zeroed feature for
kernel crate to enable unsafe code to handle a constant value with
uninitialized data. With the feature, the abstractions can initialize
a phy_driver structure with zero easily; instead of initializing all
the members by hand. It's supposed to be stable in the not so distant
future.

Link: https://github.com/rust-lang/rust/pull/116218

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/Kconfig         |   8 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 754 ++++++++++++++++++++++++++++++++
 5 files changed, 774 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

Comments

Andrew Lunn Dec. 11, 2023, 2:01 p.m. UTC | #1
On Mon, Dec 11, 2023 at 08:49:21AM +0900, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Alice Ryhl Dec. 11, 2023, 9:46 p.m. UTC | #2
On 12/11/23 00:49, FUJITA Tomonori wrote:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Overall looks fine to me. Just a few comments below that confuse me:

> +    /// Gets the state of PHY state machine states.
> +    pub fn state(&self) -> DeviceState {
> +        let phydev = self.0.get();
> +        // SAFETY: The struct invariant ensures that we may access
> +        // this field without additional synchronization.
> +        let state = unsafe { (*phydev).state };
> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
> +        // when it becomes possible.
> +        // better to call WARN_ONCE() when the state is out-of-range.

Did you mix up two comments here? This doesn't parse in my brain.
> +    /// Reads a given C22 PHY register.
> +    // This function reads a hardware register and updates the stats so takes `&mut self`.
> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.

This sentence also doesn't parse in my brain. Perhaps "So it's just an 
FFI call" or similar?

Alice
FUJITA Tomonori Dec. 11, 2023, 11:15 p.m. UTC | #3
On Mon, 11 Dec 2023 22:46:01 +0100
Alice Ryhl <alice@ryhl.io> wrote:

> On 12/11/23 00:49, FUJITA Tomonori wrote:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
>> This patch enables unstable const_maybe_uninit_zeroed feature for
>> kernel crate to enable unsafe code to handle a constant value with
>> uninitialized data. With the feature, the abstractions can initialize
>> a phy_driver structure with zero easily; instead of initializing all
>> the members by hand. It's supposed to be stable in the not so distant
>> future.
>> Link: https://github.com/rust-lang/rust/pull/116218
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Overall looks fine to me. Just a few comments below that confuse me:

Thanks.

>> +    /// Gets the state of PHY state machine states.
>> +    pub fn state(&self) -> DeviceState {
>> +        let phydev = self.0.get();
>> +        // SAFETY: The struct invariant ensures that we may access
>> +        // this field without additional synchronization.
>> +        let state = unsafe { (*phydev).state };
>> + // TODO: this conversion code will be replaced with automatically
>> generated code by bindgen
>> +        // when it becomes possible.
>> +        // better to call WARN_ONCE() when the state is out-of-range.
> 
> Did you mix up two comments here? This doesn't parse in my brain.

I'll remove the second comment because all we have to do here is using
bindgen.


>> +    /// Reads a given C22 PHY register.
>> + // This function reads a hardware register and updates the stats so
>> takes `&mut self`.
>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> +        let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type
>> invariant of `Self`.
>> +        // So an FFI call with a valid pointer.
> 
> This sentence also doesn't parse in my brain. Perhaps "So it's just an
> FFI call" or similar?

"So it's just an FFI call" looks good. I'll fix all the places that
use the same comment.
Boqun Feng Dec. 11, 2023, 11:40 p.m. UTC | #4
On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
[...]
> >> +    /// Reads a given C22 PHY register.
> >> + // This function reads a hardware register and updates the stats so
> >> takes `&mut self`.
> >> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> >> +        let phydev = self.0.get();
> >> + // SAFETY: `phydev` is pointing to a valid object by the type
> >> invariant of `Self`.
> >> +        // So an FFI call with a valid pointer.
> > 
> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> > FFI call" or similar?
> 
> "So it's just an FFI call" looks good. I'll fix all the places that
> use the same comment.

I would also mention that `(*phydev).mdio.addr` is smaller than
PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
comments are supposed to describe why calling the C function won't cause
memory safety issues..

Regards,
Boqun
FUJITA Tomonori Dec. 11, 2023, 11:47 p.m. UTC | #5
On Mon, 11 Dec 2023 15:40:33 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
> [...]
>> >> +    /// Reads a given C22 PHY register.
>> >> + // This function reads a hardware register and updates the stats so
>> >> takes `&mut self`.
>> >> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> >> +        let phydev = self.0.get();
>> >> + // SAFETY: `phydev` is pointing to a valid object by the type
>> >> invariant of `Self`.
>> >> +        // So an FFI call with a valid pointer.
>> > 
>> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
>> > FFI call" or similar?
>> 
>> "So it's just an FFI call" looks good. I'll fix all the places that
>> use the same comment.
> 
> I would also mention that `(*phydev).mdio.addr` is smaller than
> PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
> mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
> comments are supposed to describe why calling the C function won't cause
> memory safety issues..

(*phydev).mdio.addr is managed in the C side and Rust code doesn't
touch it (doesn't need to know anything about it). What safety comment
should be written here?
Boqun Feng Dec. 12, 2023, 12:49 a.m. UTC | #6
On Tue, Dec 12, 2023 at 08:47:53AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 15:40:33 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> >> +    /// Reads a given C22 PHY register.
> >> >> + // This function reads a hardware register and updates the stats so
> >> >> takes `&mut self`.
> >> >> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> >> >> +        let phydev = self.0.get();
> >> >> + // SAFETY: `phydev` is pointing to a valid object by the type
> >> >> invariant of `Self`.
> >> >> +        // So an FFI call with a valid pointer.
> >> > 
> >> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> >> > FFI call" or similar?
> >> 
> >> "So it's just an FFI call" looks good. I'll fix all the places that
> >> use the same comment.
> > 
> > I would also mention that `(*phydev).mdio.addr` is smaller than
> > PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
> > mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
> > comments are supposed to describe why calling the C function won't cause
> > memory safety issues..
> 
> (*phydev).mdio.addr is managed in the C side and Rust code doesn't

It's OK to rely on C side to give a correct addr value.

> touch it (doesn't need to know anything about it). What safety comment
> should be written here?

Basically, here Rust just does the same as C does in phy_read(), right?
So why phy_read() is implemented correctly, because C side maintains the
`(*phydev).mdio.addr` in that way. We ususally don't call it out in C
code, since it's obvious(TM), and there is no safe/unsafe boundary in C
side. But in Rust code, that matters. Yes, Rust doesn't control the
value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
words, as long as C side holds the invariants, calling mdiobus_read() is
safe here. How about 

// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also `(*phydev).mdio` is totally maintained by C in a way
// that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
// `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
// `mdiobus_read`.

?

Regards,
Boqun
FUJITA Tomonori Dec. 12, 2023, 1:46 a.m. UTC | #7
On Mon, 11 Dec 2023 16:49:39 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> touch it (doesn't need to know anything about it). What safety comment
>> should be written here?
> 
> Basically, here Rust just does the same as C does in phy_read(), right?
> So why phy_read() is implemented correctly, because C side maintains the
> `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> side. But in Rust code, that matters. Yes, Rust doesn't control the
> value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> words, as long as C side holds the invariants, calling mdiobus_read() is
> safe here. How about 
> 
> // SAFETY: `phydev` points to valid object per the type invariant of
> // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> // `mdiobus_read`.

I thought that "`phydev` is pointing to a valid object by the type
invariant of `Self`." comment implies that "C side holds the invariants"

Do we need a comment about the C implementation details like
PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
side because the C code is changed any time.
Boqun Feng Dec. 12, 2023, 2:30 a.m. UTC | #8
On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 16:49:39 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> >> touch it (doesn't need to know anything about it). What safety comment
> >> should be written here?
> > 
> > Basically, here Rust just does the same as C does in phy_read(), right?
> > So why phy_read() is implemented correctly, because C side maintains the
> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> > side. But in Rust code, that matters. Yes, Rust doesn't control the
> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> > words, as long as C side holds the invariants, calling mdiobus_read() is
> > safe here. How about 
> > 
> > // SAFETY: `phydev` points to valid object per the type invariant of
> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> > // `mdiobus_read`.
> 
> I thought that "`phydev` is pointing to a valid object by the type
> invariant of `Self`." comment implies that "C side holds the invariants"
> 

By the type invariant of `Self`, you mean:

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that you are in
/// a context where all methods defined on this struct are safe to call.

my read on that only tells me the context is guaranteed to be in a
driver callback, nothing has been said about all other invariants C side
should hold.

> Do we need a comment about the C implementation details like
> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
> side because the C code is changed any time.

Well, exactly, "the C code is changed any time", I thought having more
information in Rust helps people who is going to change the C side to
see whether they may break Rust side. Plus it's the safety comment, you
need to prove that it's safe to call the function, the function is
unsafe for a reason: there are inputs that may cause issues, and writing
the safety comment is a process to think and double check.

Maybe we can simplify this a little bit, since IIUC, you just want to
call phy_read() here, but due to that Rust cannot call inline C
functions directly, hence the open-code. How about:


// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also the following just minics what `phy_read()` does in C
// side, which should be safe as long as `phydev` is valid.

?

Regards,
Boqun
FUJITA Tomonori Dec. 12, 2023, 4:04 a.m. UTC | #9
On Mon, 11 Dec 2023 18:30:36 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
>> On Mon, 11 Dec 2023 16:49:39 -0800
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> >> touch it (doesn't need to know anything about it). What safety comment
>> >> should be written here?
>> > 
>> > Basically, here Rust just does the same as C does in phy_read(), right?
>> > So why phy_read() is implemented correctly, because C side maintains the
>> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
>> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
>> > side. But in Rust code, that matters. Yes, Rust doesn't control the
>> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
>> > words, as long as C side holds the invariants, calling mdiobus_read() is
>> > safe here. How about 
>> > 
>> > // SAFETY: `phydev` points to valid object per the type invariant of
>> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
>> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
>> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
>> > // `mdiobus_read`.
>> 
>> I thought that "`phydev` is pointing to a valid object by the type
>> invariant of `Self`." comment implies that "C side holds the invariants"
>> 
> 
> By the type invariant of `Self`, you mean:
> 
> /// # Invariants
> ///
> /// Referencing a `phy_device` using this struct asserts that you are in
> /// a context where all methods defined on this struct are safe to call.
> 
> my read on that only tells me the context is guaranteed to be in a
> driver callback, nothing has been said about all other invariants C side
> should hold.

I meant that phydev points to a valid object, thus mdio and mdio.addr
do too. But after reading you phy_read() comment, I suspect that you
aren't talking about if mdio.addr points to valid memory or not. Your
point is about the validity of calling mdiobus_read() with mdio.addr?


>> Do we need a comment about the C implementation details like
>> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
>> side because the C code is changed any time.
> 
> Well, exactly, "the C code is changed any time", I thought having more
> information in Rust helps people who is going to change the C side to
> see whether they may break Rust side. Plus it's the safety comment, you

The C side people read the Rust code before changing the C code? Let's
see. 


> need to prove that it's safe to call the function, the function is
> unsafe for a reason: there are inputs that may cause issues, and writing
> the safety comment is a process to think and double check.
> 
> Maybe we can simplify this a little bit, since IIUC, you just want to
> call phy_read() here, but due to that Rust cannot call inline C
> functions directly, hence the open-code. How about:

Yeah, I hope that the discussion about inline C functions would end
with a solution.


> // SAFETY: `phydev` points to valid object per the type invariant of
> // `Self`, also the following just minics what `phy_read()` does in C
> // side, which should be safe as long as `phydev` is valid.
> 
> ?

Looks ok to me but after a quick look at in-tree Rust code, I can't
find a comment like X is valid for the first argument in this C
function. What I found are comments like X points to valid memory.
Boqun Feng Dec. 12, 2023, 6:11 a.m. UTC | #10
On Tue, Dec 12, 2023 at 01:04:10PM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 18:30:36 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
> >> On Mon, 11 Dec 2023 16:49:39 -0800
> >> Boqun Feng <boqun.feng@gmail.com> wrote:
> >> 
> >> >> touch it (doesn't need to know anything about it). What safety comment
> >> >> should be written here?
> >> > 
> >> > Basically, here Rust just does the same as C does in phy_read(), right?
> >> > So why phy_read() is implemented correctly, because C side maintains the
> >> > `(*phydev).mdio.addr` in that way. We ususally don't call it out in C
> >> > code, since it's obvious(TM), and there is no safe/unsafe boundary in C
> >> > side. But in Rust code, that matters. Yes, Rust doesn't control the
> >> > value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
> >> > words, as long as C side holds the invariants, calling mdiobus_read() is
> >> > safe here. How about 
> >> > 
> >> > // SAFETY: `phydev` points to valid object per the type invariant of
> >> > // `Self`, also `(*phydev).mdio` is totally maintained by C in a way
> >> > // that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
> >> > // `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
> >> > // `mdiobus_read`.
> >> 
> >> I thought that "`phydev` is pointing to a valid object by the type
> >> invariant of `Self`." comment implies that "C side holds the invariants"
> >> 
> > 
> > By the type invariant of `Self`, you mean:
> > 
> > /// # Invariants
> > ///
> > /// Referencing a `phy_device` using this struct asserts that you are in
> > /// a context where all methods defined on this struct are safe to call.
> > 
> > my read on that only tells me the context is guaranteed to be in a
> > driver callback, nothing has been said about all other invariants C side
> > should hold.
> 
> I meant that phydev points to a valid object, thus mdio and mdio.addr
> do too. But after reading you phy_read() comment, I suspect that you
> aren't talking about if mdio.addr points to valid memory or not. Your
> point is about the validity of calling mdiobus_read() with mdio.addr?
> 

Yes, I was talking about the safety comment for calling mdiobus_read().

> 
> >> Do we need a comment about the C implementation details like
> >> PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
> >> side because the C code is changed any time.
> > 
> > Well, exactly, "the C code is changed any time", I thought having more
> > information in Rust helps people who is going to change the C side to
> > see whether they may break Rust side. Plus it's the safety comment, you
> 
> The C side people read the Rust code before changing the C code? Let's
> see. 
> 

Hmm... I usually won't call someone "C side people". I mean, the project
has C part and Rust part, but the community is one.

In case of myself, I write both C and Rust, if I'm going to change some
C side function, I may want to see the usage at Rust side, especially
whether my changes could break the safety, and safety comments may be
important.

> 
> > need to prove that it's safe to call the function, the function is
> > unsafe for a reason: there are inputs that may cause issues, and writing
> > the safety comment is a process to think and double check.
> > 
> > Maybe we can simplify this a little bit, since IIUC, you just want to
> > call phy_read() here, but due to that Rust cannot call inline C
> > functions directly, hence the open-code. How about:
> 
> Yeah, I hope that the discussion about inline C functions would end
> with a solution.
> 
> 
> > // SAFETY: `phydev` points to valid object per the type invariant of
> > // `Self`, also the following just minics what `phy_read()` does in C
> > // side, which should be safe as long as `phydev` is valid.
> > 
> > ?
> 
> Looks ok to me but after a quick look at in-tree Rust code, I can't
> find a comment like X is valid for the first argument in this C
> function. What I found are comments like X points to valid memory.

Hmm.. maybe "is valid" could be a confusing term, so the point is: if
`phydev` is pointing to a properly maintained struct phy_device, then an
open code of phy_read() should be safe. Maybe "..., which should be safe
as long as `phydev` points to a valid struct phy_device" ?

Regards,
Boqun
Alice Ryhl Dec. 12, 2023, 9:23 a.m. UTC | #11
On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Mon, 11 Dec 2023 22:46:01 +0100
> Alice Ryhl <alice@ryhl.io> wrote:
> >> +    /// Gets the state of PHY state machine states.
> >> +    pub fn state(&self) -> DeviceState {
> >> +        let phydev = self.0.get();
> >> +        // SAFETY: The struct invariant ensures that we may access
> >> +        // this field without additional synchronization.
> >> +        let state = unsafe { (*phydev).state };
> >> + // TODO: this conversion code will be replaced with automatically
> >> generated code by bindgen
> >> +        // when it becomes possible.
> >> +        // better to call WARN_ONCE() when the state is out-of-range.
> >
> > Did you mix up two comments here? This doesn't parse in my brain.
>
> I'll remove the second comment because all we have to do here is using
> bindgen.
>
>
> >> +    /// Reads a given C22 PHY register.
> >> + // This function reads a hardware register and updates the stats so
> >> takes `&mut self`.
> >> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> >> +        let phydev = self.0.get();
> >> + // SAFETY: `phydev` is pointing to a valid object by the type
> >> invariant of `Self`.
> >> +        // So an FFI call with a valid pointer.
> >
> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
> > FFI call" or similar?
>
> "So it's just an FFI call" looks good. I'll fix all the places that
> use the same comment.

If you make those two comment changes, then you can add

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
FUJITA Tomonori Dec. 12, 2023, 10:56 a.m. UTC | #12
On Tue, 12 Dec 2023 10:23:30 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Dec 12, 2023 at 12:15 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Mon, 11 Dec 2023 22:46:01 +0100
>> Alice Ryhl <alice@ryhl.io> wrote:
>> >> +    /// Gets the state of PHY state machine states.
>> >> +    pub fn state(&self) -> DeviceState {
>> >> +        let phydev = self.0.get();
>> >> +        // SAFETY: The struct invariant ensures that we may access
>> >> +        // this field without additional synchronization.
>> >> +        let state = unsafe { (*phydev).state };
>> >> + // TODO: this conversion code will be replaced with automatically
>> >> generated code by bindgen
>> >> +        // when it becomes possible.
>> >> +        // better to call WARN_ONCE() when the state is out-of-range.
>> >
>> > Did you mix up two comments here? This doesn't parse in my brain.
>>
>> I'll remove the second comment because all we have to do here is using
>> bindgen.
>>
>>
>> >> +    /// Reads a given C22 PHY register.
>> >> + // This function reads a hardware register and updates the stats so
>> >> takes `&mut self`.
>> >> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> >> +        let phydev = self.0.get();
>> >> + // SAFETY: `phydev` is pointing to a valid object by the type
>> >> invariant of `Self`.
>> >> +        // So an FFI call with a valid pointer.
>> >
>> > This sentence also doesn't parse in my brain. Perhaps "So it's just an
>> > FFI call" or similar?
>>
>> "So it's just an FFI call" looks good. I'll fix all the places that
>> use the same comment.
> 
> If you make those two comment changes, then you can add
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

I will, thanks!
Miguel Ojeda Dec. 12, 2023, 12:55 p.m. UTC | #13
On Tue, Dec 12, 2023 at 5:04 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> The C side people read the Rust code before changing the C code? Let's
> see.

In the beginning, it is true that developers may forget often. But it
is still the case that they need to be mindful of the Rust side, just
like when changing some C function you need to take care of changing
its callers. That is also why we want to have maintainers of each
subsystem onboard.

In some cases it may be useful to annotate/mark the C side with a
comment warning about it (or similar) if you think it is likely that a
change on the C side will go unnoticed.

Cheers,
Miguel
FUJITA Tomonori Dec. 12, 2023, 1:02 p.m. UTC | #14
On Mon, 11 Dec 2023 22:11:15 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> > // SAFETY: `phydev` points to valid object per the type invariant of
>> > // `Self`, also the following just minics what `phy_read()` does in C
>> > // side, which should be safe as long as `phydev` is valid.
>> > 
>> > ?
>> 
>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>> find a comment like X is valid for the first argument in this C
>> function. What I found are comments like X points to valid memory.
> 
> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
> `phydev` is pointing to a properly maintained struct phy_device, then an
> open code of phy_read() should be safe. Maybe "..., which should be safe
> as long as `phydev` points to a valid struct phy_device" ?

As Alice suggested, I updated the comment. The current comment is:

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call.
let ret = unsafe {
    bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
};

If phy_read() is called here, I assume that you are happy about the
above comment. The way to call mdiobus_read() here is safe because it
just an open code of phy_read(). Simply adding it works for you?

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call, open code of `phy_read()`.
Benno Lossin Dec. 12, 2023, 5:35 p.m. UTC | #15
On 12/12/23 14:02, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 22:11:15 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>> // side, which should be safe as long as `phydev` is valid.
>>>>
>>>> ?
>>>
>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>> find a comment like X is valid for the first argument in this C
>>> function. What I found are comments like X points to valid memory.
>>
>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>> `phydev` is pointing to a properly maintained struct phy_device, then an
>> open code of phy_read() should be safe. Maybe "..., which should be safe
>> as long as `phydev` points to a valid struct phy_device" ?
> 
> As Alice suggested, I updated the comment. The current comment is:
> 
> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> // So it's just an FFI call.
> let ret = unsafe {
>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> };

I still think you need to justify why `mdio.bus` is a pointer that you
can give to `midobus_read`. After looking at the C code, it seems like
that the pointer needs to point to a valid `struct mii_bus`.
This *could* just be an invariant of `struct phy_device` [1], but where
do we document that?

We could make an exception here and treat this differently until bindgen
can handle the `static inline` functions, but I am not so sure if we
want to have this as a general pattern. We need to discuss this more.


[1]: Technically it is a combination of the following invariants:
- the `mdio` field of `struct phy_device` is a valid `struct mido_device`
- the `bus` field of `struct mdio_device` is a valid pointer to a valid
  `struct mii_bus`.

> If phy_read() is called here, I assume that you are happy about the
> above comment. The way to call mdiobus_read() here is safe because it
> just an open code of phy_read(). Simply adding it works for you?
> 
> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> // So it's just an FFI call, open code of `phy_read()`.

This would be fine if we decide to go with the exception I detailed
above. Although instead of "open code" I would write "see implementation
of `phy_read()`".
Boqun Feng Dec. 12, 2023, 8:23 p.m. UTC | #16
On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
> On 12/12/23 14:02, FUJITA Tomonori wrote:
> > On Mon, 11 Dec 2023 22:11:15 -0800
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> >>>> // SAFETY: `phydev` points to valid object per the type invariant of
> >>>> // `Self`, also the following just minics what `phy_read()` does in C
> >>>> // side, which should be safe as long as `phydev` is valid.
> >>>>
> >>>> ?
> >>>
> >>> Looks ok to me but after a quick look at in-tree Rust code, I can't
> >>> find a comment like X is valid for the first argument in this C
> >>> function. What I found are comments like X points to valid memory.
> >>
> >> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
> >> `phydev` is pointing to a properly maintained struct phy_device, then an
> >> open code of phy_read() should be safe. Maybe "..., which should be safe
> >> as long as `phydev` points to a valid struct phy_device" ?
> > 
> > As Alice suggested, I updated the comment. The current comment is:
> > 
> > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > // So it's just an FFI call.
> > let ret = unsafe {
> >     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> > };
> 
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`. After looking at the C code, it seems like
> that the pointer needs to point to a valid `struct mii_bus`.
> This *could* just be an invariant of `struct phy_device` [1], but where
> do we document that?

Yeah, it's better if we call it out in the type invariant.

> 
> We could make an exception here and treat this differently until bindgen
> can handle the `static inline` functions, but I am not so sure if we
> want to have this as a general pattern. We need to discuss this more.
> 

Agreed, here my latest suggestion was definitely a workaround.

> 
> [1]: Technically it is a combination of the following invariants:
> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>   `struct mii_bus`.
> 
> > If phy_read() is called here, I assume that you are happy about the
> > above comment. The way to call mdiobus_read() here is safe because it
> > just an open code of phy_read(). Simply adding it works for you?
> > 
> > // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > // So it's just an FFI call, open code of `phy_read()`.
> 
> This would be fine if we decide to go with the exception I detailed
> above. Although instead of "open code" I would write "see implementation
> of `phy_read()`".
> 

So the rationale here is the callsite of mdiobus_read() is just a
open-code version of phy_read(), so if we meet the same requirement of
phy_read(), we should be safe here. Maybe:

	"... open code of `phy_read()` with a valid phy_device pointer
	`phydev`"

?

Regards,
Boqun

> -- 
> Cheers,
> Benno
>
Benno Lossin Dec. 12, 2023, 10:40 p.m. UTC | #17
On 12/12/23 21:23, Boqun Feng wrote:
> On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
>> [1]: Technically it is a combination of the following invariants:
>> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
>> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>>   `struct mii_bus`.
>>
>>> If phy_read() is called here, I assume that you are happy about the
>>> above comment. The way to call mdiobus_read() here is safe because it
>>> just an open code of phy_read(). Simply adding it works for you?
>>>
>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> // So it's just an FFI call, open code of `phy_read()`.
>>
>> This would be fine if we decide to go with the exception I detailed
>> above. Although instead of "open code" I would write "see implementation
>> of `phy_read()`".
>>
> 
> So the rationale here is the callsite of mdiobus_read() is just a
> open-code version of phy_read(), so if we meet the same requirement of
> phy_read(), we should be safe here. Maybe:
> 
> 	"... open code of `phy_read()` with a valid phy_device pointer
> 	`phydev`"
> 
> ?

Hmm that might be OK if we add "TODO: replace this with `phy_read` once
bindgen can handle static inline functions.".

Actually, why can't we just use the normal `rust_helper_*` approach? So
just create a `rust_helper_phy_read` that calls `phy_read`. Then call
that from the rust side. Doing this means that we can just keep the
invariants of `struct phy_device` opaque to the Rust side.
That would probably be preferable to adding the `TODO`, since when
bindgen has this feature available, we will automatically handle this
and not forget it. Also we have no issue with diverging code.
FUJITA Tomonori Dec. 12, 2023, 11:01 p.m. UTC | #18
On Tue, 12 Dec 2023 17:35:34 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 12/12/23 14:02, FUJITA Tomonori wrote:
>> On Mon, 11 Dec 2023 22:11:15 -0800
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>>> // side, which should be safe as long as `phydev` is valid.
>>>>>
>>>>> ?
>>>>
>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>>> find a comment like X is valid for the first argument in this C
>>>> function. What I found are comments like X points to valid memory.
>>>
>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>>> `phydev` is pointing to a properly maintained struct phy_device, then an
>>> open code of phy_read() should be safe. Maybe "..., which should be safe
>>> as long as `phydev` points to a valid struct phy_device" ?
>> 
>> As Alice suggested, I updated the comment. The current comment is:
>> 
>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> // So it's just an FFI call.
>> let ret = unsafe {
>>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>> };
> 
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`. After looking at the C code, it seems like
> that the pointer needs to point to a valid `struct mii_bus`.
> This *could* just be an invariant of `struct phy_device` [1], but where
> do we document that?

If phy_device points to a valid object, phy_device.mdio is valid.

A mii_bus must exist before a phy device. A bus is scanned and then a
phy device is found (so phy_device object is crated).

https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634
Benno Lossin Dec. 12, 2023, 11:15 p.m. UTC | #19
On 12/13/23 00:01, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 17:35:34 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 12/12/23 14:02, FUJITA Tomonori wrote:
>>> On Mon, 11 Dec 2023 22:11:15 -0800
>>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>>
>>>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>>>> // side, which should be safe as long as `phydev` is valid.
>>>>>>
>>>>>> ?
>>>>>
>>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>>>> find a comment like X is valid for the first argument in this C
>>>>> function. What I found are comments like X points to valid memory.
>>>>
>>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>>>> `phydev` is pointing to a properly maintained struct phy_device, then an
>>>> open code of phy_read() should be safe. Maybe "..., which should be safe
>>>> as long as `phydev` points to a valid struct phy_device" ?
>>>
>>> As Alice suggested, I updated the comment. The current comment is:
>>>
>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> // So it's just an FFI call.
>>> let ret = unsafe {
>>>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>> };
>>
>> I still think you need to justify why `mdio.bus` is a pointer that you
>> can give to `midobus_read`. After looking at the C code, it seems like
>> that the pointer needs to point to a valid `struct mii_bus`.
>> This *could* just be an invariant of `struct phy_device` [1], but where
>> do we document that?
> 
> If phy_device points to a valid object, phy_device.mdio is valid.

Where is this documented?

> A mii_bus must exist before a phy device. A bus is scanned and then a
> phy device is found (so phy_device object is crated).
> 
> https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634

I know that this is the status. But for this to be useful as a
justification it must be written down somewhere where you can expect to
find it. Not some knowledge that "everyone just knows".
I would prefer to use the solution that I detailed in the other thread.
FUJITA Tomonori Dec. 12, 2023, 11:27 p.m. UTC | #20
On Tue, 12 Dec 2023 22:40:01 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 12/12/23 21:23, Boqun Feng wrote:
>> On Tue, Dec 12, 2023 at 05:35:34PM +0000, Benno Lossin wrote:
>>> [1]: Technically it is a combination of the following invariants:
>>> - the `mdio` field of `struct phy_device` is a valid `struct mido_device`
>>> - the `bus` field of `struct mdio_device` is a valid pointer to a valid
>>>   `struct mii_bus`.
>>>
>>>> If phy_read() is called here, I assume that you are happy about the
>>>> above comment. The way to call mdiobus_read() here is safe because it
>>>> just an open code of phy_read(). Simply adding it works for you?
>>>>
>>>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>>> // So it's just an FFI call, open code of `phy_read()`.
>>>
>>> This would be fine if we decide to go with the exception I detailed
>>> above. Although instead of "open code" I would write "see implementation
>>> of `phy_read()`".
>>>
>> 
>> So the rationale here is the callsite of mdiobus_read() is just a
>> open-code version of phy_read(), so if we meet the same requirement of
>> phy_read(), we should be safe here. Maybe:
>> 
>> 	"... open code of `phy_read()` with a valid phy_device pointer
>> 	`phydev`"
>> 
>> ?
> 
> Hmm that might be OK if we add "TODO: replace this with `phy_read` once
> bindgen can handle static inline functions.".

That's the conclusion? I suppose that a way to handle static inline
functions is still under discussion.


> Actually, why can't we just use the normal `rust_helper_*` approach? So
> just create a `rust_helper_phy_read` that calls `phy_read`. Then call
> that from the rust side. Doing this means that we can just keep the
> invariants of `struct phy_device` opaque to the Rust side.
> That would probably be preferable to adding the `TODO`, since when
> bindgen has this feature available, we will automatically handle this
> and not forget it. Also we have no issue with diverging code.

I wasn't sure that `rust_helper_*` approach is the way to go.
FUJITA Tomonori Dec. 12, 2023, 11:31 p.m. UTC | #21
On Tue, 12 Dec 2023 12:23:04 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> So the rationale here is the callsite of mdiobus_read() is just a
> open-code version of phy_read(), so if we meet the same requirement of
> phy_read(), we should be safe here. Maybe:
> 
> 	"... open code of `phy_read()` with a valid phy_device pointer
> 	`phydev`"
> 
> ?

I'll add the above comment with the similar for phy_write(), thanks!
Benno Lossin Dec. 13, 2023, 12:01 a.m. UTC | #22
On 13.12.23 00:31, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 12:23:04 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> So the rationale here is the callsite of mdiobus_read() is just a
>> open-code version of phy_read(), so if we meet the same requirement of
>> phy_read(), we should be safe here. Maybe:
>>
>> 	"... open code of `phy_read()` with a valid phy_device pointer
>> 	`phydev`"
>>
>> ?
> 
> I'll add the above comment with the similar for phy_write(), thanks!

Why don't you use the `rust_helper` approach?
Benno Lossin Dec. 13, 2023, 12:02 a.m. UTC | #23
On 13.12.23 00:27, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 22:40:01 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> Actually, why can't we just use the normal `rust_helper_*` approach? So
>> just create a `rust_helper_phy_read` that calls `phy_read`. Then call
>> that from the rust side. Doing this means that we can just keep the
>> invariants of `struct phy_device` opaque to the Rust side.
>> That would probably be preferable to adding the `TODO`, since when
>> bindgen has this feature available, we will automatically handle this
>> and not forget it. Also we have no issue with diverging code.
> 
> I wasn't sure that `rust_helper_*` approach is the way to go.

It is the way to go to avoid code duplication and call those
functions.
Andrew Lunn Dec. 13, 2023, 10:24 a.m. UTC | #24
> > The C side people read the Rust code before changing the C code? Let's
> > see. 
> > 
> 
> Hmm... I usually won't call someone "C side people". I mean, the project
> has C part and Rust part, but the community is one.
> 
> In case of myself, I write both C and Rust, if I'm going to change some
> C side function, I may want to see the usage at Rust side, especially
> whether my changes could break the safety, and safety comments may be
> important.

While i agree with your sentiment, ideally we want bilingual
developers, in reality that is not going to happen for a long time. I
could be wrong, but i expect developers to be either C developers, or
Rust developers. They are existing kernel developers who know C, or
Rust developers who are new to the kernel, and may not know much C. So
we should try to keep that in mind.

I personally don't think i have enough Rust knowledge to of even
reached the dangerous stage. But at least the hard part with Rust
seems to be the comments, not the actual code :-(

	Andrew
Andrew Lunn Dec. 13, 2023, 10:28 a.m. UTC | #25
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`.

Maybe a dumb question. Why are you limiting this to just a few members
of struct phy_device? It has ~50 members, any of which can be used by
the C side when Rust calls into C.

    Andrew
Benno Lossin Dec. 13, 2023, 12:14 p.m. UTC | #26
On 12/13/23 11:28, Andrew Lunn wrote:
>> I still think you need to justify why `mdio.bus` is a pointer that you
>> can give to `midobus_read`.
> 
> Maybe a dumb question. Why are you limiting this to just a few members
> of struct phy_device? It has ~50 members, any of which can be used by
> the C side when Rust calls into C.

I limited it to those few members, because the Rust side only uses
those.
Theoretically one could specify all invariants for all members, but that
seems like a *lot* of work.

In Rust everything [1] has to be initialized with a valid value.
Contrast this with C where everything is potentially uninitialized. As
an example, let's look at the first few fields of `PhyDevice`:

    struct PhyDevice {
        mdio: MdioDevice,
        drv: Box<PhyDriver>,
        devlink: Box<DeviceLink>,
        phy_id: u32,
        c45_ids: PhyC45DeviceIds,
        // ...
    }

Note that in Rust we now do not need to write down any invariants, since
they are all implicitly encoded. For example the `drv` field is of type
`Box<PhyDriver>`, it *always* is a pointer to an allocation that
contains a valid `PhyDriver`. So while on the C side you would have to
state this, on the Rust side we get it for free.

Rust function also make use of the "everything is in a valid state"
rule, they know that the fields are valid and thus the Rust equivalent
of `phy_read` could be safe and without any comments:

    impl PhyDevice {
        fn phy_read(&self, regnum: u32) -> i32 {
            self.mdio.bus.mdiobus_read(self.mdio.addr, regnum)
        }
    }

All of this only applies to safe code, `unsafe` code is allowed to
violate all of these things temporarily. However, when it gives a value
back to safe code [2], that value needs to be valid.

I think that specifying all of these implicit invariants in C will be
extremely laborious. Especially since the usual way of doing things in C
is not considering these invariants (at least not consciously), but
rather "just do the thing".

The way we do the interoperability is to just fully trust the C side to
produce valid values that we can feed back to the C side. Of course
there are caveats, so e.g. one needs to initialize a `struct mutex`
before it can be used, but that is what we need to capture with the
safety comments.

[1]: There are exceptions for this, but for the purposes of this
     discussion, they can be ignored. If you want to know more, you can
     read this article in the nomicon:
     https://doc.rust-lang.org/nomicon/unchecked-uninit.html

[2]: There are also exceptions, but I will omit them here.
Boqun Feng Dec. 13, 2023, 4:43 p.m. UTC | #27
On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote:
> > > The C side people read the Rust code before changing the C code? Let's
> > > see. 
> > > 
> > 
> > Hmm... I usually won't call someone "C side people". I mean, the project
> > has C part and Rust part, but the community is one.
> > 
> > In case of myself, I write both C and Rust, if I'm going to change some
> > C side function, I may want to see the usage at Rust side, especially
> > whether my changes could break the safety, and safety comments may be
> > important.
> 
> While i agree with your sentiment, ideally we want bilingual
> developers, in reality that is not going to happen for a long time. I
> could be wrong, but i expect developers to be either C developers, or
> Rust developers. They are existing kernel developers who know C, or
> Rust developers who are new to the kernel, and may not know much C. So

Sorry, I cannot agree with you. Why do we try to divide the community in
two parts? In fact, I keep telling people who want to contribute
Rust-for-Linux that one way to contribute is trying to do some C code
changes first to get familiar with the subsystem and kernel development.

The sentence from Tomo really read like: I don't want to put this
information here, since I don't think anyone would use it. Why do we
want to shutdown the door for more people to collaborate, really, why?
The only downside here is that Tomo needs to maintain a few more lines
of comments. Also the comment is not a random comment, it's the safety
comment, please see below..

> we should try to keep that in mind.
> 
> I personally don't think i have enough Rust knowledge to of even
> reached the dangerous stage. But at least the hard part with Rust
> seems to be the comments, not the actual code :-(
> 

Well, a safety comment is a basic part of Rust, which identifies the
safe/unsafe boundary (i.e. where the code could go wrong in memory
safety) and without that, the code will be just using Rust syntax and
grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
boundaries, why do they try to use Rust? Unsafe Rust is harder to write
than C, and safe Rust is pointless without a clear safe/unsafe boundary.
Plus the syntax is not liked by anyone last time I heard ;-)

Having a correct safety comment is really the bottom line. Without that,
it's just bad Rust code, which I don't think netdev doesn't want either?
Am I missing something here?

Regards,
Boqun

> 	Andrew
>
Boqun Feng Dec. 13, 2023, 5:12 p.m. UTC | #28
On Wed, Dec 13, 2023 at 08:43:09AM -0800, Boqun Feng wrote:
> On Wed, Dec 13, 2023 at 11:24:03AM +0100, Andrew Lunn wrote:
> > > > The C side people read the Rust code before changing the C code? Let's
> > > > see. 
> > > > 
> > > 
> > > Hmm... I usually won't call someone "C side people". I mean, the project
> > > has C part and Rust part, but the community is one.
> > > 
> > > In case of myself, I write both C and Rust, if I'm going to change some
> > > C side function, I may want to see the usage at Rust side, especially
> > > whether my changes could break the safety, and safety comments may be
> > > important.
> > 
> > While i agree with your sentiment, ideally we want bilingual
> > developers, in reality that is not going to happen for a long time. I
> > could be wrong, but i expect developers to be either C developers, or
> > Rust developers. They are existing kernel developers who know C, or
> > Rust developers who are new to the kernel, and may not know much C. So
> 
> Sorry, I cannot agree with you. Why do we try to divide the community in
> two parts? In fact, I keep telling people who want to contribute
> Rust-for-Linux that one way to contribute is trying to do some C code
> changes first to get familiar with the subsystem and kernel development.
> 
> The sentence from Tomo really read like: I don't want to put this
> information here, since I don't think anyone would use it. Why do we
> want to shutdown the door for more people to collaborate, really, why?
> The only downside here is that Tomo needs to maintain a few more lines
> of comments. Also the comment is not a random comment, it's the safety
> comment, please see below..
> 
> > we should try to keep that in mind.
> > 
> > I personally don't think i have enough Rust knowledge to of even
> > reached the dangerous stage. But at least the hard part with Rust
> > seems to be the comments, not the actual code :-(
> > 
> 
> Well, a safety comment is a basic part of Rust, which identifies the
> safe/unsafe boundary (i.e. where the code could go wrong in memory
> safety) and without that, the code will be just using Rust syntax and
> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Having a correct safety comment is really the bottom line. Without that,
> it's just bad Rust code, which I don't think netdev doesn't want either?

s/doesn't//

> Am I missing something here?
> 
> Regards,
> Boqun
> 
> > 	Andrew
> >
Andrew Lunn Dec. 13, 2023, 9:48 p.m. UTC | #29
> Well, a safety comment is a basic part of Rust, which identifies the
> safe/unsafe boundary (i.e. where the code could go wrong in memory
> safety) and without that, the code will be just using Rust syntax and
> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> Plus the syntax is not liked by anyone last time I heard ;-)

Maybe comments are the wrong format for this? Maybe it should be a
formal language? It could then be compiled into an executable form and
tested? It won't show it is complete, but it would at least show it is
correct/incorrect description of the assumptions. For normal builds it
would not be included in the final binary, but maybe debug or formal
verification builds it would be included?

> Having a correct safety comment is really the bottom line. Without that,
> it's just bad Rust code, which I don't think netdev doesn't want either?
> Am I missing something here?

It seems much easier to agree actual code is correct, maybe because it
is a formal language, with a compiler, and a method to test it. Is
that code really bad without the comments? It would be interesting to
look back and see how much the actual code has changed because of
these comments? I _think_ most of the review comments have resulted in
changes to the comments, not the executable code itself. Does that
mean it is much harder to write correct comments than correct code?

       Andrew
Benno Lossin Dec. 13, 2023, 11:40 p.m. UTC | #30
On 12/13/23 22:48, Andrew Lunn wrote:
>> Well, a safety comment is a basic part of Rust, which identifies the
>> safe/unsafe boundary (i.e. where the code could go wrong in memory
>> safety) and without that, the code will be just using Rust syntax and
>> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
>> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
>> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
>> Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Maybe comments are the wrong format for this? Maybe it should be a
> formal language? It could then be compiled into an executable form and
> tested? It won't show it is complete, but it would at least show it is
> correct/incorrect description of the assumptions. For normal builds it
> would not be included in the final binary, but maybe debug or formal
> verification builds it would be included?

That is an interesting suggestion, do you have any specific tools in
mind?
There are some Rust tools for formal verification, see
https://rust-formal-methods.github.io/tools.html
but I don't know if they can be used in the kernel, especially since we
would need a tool that also supports C (I have no experience/knowledge
of verification tools for C, so maybe you have something).
Also my experience tells me that there are several issues with formal
verification in practice:

1. When you want to use some formal system to prove something it is
   often an "all or nothing" game. So you will have to first verify
   everything that lies beneath you, or assume that it is correctly
   implemented. But assuming that everything is correctly implemented is
   rather dangerous, because if you base your formal system on classical
   logic [1], then a single contradiction allows you to prove
   everything. So in order for you to be _sure_ that it is correct, you
   need to work from the ground up.

2. There is no formal Rust memory model. So proving anything for
   interoperability between Rust and C is going to be challenging.

3. The burden of fully verifying a program is great. I know this, as I
   have some experience in this field. Now the programmer not only needs
   to know how to write a piece of code, but also how to write the
   required statements in the formal system and most importantly how to
   prove said statements from the axioms and theorems.


When using safety comments, we avoid the problems of having to prove the
statements formally (which is _very_ difficult). Of course people still
need to know how to write safety comments, which is why I am working on
a standard for safety comments. I hope to post an RFC in a couple weeks.
It will also make the safety comments more formal by having a fixed
set of phrases with exact interpretations, so there can be less room for
misunderstandings.


[2]: You might try to work around this by using a paraconsistent logic,
     but I have little to no experience with that field, so I cannot
     really say more than "it exists".


>> Having a correct safety comment is really the bottom line. Without that,
>> it's just bad Rust code, which I don't think netdev doesn't want either?
>> Am I missing something here?
> 
> It seems much easier to agree actual code is correct, maybe because it
> is a formal language, with a compiler, and a method to test it. Is

I disagree. You always have to consider the entire kernel when you want
to determine if some piece of code is correct. This is by the nature of
the formal language, anything can affect anything.
For example you consider this:

    foo(void **ptr) {
        *ptr = NULL;
        synchronize_rcu()
        print(**x)
    }

How do you know that this is correct? Well you have to look at all
locations where `foo` is invoked and see if after a `synchronize_rcu`
the supplied pointer is valid.

If we do the safety comment stuff correctly, then we have a _local_
property, so something where you do not have to consider the entire
kernel. Instead you assume that all other safety comments are correct
and then only verify the boundary. If all boundaries agree, we have a
reasonably correct program.

> that code really bad without the comments? It would be interesting to
> look back and see how much the actual code has changed because of
> these comments? I _think_ most of the review comments have resulted in
> changes to the comments, not the executable code itself. Does that
> mean it is much harder to write correct comments than correct code?

The code not having changed does not mean that it is correct. There are
no obvious issues present, but can we really know that it is correct?
Only time will tell (or a formal verification).

The issue that we currently have with this patch series is that the
people who know how the stuff works and the people who know how to
write good safety comments are not the same. I hope that my safety
standard will help close this gap.

For example we do not know how the synchronization mechanism for
`phy_suspend` and `phy_resume` work, but you mentioned in some previous
thread that the knowledge is actually somewhere out there. It would help
us a lot if you could give us a link or an explanation. Then we can work
on a suitable safety comment.
Benno Lossin Dec. 13, 2023, 11:51 p.m. UTC | #31
On 12/14/23 00:40, Benno Lossin wrote:
> On 12/13/23 22:48, Andrew Lunn wrote:
>>> Well, a safety comment is a basic part of Rust, which identifies the
>>> safe/unsafe boundary (i.e. where the code could go wrong in memory
>>> safety) and without that, the code will be just using Rust syntax and
>>> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
>>> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
>>> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
>>> Plus the syntax is not liked by anyone last time I heard ;-)
>>
>> Maybe comments are the wrong format for this? Maybe it should be a
>> formal language? It could then be compiled into an executable form and
>> tested? It won't show it is complete, but it would at least show it is
>> correct/incorrect description of the assumptions. For normal builds it
>> would not be included in the final binary, but maybe debug or formal
>> verification builds it would be included?
> 
> That is an interesting suggestion, do you have any specific tools in
> mind?
> There are some Rust tools for formal verification, see
> https://rust-formal-methods.github.io/tools.html
> but I don't know if they can be used in the kernel, especially since we
> would need a tool that also supports C (I have no experience/knowledge
> of verification tools for C, so maybe you have something).
> Also my experience tells me that there are several issues with formal
> verification in practice:

Don't get me wrong, I would welcome a more formalized approach. I just
have seen what that entails and I believe Rust (with safety comments)
to be a good compromise that still allows programmers with no knowledge
in formal systems to program and reasonable correctness.

> 1. When you want to use some formal system to prove something it is
>    often an "all or nothing" game. So you will have to first verify
>    everything that lies beneath you, or assume that it is correctly
>    implemented. But assuming that everything is correctly implemented is
>    rather dangerous, because if you base your formal system on classical
>    logic [1], then a single contradiction allows you to prove
>    everything. So in order for you to be _sure_ that it is correct, you
>    need to work from the ground up.
> 
> 2. There is no formal Rust memory model. So proving anything for
>    interoperability between Rust and C is going to be challenging.
> 
> 3. The burden of fully verifying a program is great. I know this, as I
>    have some experience in this field. Now the programmer not only needs
>    to know how to write a piece of code, but also how to write the
>    required statements in the formal system and most importantly how to
>    prove said statements from the axioms and theorems.
> 
> 
> When using safety comments, we avoid the problems of having to prove the
> statements formally (which is _very_ difficult). Of course people still
> need to know how to write safety comments, which is why I am working on
> a standard for safety comments. I hope to post an RFC in a couple weeks.
> It will also make the safety comments more formal by having a fixed
> set of phrases with exact interpretations, so there can be less room for
> misunderstandings.
> 
> 
> [2]: You might try to work around this by using a paraconsistent logic,

This should actually be [1]:.

>      but I have little to no experience with that field, so I cannot
>      really say more than "it exists".
Boqun Feng Dec. 13, 2023, 11:59 p.m. UTC | #32
On Wed, Dec 13, 2023 at 10:48:39PM +0100, Andrew Lunn wrote:
> > Well, a safety comment is a basic part of Rust, which identifies the
> > safe/unsafe boundary (i.e. where the code could go wrong in memory
> > safety) and without that, the code will be just using Rust syntax and
> > grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> > boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> > than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> > Plus the syntax is not liked by anyone last time I heard ;-)
> 
> Maybe comments are the wrong format for this? Maybe it should be a

Maybe, but they are what we have right now. I do believe the unsafe <->
safe boundary in Rust needs better tooling: using comments may be a
little bit abitrary. However, let's stick to what we have right now.
Benno is actually working our Rust safety standard (feel free to correct
me if I get the name wrong), hopefully that helps.

> formal language? It could then be compiled into an executable form and
> tested? It won't show it is complete, but it would at least show it is
> correct/incorrect description of the assumptions. For normal builds it
> would not be included in the final binary, but maybe debug or formal
> verification builds it would be included?
> 

Good idea, we actually want something similar when we were talking about
Benno's safety standard, but as you may know a complete one would take
years, or maybe impossbile. Also, kernel's safety requirement sometimes
is weird and non-trivial, it's going to be a learning process ;-) A tool
is certainly what we would like to look into if we have more experience
in Rust abstraction and more time.

> > Having a correct safety comment is really the bottom line. Without that,
> > it's just bad Rust code, which I don't think netdev doesn't want either?
> > Am I missing something here?
> 
> It seems much easier to agree actual code is correct, maybe because it
> is a formal language, with a compiler, and a method to test it. Is
> that code really bad without the comments? It would be interesting to

Note that most of the comment reviews are on *safety* comments (type
invariants are related to safety comments). To me, in the review
process, safety comments are similar to commit logs. In kernel patch
reviews, we sometimes say "thanks for the explanation but please put
that in the commit log", that is, even we know the code is correct and
the patchset has been explained, we still want to see the explanation
written down somewhere. Commit logs are ways to explain "why do I want
to do this", and safety comments are ways to explain "why can I do the
following safely". They are both ways to communicate during the review
and for the future readers.

So is a patchset really bad without commit logs? ;-)

> look back and see how much the actual code has changed because of
> these comments? I _think_ most of the review comments have resulted in

I was trying to figure out the answer, but the patchset just has too
many versions.. and discussions were splitted between versions...

> changes to the comments, not the executable code itself. Does that
> mean it is much harder to write correct comments than correct code?
> 

If one can explain their code to others and make sure others understand
the correctness of the code, then the code is very likely correct. So
I think yes, writing correct comments is harder than correct code.

I admit the review process took a bit long because it's a learning
process for both developers and reviewers: being the first in-tree Rust
driver may not be that easy, please understand the high standard
expectation.

Regards,
Boqun

>        Andrew
Andrew Lunn Dec. 14, 2023, 9:26 a.m. UTC | #33
On Wed, Dec 13, 2023 at 11:40:26PM +0000, Benno Lossin wrote:
> On 12/13/23 22:48, Andrew Lunn wrote:
> >> Well, a safety comment is a basic part of Rust, which identifies the
> >> safe/unsafe boundary (i.e. where the code could go wrong in memory
> >> safety) and without that, the code will be just using Rust syntax and
> >> grammar. Honestly, if one doesn't try hard to identify the safe/unsafe
> >> boundaries, why do they try to use Rust? Unsafe Rust is harder to write
> >> than C, and safe Rust is pointless without a clear safe/unsafe boundary.
> >> Plus the syntax is not liked by anyone last time I heard ;-)
> > 
> > Maybe comments are the wrong format for this? Maybe it should be a
> > formal language? It could then be compiled into an executable form and
> > tested? It won't show it is complete, but it would at least show it is
> > correct/incorrect description of the assumptions. For normal builds it
> > would not be included in the final binary, but maybe debug or formal
> > verification builds it would be included?
> 
> That is an interesting suggestion, do you have any specific tools in
> mind?

Sorry, no. I've no experience in this field at all. But given the
discussions this patch has caused, simply a list of C or Rust
expressions which evaluate to True when an assumption is correct would
be a good start.

We have said that we assume the phydev->lock is held. That is easy to
express in code. We have said that phydev->mdio must be set, which is
again easy to express. phydev->mdio.addr must be in the range
0..PHY_MAX_ADDR, etc.

You probably cannot express all the safety requirements this way, but
the set you can describe should be easy to understand and also
unambiguous, since it is code. The rest still can be as comments.  It
would be easy to compile this code and insert it before the function
on a verification build. Its only runtime checking, but its more
functional than comments which the compiler just throws away. And
maybe subsystems like this which are pretty much always slow path
might even leave them enabled all the time, to act as a set of
assert()s, which you sometimes see in code bases.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 25cfc5ded1da..8d00cece5e23 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -60,6 +60,14 @@  config FIXED_PHY
 
 	  Currently tested with mpc866ads and mpc8349e-mitx.
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "Rust PHYLIB abstractions 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 phylib core.
+
 config SFP
 	tristate "SFP cage support"
 	depends on I2C && PHYLINK
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 85f013ed4ca4..eaf01df7d97a 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,9 @@ 
 
 #include <kunit/test.h>
 #include <linux/errname.h>
+#include <linux/ethtool.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e6aff80b521f..7ac39874aeac 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -14,6 +14,7 @@ 
 #![no_std]
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
+#![feature(const_maybe_uninit_zeroed)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(offset_of)]
@@ -38,6 +39,8 @@ 
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..fe415cb369d3
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..5d220187eec9
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,754 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@gmail.com>
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
+
+use core::marker::PhantomData;
+
+/// PHY state machine states.
+///
+/// Corresponds to the kernel's [`enum phy_state`].
+///
+/// Some of PHY drivers access to the state of PHY's software state machine.
+///
+/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
+#[derive(PartialEq, Eq)]
+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+}
+
+/// A mode of Ethernet communication.
+///
+/// PHY drivers get duplex information from hardware and update the current state.
+pub enum DuplexMode {
+    /// PHY is in full-duplex mode.
+    Full,
+    /// PHY is in half-duplex mode.
+    Half,
+    /// PHY is in unknown duplex mode.
+    Unknown,
+}
+
+/// An instance of a PHY device.
+///
+/// Wraps the kernel's [`struct phy_device`].
+///
+/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
+/// executes [`Driver`]'s methods during the callback.
+///
+/// # Invariants
+///
+/// Referencing a `phy_device` using this struct asserts that you are in
+/// a context where all methods defined on this struct are safe to call.
+///
+/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
+// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
+// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
+// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
+// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
+// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
+// to the instance.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// For the duration of 'a, the pointer must point at a valid `phy_device`,
+    /// and the caller must be in a context where all methods defined on this struct
+    /// are safe to call.
+    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
+        let ptr = ptr.cast::<Self>();
+        // SAFETY: by the function requirements the pointer is valid and we have unique access for
+        // the duration of `'a`.
+        unsafe { &mut *ptr }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn phy_id(&self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Gets the state of PHY state machine states.
+    pub fn state(&self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let state = unsafe { (*phydev).state };
+        // TODO: this conversion code will be replaced with automatically generated code by bindgen
+        // when it becomes possible.
+        // better to call WARN_ONCE() when the state is out-of-range.
+        match state {
+            bindings::phy_state_PHY_DOWN => DeviceState::Down,
+            bindings::phy_state_PHY_READY => DeviceState::Ready,
+            bindings::phy_state_PHY_HALTED => DeviceState::Halted,
+            bindings::phy_state_PHY_ERROR => DeviceState::Error,
+            bindings::phy_state_PHY_UP => DeviceState::Up,
+            bindings::phy_state_PHY_RUNNING => DeviceState::Running,
+            bindings::phy_state_PHY_NOLINK => DeviceState::NoLink,
+            bindings::phy_state_PHY_CABLETEST => DeviceState::CableTest,
+            _ => DeviceState::Error,
+        }
+    }
+
+    /// Gets the current link state.
+    ///
+    /// It returns true if the link is up.
+    pub fn is_link_up(&self) -> bool {
+        const LINK_IS_UP: u64 = 1;
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(14, 1) == LINK_IS_UP
+    }
+
+    /// Gets the current auto-negotiation configuration.
+    ///
+    /// It returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&self) -> bool {
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
+    }
+
+    /// Gets the current auto-negotiation state.
+    ///
+    /// It returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&self) -> bool {
+        const AUTONEG_COMPLETED: u64 = 1;
+        // TODO: the code to access to the bit field will be replaced with automatically
+        // generated code by bindgen when it becomes possible.
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+        bit_field.get(15, 1) == AUTONEG_COMPLETED
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&mut self, speed: u32) {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).speed = speed as i32 };
+    }
+
+    /// Sets duplex mode.
+    pub fn set_duplex(&mut self, mode: DuplexMode) {
+        let phydev = self.0.get();
+        let v = match mode {
+            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
+            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
+            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
+        };
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        unsafe { (*phydev).duplex = v };
+    }
+
+    /// Reads a given C22 PHY register.
+    // This function reads a hardware register and updates the stats so takes `&mut self`.
+    pub fn read(&mut self, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Writes a given C22 PHY register.
+    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
+        })
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Resolves the advertisements into PHY settings.
+    pub fn resolve_aneg_linkmode(&mut self) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
+    }
+
+    /// Executes software reset the PHY via `BMCR_RESET` bit.
+    pub fn genphy_soft_reset(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // so an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_init_hw(phydev) })
+    }
+
+    /// Starts auto-negotiation.
+    pub fn start_aneg(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::_phy_start_aneg(phydev) })
+    }
+
+    /// Resumes the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_resume(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_resume(phydev) })
+    }
+
+    /// Suspends the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_suspend(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_suspend(phydev) })
+    }
+
+    /// Checks the link status and updates current link state.
+    pub fn genphy_read_status(&mut self) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Updates the link status.
+    pub fn genphy_update_link(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_update_link(phydev) })
+    }
+
+    /// Reads link partner ability.
+    pub fn genphy_read_lpa(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
+    }
+
+    /// Reads PHY abilities.
+    pub fn genphy_read_abilities(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
+    }
+}
+
+/// Defines certain other features this PHY supports (like interrupts).
+///
+/// These flag values are used in [`Driver::FLAGS`].
+pub mod flags {
+    /// PHY is internal.
+    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
+    /// PHY needs to be reset after the refclk is enabled.
+    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
+    /// Polling is used to detect PHY status changes.
+    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
+    /// Don't suspend.
+    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
+}
+
+/// An adapter for the registration of a PHY driver.
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C core code ensures that the accessors on
+            // `Device` are okay to call even though `phy_device->lock`
+            // might not be held.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: The C core code ensures that the accessors on
+            // `Device` are okay to call even though `phy_device->lock`
+            // might not be held.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: This callback is called only in contexts
+        // where we hold `phy_device->lock`, so the accessors on
+        // `Device` are okay to call.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            // CAST: the C side verifies devnum < 32.
+            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
+            Ok(ret.into())
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: This callback is called only in contexts
+            // where we hold `phy_device->lock`, so the accessors on
+            // `Device` are okay to call.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum as u8, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: This callback is called only in contexts
+        // where we hold `phy_device->lock`, so the accessors on
+        // `Device` are okay to call.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+}
+
+/// Driver structure for a particular PHY type.
+///
+/// Wraps the kernel's [`struct phy_driver`].
+/// This is used to register a driver for a particular PHY type with the kernel.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+///
+/// [`struct phy_driver`]: ../../../../../../../include/linux/phy.h
+#[repr(transparent)]
+pub struct DriverVTable(Opaque<bindings::phy_driver>);
+
+// SAFETY: `DriverVTable` has no &self methods, so immutable references to it
+// are useless.
+unsafe impl Sync for DriverVTable {}
+
+/// Creates a [`DriverVTable`] instance from [`Driver`].
+///
+/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
+///
+/// [`module_phy_driver`]: crate::module_phy_driver
+pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
+    // INVARIANT: All the fields of `struct phy_driver` are initialized properly.
+    DriverVTable(Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr().cast_mut(),
+        flags: T::FLAGS,
+        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
+        soft_reset: if T::HAS_SOFT_RESET {
+            Some(Adapter::<T>::soft_reset_callback)
+        } else {
+            None
+        },
+        get_features: if T::HAS_GET_FEATURES {
+            Some(Adapter::<T>::get_features_callback)
+        } else {
+            None
+        },
+        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
+            Some(Adapter::<T>::match_phy_device_callback)
+        } else {
+            None
+        },
+        suspend: if T::HAS_SUSPEND {
+            Some(Adapter::<T>::suspend_callback)
+        } else {
+            None
+        },
+        resume: if T::HAS_RESUME {
+            Some(Adapter::<T>::resume_callback)
+        } else {
+            None
+        },
+        config_aneg: if T::HAS_CONFIG_ANEG {
+            Some(Adapter::<T>::config_aneg_callback)
+        } else {
+            None
+        },
+        read_status: if T::HAS_READ_STATUS {
+            Some(Adapter::<T>::read_status_callback)
+        } else {
+            None
+        },
+        read_mmd: if T::HAS_READ_MMD {
+            Some(Adapter::<T>::read_mmd_callback)
+        } else {
+            None
+        },
+        write_mmd: if T::HAS_WRITE_MMD {
+            Some(Adapter::<T>::write_mmd_callback)
+        } else {
+            None
+        },
+        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
+            Some(Adapter::<T>::link_change_notify_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+    }))
+}
+
+/// Driver implementation for a particular PHY type.
+///
+/// This trait is used to create a [`DriverVTable`].
+#[vtable]
+pub trait Driver {
+    /// Defines certain other features this PHY supports.
+    /// It is a combination of the flags in the [`flags`] module.
+    const FLAGS: u32 = 0;
+
+    /// The friendly name of this PHY type.
+    const NAME: &'static CStr;
+
+    /// This driver only works for PHYs with IDs which match this field.
+    /// The default id and mask are zero.
+    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
+
+    /// Issues a PHY software reset.
+    fn soft_reset(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Probes the hardware to determine what abilities it has.
+    fn get_features(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Returns true if this is a suitable driver for the given phydev.
+    /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
+    fn match_phy_device(_dev: &Device) -> bool {
+        false
+    }
+
+    /// Configures the advertisement and resets auto-negotiation
+    /// if auto-negotiation is enabled.
+    fn config_aneg(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Determines the negotiated speed and duplex.
+    fn read_status(_dev: &mut Device) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Suspends the hardware, saving state if needed.
+    fn suspend(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Resumes the hardware, restoring state if needed.
+    fn resume(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD read function for reading a MMD register.
+    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD write function for writing a MMD register.
+    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Callback for notification of link change.
+    fn link_change_notify(_dev: &mut Device) {}
+}
+
+/// Registration structure for PHY drivers.
+///
+/// Registers [`DriverVTable`] instances with the kernel. They will be unregistered when dropped.
+///
+/// # Invariants
+///
+/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: Pin<&'static mut [DriverVTable]>,
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    pub fn register(
+        module: &'static crate::ThisModule,
+        drivers: Pin<&'static mut [DriverVTable]>,
+    ) -> Result<Self> {
+        if drivers.is_empty() {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of
+        // the `drivers` slice are initialized properly. `drivers` will not be moved.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
+        })?;
+        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
+        Ok(Registration { drivers })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `self.drivers` is valid.
+        // So an FFI call with a valid pointer.
+        unsafe {
+            bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32)
+        };
+    }
+}
+
+/// An identifier for PHY devices on an MDIO/MII bus.
+///
+/// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate
+/// PHY driver.
+pub struct DeviceId {
+    id: u32,
+    mask: DeviceMask,
+}
+
+impl DeviceId {
+    /// Creates a new instance with the exact match mask.
+    pub const fn new_with_exact_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Exact,
+        }
+    }
+
+    /// Creates a new instance with the model match mask.
+    pub const fn new_with_model_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Model,
+        }
+    }
+
+    /// Creates a new instance with the vendor match mask.
+    pub const fn new_with_vendor_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Vendor,
+        }
+    }
+
+    /// Creates a new instance with a custom match mask.
+    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Custom(mask),
+        }
+    }
+
+    /// Creates a new instance from [`Driver`].
+    pub const fn new_with_driver<T: Driver>() -> Self {
+        T::PHY_DEVICE_ID
+    }
+
+    /// Get a `mask` as u32.
+    pub const fn mask_as_int(&self) -> u32 {
+        self.mask.as_int()
+    }
+
+    // macro use only
+    #[doc(hidden)]
+    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
+        bindings::mdio_device_id {
+            phy_id: self.id,
+            phy_id_mask: self.mask.as_int(),
+        }
+    }
+}
+
+enum DeviceMask {
+    Exact,
+    Model,
+    Vendor,
+    Custom(u32),
+}
+
+impl DeviceMask {
+    const MASK_EXACT: u32 = !0;
+    const MASK_MODEL: u32 = !0 << 4;
+    const MASK_VENDOR: u32 = !0 << 10;
+
+    const fn as_int(&self) -> u32 {
+        match self {
+            DeviceMask::Exact => Self::MASK_EXACT,
+            DeviceMask::Model => Self::MASK_MODEL,
+            DeviceMask::Vendor => Self::MASK_VENDOR,
+            DeviceMask::Custom(mask) => *mask,
+        }
+    }
+}