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

Commit Message

FUJITA Tomonori Oct. 12, 2023, 12:53 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 <[e-mail]>
---
 init/Kconfig                    |   8 +
 rust/Makefile                   |   1 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 679 ++++++++++++++++++++++++++++++++
 6 files changed, 700 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

Comments

Benno Lossin Oct. 13, 2023, 9:31 p.m. UTC | #1

On 12.10.23 14:53, 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 <[e-mail]>
> ---
>   init/Kconfig                    |   8 +
>   rust/Makefile                   |   1 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 679 ++++++++++++++++++++++++++++++++
>   6 files changed, 700 insertions(+)
>   create mode 100644 rust/kernel/net.rs
>   create mode 100644 rust/kernel/net/phy.rs
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 6d35728b94b2..0fc6f5568748 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1903,6 +1903,14 @@ config RUST
> 
>   	  If unsure, say N.
> 
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "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.
> +

I find it a bit weird that this is its own option under "General". I think
it would be reasonable to put it under "Rust", since that would also scale
better when other subsystems do this.

>   config RUSTC_VERSION_TEXT
>   	string
>   	depends on RUST
> diff --git a/rust/Makefile b/rust/Makefile
> index 87958e864be0..f67e55945b36 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@
>         cmd_bindgen = \
>   	$(BINDGEN) $< $(bindgen_target_flags) \
>   		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
> +		--rustified-enum phy_state\

Please change this to Miguel's solution.

>   		--no-debug '.*' \
>   		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
>   		$(bindgen_target_cflags) $(bindgen_target_extra)

[...]

> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function can be called only in the callbacks in `phy_driver`. PHYLIB guarantees

"can be called in" -> "must only be called from"

> +    /// the exclusive access for the duration of the lifetime `'a`.

In some other thread you mentioned that no lock is held for
`resume`/`suspend`, how does this interact with it?

> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
> +        // `Device` type being transparent makes the cast ok.
> +        unsafe { &mut *ptr.cast() }

please refactor to

     // CAST: ...
     let ptr = ptr.cast::<Self>();
     // SAFETY: ...
     unsafe { &mut *ptr }

> +    }

[...]

> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&self, speed: u32) {

This function modifies state, but is `&self`?

> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let mut phydev = unsafe { *self.0.get() };
> +        phydev.speed = speed as i32;
> +    }
> +
> +    /// Sets duplex mode.
> +    pub fn set_duplex(&self, mode: DuplexMode) {

This function modifies state, but is `&self`?

> +        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: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let mut phydev = unsafe { *self.0.get() };
> +        phydev.duplex = v;

Note that this piece of code will actually not do the correct thing. It
will create a copy of `phydev` on the stack and modify that instead of the
pointee of `self`. I think the code was fine before this change.

> +    }
> +
> +    /// Reads a given C22 PHY register.
> +    pub fn read(&self, regnum: u16) -> Result<u16> {

No idea if this function should be `&mut self` or `&self`. Would
it be ok for mutltiple threads to call this function concurrently?
If yes, then leave it as `&self`, if no then change it to `&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.
> +        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(&self, regnum: u16, val: u16) -> Result {

This should probably be `&mut self`, but not sure.

> +        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(&self, page: u16, regnum: u16) -> Result<u16> {

Again same question (also for all other functions below that call the C
side).

> +        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)
> +        }
> +    }

[...]

> +}
> +
> +/// Defines certain other features this PHY supports (like interrupts).

Maybe add a link where these flags can be used.

> +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;
> +}

[...]

> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[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;

What would happen if I set this to some value that is not a combination of
the flag values above? I expect that bits that are not part of the flag
values above to be ignored.

> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;
> +
> +    /// This driver only works for PHYs with IDs which match this field.

Mention that the default value is 0.

> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);

[...]

> +}
> +
> +/// Registration structure for a PHY driver.
> +///
> +/// # Invariants
> +///
> +/// All elements of the `drivers` slice are valid and currently registered
> +/// to the kernel via `phy_drivers_register`.

Since `DriverType` is now safe a wrapper type, this invariant should be
moved to that type instead.

> +pub struct Registration {
> +    drivers: &'static [DriverType],
> +}
> +
> +impl Registration {
> +    /// Registers a PHY driver.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The values of the `drivers` array must be initialized properly.

With the above change you do not need this (since all instances of
`DriverType` are always initialized). But I am not sure if it would be
fine to call `phy_driver_register` multiple times with the same driver
without unregistering it first.

I thought about this implementation of `Registration` a bit and I think
there are two possible ways to avoid both this `unsafe` function and the
mutable static in the `module_phy_driver` macro from patch 2:

Option 1:
- make the constructor of `DriverType` register the driver and the `Drop`
   impl unregister the driver.
- remove the `Registration` type.
- in the `module_phy_driver` macro create the array of `DriverType`s inside
   of the `Module` struct.

The disadvantage of this solution is that `phy_drivers_register` is called
for every driver (and also `phy_drivers_unregister`). But if you and Andrew
think that that is fine, then go with this option. The other advantage of
this solution is that it also becomes safely usable without the
`module_phy_driver` macro. For exmaple when a more complex `Module`
struct is needed by a driver.

Option 2:
- remove the `Registration` type.
- in the `module_phy_driver` macro: create the array of `DriverType`s inside
   of the `Module` struct.
- in the `module_phy_driver` macro: register the drivers in the
   `Module::init` function and unregister them in the `Drop` impl of
   `Module`.

This approach only has one call to `phy_drivers_(un)register`, but cannot
really be used safely without the `module_phy_driver` macro.

> +    pub unsafe fn register(
> +        module: &'static crate::ThisModule,
> +        drivers: &'static [DriverType],
> +    ) -> Result<Self> {
> +        if drivers.is_empty() {
> +            return Err(code::EINVAL);
> +        }
> +        // SAFETY: The safety requirements of the function ensure that `drivers` array are initialized properly.
> +        // 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 safety requirements of the function and the success of `phy_drivers_register` ensure
> +        // the invariants.
> +        Ok(Registration { drivers })
> +    }
> +}

[...]

> +
> +    /// 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 as_mdio_device_id(&self) -> bindings::mdio_device_id {

I would name this just `mdio_device_id`.

Andrew Lunn Oct. 14, 2023, 2:12 a.m. UTC | #2

> > +config RUST_PHYLIB_ABSTRACTIONS
> > +        bool "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.
> > +
> 
> I find it a bit weird that this is its own option under "General". I think
> it would be reasonable to put it under "Rust", since that would also scale
> better when other subsystems do this.

To some extent, this is just a temporary location. Once the
restrictions of the build systems are solved, i expect this will move
into drivers/net/phy/Kconfig, inside the 'if PHYLIB'. However, i
agree, this should be under the Rust menu.

> > +    }
> > +
> > +    /// Reads a given C22 PHY register.
> > +    pub fn read(&self, regnum: u16) -> Result<u16> {
> 
> No idea if this function should be `&mut self` or `&self`. Would
> it be ok for mutltiple threads to call this function concurrently?
> If yes, then leave it as `&self`, if no then change it to `&mut self`.

The MDIO layer before has a lock, so its will serialize parallel
reads/writes. With the current Rust integration, it should never be
possible for multiple threads to be active at once, but if it does
happen, its not a problem anyway.

 	Andrew

FUJITA Tomonori Oct. 14, 2023, 4:50 a.m. UTC | #3

On Sat, 14 Oct 2023 04:12:57 +0200
Andrew Lunn <[e-mail]> wrote:

>> > +config RUST_PHYLIB_ABSTRACTIONS
>> > +        bool "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.
>> > +
>> 
>> I find it a bit weird that this is its own option under "General". I think
>> it would be reasonable to put it under "Rust", since that would also scale
>> better when other subsystems do this.
> 
> To some extent, this is just a temporary location. Once the
> restrictions of the build systems are solved, i expect this will move
> into drivers/net/phy/Kconfig, inside the 'if PHYLIB'. However, i
> agree, this should be under the Rust menu.

Sure, I'll do in the next patchset.

FUJITA Tomonori Oct. 14, 2023, 7:22 a.m. UTC | #4

On Fri, 13 Oct 2023 21:31:16 +0000
Benno Lossin <[e-mail]> wrote:

>> +/// An instance of a PHY device.
>> +///
>> +/// Wraps the kernel's `struct phy_device`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This function can be called only in the callbacks in `phy_driver`. PHYLIB guarantees
> 
> "can be called in" -> "must only be called from"

Fixed.

>> +    /// the exclusive access for the duration of the lifetime `'a`.
> 
> In some other thread you mentioned that no lock is held for
> `resume`/`suspend`, how does this interact with it?

The same quesiton, 4th time? 

PHYLIB is implemented in a way that PHY drivers exlusively access to
phy_device during the callbacks.


>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
>> +        // `Device` type being transparent makes the cast ok.
>> +        unsafe { &mut *ptr.cast() }
> 
> please refactor to
> 
>      // CAST: ...
>      let ptr = ptr.cast::<Self>();
>      // SAFETY: ...
>      unsafe { &mut *ptr }

I can but please tell the exactly comments for after CAST and SAFETY.

I can't find the description of CAST comment in
Documentation/rust/coding-guidelines.rst. So please add why and how to
avoid repeating the same review comment in the future.


>> +    /// Returns true if auto-negotiation is completed.
>> +    pub fn is_autoneg_completed(&self) -> bool {
>> +        const AUTONEG_COMPLETED: u32 = 1;
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let phydev = unsafe { *self.0.get() };
>> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
>> +    }
>> +
>> +    /// Sets the speed of the PHY.
>> +    pub fn set_speed(&self, speed: u32) {
> 
> This function modifies state, but is `&self`?

Boqun asked me to drop mut on v3 review and then you ask why on v4?
Trying to find a way to discourage developpers to write Rust
abstractions? :)

I would recommend the Rust reviewers to make sure that such would
not happen. I really appreciate comments but inconsistent reviewing is
painful.


>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let mut phydev = unsafe { *self.0.get() };
>> +        phydev.speed = speed as i32;
>> +    }
>> +
>> +    /// Sets duplex mode.
>> +    pub fn set_duplex(&self, mode: DuplexMode) {
> 
> This function modifies state, but is `&self`?

Ditto.


>> +        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: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        let mut phydev = unsafe { *self.0.get() };
>> +        phydev.duplex = v;
> 
> Note that this piece of code will actually not do the correct thing. It
> will create a copy of `phydev` on the stack and modify that instead of the
> pointee of `self`. I think the code was fine before this change.

Oops, reverted.


>> +    /// Writes a given C22 PHY register.
>> +    pub fn write(&self, regnum: u16, val: u16) -> Result {
> 
> This should probably be `&mut self`, but not sure.

Please discuss with Boqun what should be.


>> +        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(&self, page: u16, regnum: u16) -> Result<u16> {
> 
> Again same question (also for all other functions below that call the C
> side).

Ditto (also for all other functions below that call the C side).


>> +        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)
>> +        }
>> +    }
> 
> [...]
> 
>> +}
>> +
>> +/// Defines certain other features this PHY supports (like interrupts).
> 
> Maybe add a link where these flags can be used.

I already put the link to here in trait Driver.


>> +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;
>> +}
> 
> [...]
> 
>> +
>> +/// Corresponds to functions in `struct phy_driver`.
>> +///
>> +/// This is used to register a PHY driver.
>> +#[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;
> 
> What would happen if I set this to some value that is not a combination of
> the flag values above? I expect that bits that are not part of the flag
> values above to be ignored.

Your expectation is correct.


>> +    /// The friendly name of this PHY type.
>> +    const NAME: &'static CStr;
>> +
>> +    /// This driver only works for PHYs with IDs which match this field.
> 
> Mention that the default value is 0.

Done.


>> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> 
> [...]
> 
>> +}
>> +
>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// All elements of the `drivers` slice are valid and currently registered
>> +/// to the kernel via `phy_drivers_register`.
> 
> Since `DriverType` is now safe a wrapper type, this invariant should be
> moved to that type instead.

Understood.


>> +pub struct Registration {
>> +    drivers: &'static [DriverType],
>> +}
>> +
>> +impl Registration {
>> +    /// Registers a PHY driver.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The values of the `drivers` array must be initialized properly.
> 
> With the above change you do not need this (since all instances of
> `DriverType` are always initialized). But I am not sure if it would be

Nice.


> fine to call `phy_driver_register` multiple times with the same driver
> without unregistering it first.

The second call `phy_driver_register` with the same drivers (without
unregistered) returns an error. You don't need to worry.


>> +    /// 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 as_mdio_device_id(&self) -> bindings::mdio_device_id {
> 
> I would name this just `mdio_device_id`.

Either is fine by me. Please tell me why for future reference.

Benno Lossin Oct. 14, 2023, 8:07 a.m. UTC | #5

On 14.10.23 09:22, FUJITA Tomonori wrote:
> On Fri, 13 Oct 2023 21:31:16 +0000
> Benno Lossin <[e-mail]> wrote:
>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>
>> In some other thread you mentioned that no lock is held for
>> `resume`/`suspend`, how does this interact with it?
> 
> The same quesiton, 4th time?

Yes, it is not clear to me from the code/safety comment alone why
this is safe. Please improve the comment such that that is the case.

> PHYLIB is implemented in a way that PHY drivers exlusively access to
> phy_device during the callbacks.

As I suggested in a previous thread, it would be extremely helpful
if you add a comment on the `phy` abstractions module that explains
how `PHYLIB` is implemented. Explain that it takes care of locking
and other safety related things.

>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
>>> +        // `Device` type being transparent makes the cast ok.
>>> +        unsafe { &mut *ptr.cast() }
>>
>> please refactor to
>>
>>       // CAST: ...
>>       let ptr = ptr.cast::<Self>();
>>       // SAFETY: ...
>>       unsafe { &mut *ptr }
> 
> I can but please tell the exactly comments for after CAST and SAFETY.
> 
> I can't find the description of CAST comment in
> Documentation/rust/coding-guidelines.rst. So please add why and how to
> avoid repeating the same review comment in the future.

I haven't had the time to finish my work on the standardization of
`SAFETY` (and also `CAST`) comments, but I am working on that.

        // 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 }

>>> +    /// Returns true if auto-negotiation is completed.
>>> +    pub fn is_autoneg_completed(&self) -> bool {
>>> +        const AUTONEG_COMPLETED: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>>> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
>>> +    }
>>> +
>>> +    /// Sets the speed of the PHY.
>>> +    pub fn set_speed(&self, speed: u32) {
>>
>> This function modifies state, but is `&self`?
> 
> Boqun asked me to drop mut on v3 review and then you ask why on v4?
> Trying to find a way to discourage developpers to write Rust
> abstractions? :)
> 
> I would recommend the Rust reviewers to make sure that such would
> not happen. I really appreciate comments but inconsistent reviewing is
> painful.

I agree with Boqun. Before Boqun's suggestion all functions were
`&mut self`. Now all functions are `&self`. Both are incorrect. A
function that takes `&mut self` can modify the state of `Self`,
but it is weird for it to not modify anything at all. Such a
function also can only be called by a single thread (per instance
of `Self`) at a time. Functions with `&self` cannot modify the
state of `Self`, except of course with interior mutability. If
they do modify state with interior mutability, then they should
have a good reason to do that.

What I want you to do here is think about which functions should
be `&mut self` and which should be `&self`, since clearly just
one or the other is wrong here.

>>> +        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)
>>> +        }
>>> +    }
>>
>> [...]
>>
>>> +}
>>> +
>>> +/// Defines certain other features this PHY supports (like interrupts).
>>
>> Maybe add a link where these flags can be used.
> 
> I already put the link to here in trait Driver.

I am asking about a link here, as it is a bit confusing when
you just stumble over this flag module here. It doesn't hurt
to link more.

>>> +pub struct Registration {
>>> +    drivers: &'static [DriverType],
>>> +}
>>> +
>>> +impl Registration {
>>> +    /// Registers a PHY driver.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// The values of the `drivers` array must be initialized properly.
>>
>> With the above change you do not need this (since all instances of
>> `DriverType` are always initialized). But I am not sure if it would be
> 
> Nice.
> 
> 
>> fine to call `phy_driver_register` multiple times with the same driver
>> without unregistering it first.
> 
> The second call `phy_driver_register` with the same drivers (without
> unregistered) returns an error. You don't need to worry.

I see, then it's fine.

>>> +    /// 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 as_mdio_device_id(&self) -> bindings::mdio_device_id {
>>
>> I would name this just `mdio_device_id`.
> 
> Either is fine by me. Please tell me why for future reference.

Functions starting with `as_` or `to_` in Rust generally indicate
some kind of conversion. `to_` functions generally take just `self`
by value and `as_` conversions take just `&self`/`&mut self`. See
`Option::as_ref` or `Option::as_mut`. This function is not really
a conversion, rather it is a getter.

FUJITA Tomonori Oct. 14, 2023, 10:32 a.m. UTC | #6

On Sat, 14 Oct 2023 08:07:03 +0000
Benno Lossin <[e-mail]> wrote:

> On 14.10.23 09:22, FUJITA Tomonori wrote:
>> On Fri, 13 Oct 2023 21:31:16 +0000
>> Benno Lossin <[e-mail]> wrote:
>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>
>>> In some other thread you mentioned that no lock is held for
>>> `resume`/`suspend`, how does this interact with it?
>> 
>> The same quesiton, 4th time?
> 
> Yes, it is not clear to me from the code/safety comment alone why
> this is safe. Please improve the comment such that that is the case.
>
>> PHYLIB is implemented in a way that PHY drivers exlusively access to
>> phy_device during the callbacks.
> 
> As I suggested in a previous thread, it would be extremely helpful
> if you add a comment on the `phy` abstractions module that explains
> how `PHYLIB` is implemented. Explain that it takes care of locking
> and other safety related things.

From my understanding, the callers of suspend() try to call suspend()
for a device only once. They lock a device and get the current state
and update the sate, then unlock the device. If the state is a
paticular value, then call suspend(). suspend() and resume() are also
called where only one thread can access a device.


>>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>>> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
>>>> +        // `Device` type being transparent makes the cast ok.
>>>> +        unsafe { &mut *ptr.cast() }
>>>
>>> please refactor to
>>>
>>>       // CAST: ...
>>>       let ptr = ptr.cast::<Self>();
>>>       // SAFETY: ...
>>>       unsafe { &mut *ptr }
>> 
>> I can but please tell the exactly comments for after CAST and SAFETY.
>> 
>> I can't find the description of CAST comment in
>> Documentation/rust/coding-guidelines.rst. So please add why and how to
>> avoid repeating the same review comment in the future.
> 
> I haven't had the time to finish my work on the standardization of
> `SAFETY` (and also `CAST`) comments, but I am working on that.
> 
>         // 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 }

Thanks, I'll copy-and-paste it.


>>>> +    /// Returns true if auto-negotiation is completed.
>>>> +    pub fn is_autoneg_completed(&self) -> bool {
>>>> +        const AUTONEG_COMPLETED: u32 = 1;
>>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>>> +        let phydev = unsafe { *self.0.get() };
>>>> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
>>>> +    }
>>>> +
>>>> +    /// Sets the speed of the PHY.
>>>> +    pub fn set_speed(&self, speed: u32) {
>>>
>>> This function modifies state, but is `&self`?
>> 
>> Boqun asked me to drop mut on v3 review and then you ask why on v4?
>> Trying to find a way to discourage developpers to write Rust
>> abstractions? :)
>> 
>> I would recommend the Rust reviewers to make sure that such would
>> not happen. I really appreciate comments but inconsistent reviewing is
>> painful.
> 
> I agree with Boqun. Before Boqun's suggestion all functions were
> `&mut self`. Now all functions are `&self`. Both are incorrect. A
> function that takes `&mut self` can modify the state of `Self`,
> but it is weird for it to not modify anything at all. Such a
> function also can only be called by a single thread (per instance
> of `Self`) at a time. Functions with `&self` cannot modify the
> state of `Self`, except of course with interior mutability. If
> they do modify state with interior mutability, then they should
> have a good reason to do that.
> 
> What I want you to do here is think about which functions should
> be `&mut self` and which should be `&self`, since clearly just
> one or the other is wrong here.

https://lore.kernel.org/netdev/[e-mail]/T/#mb7d219b2e17d3f3e31a0d05697d91eb8205c5c6e

Hmm, I undertood that he suggested all mut.

Anyway,

phy_id()
state()
get_link()
is_autoneg_enabled()
is_autoneg_completed()

doesn't modify Self.

The rest modifies then need to be &mut self? Note that function like read_*
updates the C data structure.


>>>> +        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)
>>>> +        }
>>>> +    }
>>>
>>> [...]
>>>
>>>> +}
>>>> +
>>>> +/// Defines certain other features this PHY supports (like interrupts).
>>>
>>> Maybe add a link where these flags can be used.
>> 
>> I already put the link to here in trait Driver.
> 
> I am asking about a link here, as it is a bit confusing when
> you just stumble over this flag module here. It doesn't hurt
> to link more.

I can't find the code does the similar. What exactly do you expect?
Like this?

/// Defines certain other features this PHY supports (like interrupts) for [`Driver`]'s `FLAGS`.
pub mod flags {

>>>> +    /// 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 as_mdio_device_id(&self) -> bindings::mdio_device_id {
>>>
>>> I would name this just `mdio_device_id`.
>> 
>> Either is fine by me. Please tell me why for future reference.
> 
> Functions starting with `as_` or `to_` in Rust generally indicate
> some kind of conversion. `to_` functions generally take just `self`
> by value and `as_` conversions take just `&self`/`&mut self`. See
> `Option::as_ref` or `Option::as_mut`. This function is not really
> a conversion, rather it is a getter.

I think that Trevor suggested that name. Either works for me but I'll
go with your suggestion.

Miguel Ojeda Oct. 14, 2023, noon UTC | #7

On Sat, Oct 14, 2023 at 9:22 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> The same quesiton, 4th time?

Perhaps you should then be documenting this better in the code?

> Boqun asked me to drop mut on v3 review and then you ask why on v4?
> Trying to find a way to discourage developpers to write Rust
> abstractions? :)
>
> I would recommend the Rust reviewers to make sure that such would
> not happen. I really appreciate comments but inconsistent reviewing is
> painful.

Different people will give you different feedback. That feedback may
be inconsistent or may pull you in different directions, which is
typically a sign that things are not clear for at least somebody. Some
feedback may be simply wrong, too. It is what it is, even if we try
our best to be consistent.

It is especially interesting that you are the one saying this, because
you were the one that wanted to go quickly to the mailing list,
including the netdev one. That is perfectly fine, but the result is
that people may not be on the same page and it will take time to
converge, especially for something new. So I am not sure what you are
complaining about.

Now, something more serious is happening here, which is you implying
that reviewers are intentionally trying to discourage you. That is
simply not acceptable.

Cheers,
Miguel

Benno Lossin Oct. 14, 2023, 2:54 p.m. UTC | #8

On 14.10.23 12:32, FUJITA Tomonori wrote:
> On Sat, 14 Oct 2023 08:07:03 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 14.10.23 09:22, FUJITA Tomonori wrote:
>>> On Fri, 13 Oct 2023 21:31:16 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>>
>>>> In some other thread you mentioned that no lock is held for
>>>> `resume`/`suspend`, how does this interact with it?
>>>
>>> The same quesiton, 4th time?
>>
>> Yes, it is not clear to me from the code/safety comment alone why
>> this is safe. Please improve the comment such that that is the case.
>>
>>> PHYLIB is implemented in a way that PHY drivers exlusively access to
>>> phy_device during the callbacks.
>>
>> As I suggested in a previous thread, it would be extremely helpful
>> if you add a comment on the `phy` abstractions module that explains
>> how `PHYLIB` is implemented. Explain that it takes care of locking
>> and other safety related things.
> 
>  From my understanding, the callers of suspend() try to call suspend()
> for a device only once. They lock a device and get the current state
> and update the sate, then unlock the device. If the state is a
> paticular value, then call suspend(). suspend() and resume() are also
> called where only one thread can access a device.

Maybe explain this in the docs? In the future, when I will come
into contact with this again, I will probably have forgotten this
conversation, but the docs are permanent and can be re-read.

>>>>> +    /// Returns true if auto-negotiation is completed.
>>>>> +    pub fn is_autoneg_completed(&self) -> bool {
>>>>> +        const AUTONEG_COMPLETED: u32 = 1;
>>>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>>>> +        let phydev = unsafe { *self.0.get() };
>>>>> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
>>>>> +    }
>>>>> +
>>>>> +    /// Sets the speed of the PHY.
>>>>> +    pub fn set_speed(&self, speed: u32) {
>>>>
>>>> This function modifies state, but is `&self`?
>>>
>>> Boqun asked me to drop mut on v3 review and then you ask why on v4?
>>> Trying to find a way to discourage developpers to write Rust
>>> abstractions? :)
>>>
>>> I would recommend the Rust reviewers to make sure that such would
>>> not happen. I really appreciate comments but inconsistent reviewing is
>>> painful.
>>
>> I agree with Boqun. Before Boqun's suggestion all functions were
>> `&mut self`. Now all functions are `&self`. Both are incorrect. A
>> function that takes `&mut self` can modify the state of `Self`,
>> but it is weird for it to not modify anything at all. Such a
>> function also can only be called by a single thread (per instance
>> of `Self`) at a time. Functions with `&self` cannot modify the
>> state of `Self`, except of course with interior mutability. If
>> they do modify state with interior mutability, then they should
>> have a good reason to do that.
>>
>> What I want you to do here is think about which functions should
>> be `&mut self` and which should be `&self`, since clearly just
>> one or the other is wrong here.
> 
> https://lore.kernel.org/netdev/[e-mail]/T/#mb7d219b2e17d3f3e31a0d05697d91eb8205c5c6e
> 
> Hmm, I undertood that he suggested all mut.

That remark seems to me to only apply to the return type of
`assume_locked` in that thread.

> Anyway,
> 
> phy_id()
> state()
> get_link()
> is_autoneg_enabled()
> is_autoneg_completed()
> 
> doesn't modify Self.

yes, these should all be `&self`.

> The rest modifies then need to be &mut self? Note that function like read_*
> updates the C data structure.

What exactly does it update? In Rust there is interior mutability
which is used to implement mutexes. Interior mutability allows
you to modify values despite only having a `&T` (for more info
see [1]). Our `Opaque<T>` type uses this pattern as well (since
you get a `*mut T` from `&Opaque<T>`) and it is the job of the
abstraction writer to figure out what mutability to use.

[1]: https://doc.rust-lang.org/reference/interior-mutability.html

I have no idea what exactly `read_*` modifies on the C side.
Mapping C functions to `&self`, `&mut self` and other receiver types
is not obvious in all cases. I would focus more on the following aspect
of `&mut self` and `&self`:

Since `&mut self` is unique, only one thread per instance of `Self`
can call that function. So use this when the C side would use a lock.
(or requires that only one thread calls that code)

Since multiple `&self` references are allowed to coexist, you should
use this for functions which perform their own serialization/do not
require serialization.

If you cannot decide what certain function receivers should be, then
we can help you, but I would need more info on what the C side is doing.

>>>>> +        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)
>>>>> +        }
>>>>> +    }
>>>>
>>>> [...]
>>>>
>>>>> +}
>>>>> +
>>>>> +/// Defines certain other features this PHY supports (like interrupts).
>>>>
>>>> Maybe add a link where these flags can be used.
>>>
>>> I already put the link to here in trait Driver.
>>
>> I am asking about a link here, as it is a bit confusing when
>> you just stumble over this flag module here. It doesn't hurt
>> to link more.
> 
> I can't find the code does the similar. What exactly do you expect?
> Like this?
> 
> /// Defines certain other features this PHY supports (like interrupts) for [`Driver`]'s `FLAGS`.

IIRC you can directly link to the field:

     [`Driver::FLAGS`]

Also maybe split the sentence. So one idea would be:

     /// Defines certain other features this PHY supports (like interrupts).
     ///
     /// These flag values are used in [`Driver::FLAGS`].

Boqun Feng Oct. 14, 2023, 3:53 p.m. UTC | #9

On Sat, Oct 14, 2023 at 02:54:30PM +0000, Benno Lossin wrote:
[...]
> >>>
> >>> Boqun asked me to drop mut on v3 review and then you ask why on v4?
> >>> Trying to find a way to discourage developpers to write Rust
> >>> abstractions? :)
> >>>
> >>> I would recommend the Rust reviewers to make sure that such would
> >>> not happen. I really appreciate comments but inconsistent reviewing is
> >>> painful.
> >>
> >> I agree with Boqun. Before Boqun's suggestion all functions were
> >> `&mut self`. Now all functions are `&self`. Both are incorrect. A
> >> function that takes `&mut self` can modify the state of `Self`,
> >> but it is weird for it to not modify anything at all. Such a
> >> function also can only be called by a single thread (per instance
> >> of `Self`) at a time. Functions with `&self` cannot modify the
> >> state of `Self`, except of course with interior mutability. If
> >> they do modify state with interior mutability, then they should
> >> have a good reason to do that.
> >>
> >> What I want you to do here is think about which functions should
> >> be `&mut self` and which should be `&self`, since clearly just
> >> one or the other is wrong here.
> > 
> > https://lore.kernel.org/netdev/[e-mail]/T/#mb7d219b2e17d3f3e31a0d05697d91eb8205c5c6e
> > 
> > Hmm, I undertood that he suggested all mut.
> 

To be clear, I was only talking about phy_id() at the email thread,

My original reply:

> >>> >> +    pub fn phy_id(&mut self) -> u32 {
> >>> > 
> >>> > This function doesn't modify the `self`, why does this need to be a
> >>> > `&mut self` function? Ditto for a few functions in this impl block.

so Tomo, I wasn't suggesting dropping `mut` for all functions (I used
the words "a few" not "all"), just dropping them accordingly.

Actually this is an excellent example for the fragile of relying on
implicit requirements ;-) The original intent got lost track in a few
email exchanges. 

API soundness is not the sliver bullet, but it at least tries to bring
a common base for handling such problems. Again maybe you now can
understand why we push so hard on "tiny" things again and again.

Also of course Rust is still somehow immature, so if you have an idea
but cannot express in Rust, feel free to call it out, we can work
together to 1) find a temp solution and 2) push Rust to improve. That's
one of the points of the experiment.

Regards,
Boqun

> That remark seems to me to only apply to the return type of
> `assume_locked` in that thread.
>

FUJITA Tomonori Oct. 14, 2023, 4:15 p.m. UTC | #10

On Sat, 14 Oct 2023 14:54:30 +0000
Benno Lossin <[e-mail]> wrote:

> On 14.10.23 12:32, FUJITA Tomonori wrote:
>> On Sat, 14 Oct 2023 08:07:03 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>> On 14.10.23 09:22, FUJITA Tomonori wrote:
>>>> On Fri, 13 Oct 2023 21:31:16 +0000
>>>> Benno Lossin <[e-mail]> wrote:
>>>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>>>
>>>>> In some other thread you mentioned that no lock is held for
>>>>> `resume`/`suspend`, how does this interact with it?
>>>>
>>>> The same quesiton, 4th time?
>>>
>>> Yes, it is not clear to me from the code/safety comment alone why
>>> this is safe. Please improve the comment such that that is the case.
>>>
>>>> PHYLIB is implemented in a way that PHY drivers exlusively access to
>>>> phy_device during the callbacks.
>>>
>>> As I suggested in a previous thread, it would be extremely helpful
>>> if you add a comment on the `phy` abstractions module that explains
>>> how `PHYLIB` is implemented. Explain that it takes care of locking
>>> and other safety related things.
>> 
>>  From my understanding, the callers of suspend() try to call suspend()
>> for a device only once. They lock a device and get the current state
>> and update the sate, then unlock the device. If the state is a
>> paticular value, then call suspend(). suspend() and resume() are also
>> called where only one thread can access a device.
> 
> Maybe explain this in the docs? In the future, when I will come
> into contact with this again, I will probably have forgotten this
> conversation, but the docs are permanent and can be re-read.

You meant adding this to the code?  like dding this to Device's #
Safety comment?


>> Anyway,
>> 
>> phy_id()
>> state()
>> get_link()
>> is_autoneg_enabled()
>> is_autoneg_completed()
>> 
>> doesn't modify Self.
> 
> yes, these should all be `&self`.
> 
>> The rest modifies then need to be &mut self? Note that function like read_*
>> updates the C data structure.
> 
> What exactly does it update? In Rust there is interior mutability
> which is used to implement mutexes. Interior mutability allows
> you to modify values despite only having a `&T` (for more info
> see [1]). Our `Opaque<T>` type uses this pattern as well (since
> you get a `*mut T` from `&Opaque<T>`) and it is the job of the
> abstraction writer to figure out what mutability to use.
> 
> [1]: https://doc.rust-lang.org/reference/interior-mutability.html
> 
> I have no idea what exactly `read_*` modifies on the C side.
> Mapping C functions to `&self`, `&mut self` and other receiver types
> is not obvious in all cases. I would focus more on the following aspect
> of `&mut self` and `&self`:
> 
> Since `&mut self` is unique, only one thread per instance of `Self`
> can call that function. So use this when the C side would use a lock.
> (or requires that only one thread calls that code)

I guess that the rest are &mut self but let me continue to make sure.

I think that you already know that Device instance only was created in
the callbacks. Before the callbacks are called, PHYLIB holds
phydev->lock except for resume()/suspend(). As explained in the
previous mail, only one thread calls resume()/suspend().

btw, methods in Device calling a C side function like mdiobus_read,
mdiobus_write, etc which never touch phydev->lock. Note that the c
side functions in resume()/suspned() methods don't touch phydev->lock
too.

There are two types how the methods in Device changes the C side data.

1. read/write/read_paged

They call the C side functions, mdiobus_read, mdiobus_write,
phy_read_paged, respectively.

phy_device has a pointer to mii_bus object. It has stats for
read/write. So everytime they are called, stats is updated.

2. the rest

The C side functions in the rest of methods in Device updates some
members in phy_device like set_speed() method does.


> Since multiple `&self` references are allowed to coexist, you should
> use this for functions which perform their own serialization/do not
> require serialization.

just to be sure, the C side guarantees that only one reference exists.


> If you cannot decide what certain function receivers should be, then
> we can help you, but I would need more info on what the C side is doing.

If you need more info on the C side, please let me know.


>>>>>> +/// Defines certain other features this PHY supports (like interrupts).
>>>>>
>>>>> Maybe add a link where these flags can be used.
>>>>
>>>> I already put the link to here in trait Driver.
>>>
>>> I am asking about a link here, as it is a bit confusing when
>>> you just stumble over this flag module here. It doesn't hurt
>>> to link more.
>> 
>> I can't find the code does the similar. What exactly do you expect?
>> Like this?
>> 
>> /// Defines certain other features this PHY supports (like interrupts) for [`Driver`]'s `FLAGS`.
> 
> IIRC you can directly link to the field:
> 
>      [`Driver::FLAGS`]
> 
> Also maybe split the sentence. So one idea would be:
> 
>      /// Defines certain other features this PHY supports (like interrupts).
>      ///
>      /// These flag values are used in [`Driver::FLAGS`].

Thanks, will do.

Miguel Ojeda Oct. 14, 2023, 5 p.m. UTC | #11

On Sat, Oct 14, 2023 at 4:13 AM Andrew Lunn <[e-mail]> wrote:
>
> To some extent, this is just a temporary location. Once the
> restrictions of the build systems are solved, i expect this will move
> into drivers/net/phy/Kconfig, inside the 'if PHYLIB'. However, i
> agree, this should be under the Rust menu.

No, it is orthogonal to the build system restrictions.

In other words, the Kconfig entry could be moved there already. In
fact, I would suggest so.

Cheers,
Miguel

Benno Lossin Oct. 14, 2023, 5:07 p.m. UTC | #12

On 14.10.23 18:15, FUJITA Tomonori wrote:
> On Sat, 14 Oct 2023 14:54:30 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 14.10.23 12:32, FUJITA Tomonori wrote:
>>> On Sat, 14 Oct 2023 08:07:03 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>
>>>> On 14.10.23 09:22, FUJITA Tomonori wrote:
>>>>> On Fri, 13 Oct 2023 21:31:16 +0000
>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>>>>
>>>>>> In some other thread you mentioned that no lock is held for
>>>>>> `resume`/`suspend`, how does this interact with it?
>>>>>
>>>>> The same quesiton, 4th time?
>>>>
>>>> Yes, it is not clear to me from the code/safety comment alone why
>>>> this is safe. Please improve the comment such that that is the case.
>>>>
>>>>> PHYLIB is implemented in a way that PHY drivers exlusively access to
>>>>> phy_device during the callbacks.
>>>>
>>>> As I suggested in a previous thread, it would be extremely helpful
>>>> if you add a comment on the `phy` abstractions module that explains
>>>> how `PHYLIB` is implemented. Explain that it takes care of locking
>>>> and other safety related things.
>>>
>>>   From my understanding, the callers of suspend() try to call suspend()
>>> for a device only once. They lock a device and get the current state
>>> and update the sate, then unlock the device. If the state is a
>>> paticular value, then call suspend(). suspend() and resume() are also
>>> called where only one thread can access a device.
>>
>> Maybe explain this in the docs? In the future, when I will come
>> into contact with this again, I will probably have forgotten this
>> conversation, but the docs are permanent and can be re-read.
> 
> You meant adding this to the code?  like dding this to Device's #
> Safety comment?

I would not put it in the `# Safety` section. Instead, put this
information on the phy module itself (the `//!` comments at the very
top of the file). I also would suggest to not take the above paragraph
word by word, but to improve and extend it.

>>> Anyway,
>>>
>>> phy_id()
>>> state()
>>> get_link()
>>> is_autoneg_enabled()
>>> is_autoneg_completed()
>>>
>>> doesn't modify Self.
>>
>> yes, these should all be `&self`.
>>
>>> The rest modifies then need to be &mut self? Note that function like read_*
>>> updates the C data structure.
>>
>> What exactly does it update? In Rust there is interior mutability
>> which is used to implement mutexes. Interior mutability allows
>> you to modify values despite only having a `&T` (for more info
>> see [1]). Our `Opaque<T>` type uses this pattern as well (since
>> you get a `*mut T` from `&Opaque<T>`) and it is the job of the
>> abstraction writer to figure out what mutability to use.
>>
>> [1]: https://doc.rust-lang.org/reference/interior-mutability.html
>>
>> I have no idea what exactly `read_*` modifies on the C side.
>> Mapping C functions to `&self`, `&mut self` and other receiver types
>> is not obvious in all cases. I would focus more on the following aspect
>> of `&mut self` and `&self`:
>>
>> Since `&mut self` is unique, only one thread per instance of `Self`
>> can call that function. So use this when the C side would use a lock.
>> (or requires that only one thread calls that code)
> 
> I guess that the rest are &mut self but let me continue to make sure.
> 
> I think that you already know that Device instance only was created in
> the callbacks. Before the callbacks are called, PHYLIB holds
> phydev->lock except for resume()/suspend(). As explained in the
> previous mail, only one thread calls resume()/suspend().

The information in this paragraph would also fit nicely into the
phy module docs.

> btw, methods in Device calling a C side function like mdiobus_read,
> mdiobus_write, etc which never touch phydev->lock. Note that the c
> side functions in resume()/suspned() methods don't touch phydev->lock
> too.
> 
> There are two types how the methods in Device changes the C side data.
> 
> 1. read/write/read_paged
> 
> They call the C side functions, mdiobus_read, mdiobus_write,
> phy_read_paged, respectively.
> 
> phy_device has a pointer to mii_bus object. It has stats for
> read/write. So everytime they are called, stats is updated.

I think for reading & updating some stats using `&self`
should be fine. `write` should probably be `&mut self`.

> 2. the rest
> 
> The C side functions in the rest of methods in Device updates some
> members in phy_device like set_speed() method does.

Those setter functions should be `&mut self`.

>> Since multiple `&self` references are allowed to coexist, you should
>> use this for functions which perform their own serialization/do not
>> require serialization.
> 
> just to be sure, the C side guarantees that only one reference exists.

I see, then the `from_raw` function should definitely return
a `&mut Device`. Note that you can still call `&T` functions
when you have a `&mut T`.

>> If you cannot decide what certain function receivers should be, then
>> we can help you, but I would need more info on what the C side is doing.
> 
> If you need more info on the C side, please let me know.

What about these functions?
- resolve_aneg_linkmode
- genphy_soft_reset
- init_hw
- start_aneg
- genphy_read_status
- genphy_update_link
- genphy_read_lpa
- genphy_read_abilities

Andrew Lunn Oct. 14, 2023, 9:18 p.m. UTC | #13

> What about these functions?
> - resolve_aneg_linkmode
> - genphy_soft_reset
> - init_hw
> - start_aneg
> - genphy_read_status
> - genphy_update_link
> - genphy_read_lpa
> - genphy_read_abilities

If i'm understanding the discussion correctly, you want to know if
these C functions can modify the phydev structure that is passed to
them?

Yes, they all do modify it.

They also assume that phydev->lock is taken somewhere up the call
chain, so they are safe to modify the structure without worrying about
multiple threads being active.

There are some functions which currently don't modify the phydev
passed to them. However, we are pretty bad at putting on the const
attribute. I also think it would be dangerous to assume such functions
will forever not modify phydev.

	Andrew

FUJITA Tomonori Oct. 14, 2023, 10:39 p.m. UTC | #14

On Sat, 14 Oct 2023 17:07:09 +0000
Benno Lossin <[e-mail]> wrote:

> On 14.10.23 18:15, FUJITA Tomonori wrote:
>> On Sat, 14 Oct 2023 14:54:30 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>> On 14.10.23 12:32, FUJITA Tomonori wrote:
>>>> On Sat, 14 Oct 2023 08:07:03 +0000
>>>> Benno Lossin <[e-mail]> wrote:
>>>>
>>>>> On 14.10.23 09:22, FUJITA Tomonori wrote:
>>>>>> On Fri, 13 Oct 2023 21:31:16 +0000
>>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>>>>>
>>>>>>> In some other thread you mentioned that no lock is held for
>>>>>>> `resume`/`suspend`, how does this interact with it?
>>>>>>
>>>>>> The same quesiton, 4th time?
>>>>>
>>>>> Yes, it is not clear to me from the code/safety comment alone why
>>>>> this is safe. Please improve the comment such that that is the case.
>>>>>
>>>>>> PHYLIB is implemented in a way that PHY drivers exlusively access to
>>>>>> phy_device during the callbacks.
>>>>>
>>>>> As I suggested in a previous thread, it would be extremely helpful
>>>>> if you add a comment on the `phy` abstractions module that explains
>>>>> how `PHYLIB` is implemented. Explain that it takes care of locking
>>>>> and other safety related things.
>>>>
>>>>   From my understanding, the callers of suspend() try to call suspend()
>>>> for a device only once. They lock a device and get the current state
>>>> and update the sate, then unlock the device. If the state is a
>>>> paticular value, then call suspend(). suspend() and resume() are also
>>>> called where only one thread can access a device.
>>>
>>> Maybe explain this in the docs? In the future, when I will come
>>> into contact with this again, I will probably have forgotten this
>>> conversation, but the docs are permanent and can be re-read.
>> 
>> You meant adding this to the code?  like dding this to Device's #
>> Safety comment?
> 
> I would not put it in the `# Safety` section. Instead, put this
> information on the phy module itself (the `//!` comments at the very
> top of the file). I also would suggest to not take the above paragraph
> word by word, but to improve and extend it.

Sure, I'll try.


>>>> Anyway,
>>>>
>>>> phy_id()
>>>> state()
>>>> get_link()
>>>> is_autoneg_enabled()
>>>> is_autoneg_completed()
>>>>
>>>> doesn't modify Self.
>>>
>>> yes, these should all be `&self`.
>>>
>>>> The rest modifies then need to be &mut self? Note that function like read_*
>>>> updates the C data structure.
>>>
>>> What exactly does it update? In Rust there is interior mutability
>>> which is used to implement mutexes. Interior mutability allows
>>> you to modify values despite only having a `&T` (for more info
>>> see [1]). Our `Opaque<T>` type uses this pattern as well (since
>>> you get a `*mut T` from `&Opaque<T>`) and it is the job of the
>>> abstraction writer to figure out what mutability to use.
>>>
>>> [1]: https://doc.rust-lang.org/reference/interior-mutability.html
>>>
>>> I have no idea what exactly `read_*` modifies on the C side.
>>> Mapping C functions to `&self`, `&mut self` and other receiver types
>>> is not obvious in all cases. I would focus more on the following aspect
>>> of `&mut self` and `&self`:
>>>
>>> Since `&mut self` is unique, only one thread per instance of `Self`
>>> can call that function. So use this when the C side would use a lock.
>>> (or requires that only one thread calls that code)
>> 
>> I guess that the rest are &mut self but let me continue to make sure.
>> 
>> I think that you already know that Device instance only was created in
>> the callbacks. Before the callbacks are called, PHYLIB holds
>> phydev->lock except for resume()/suspend(). As explained in the
>> previous mail, only one thread calls resume()/suspend().
> 
> The information in this paragraph would also fit nicely into the
> phy module docs.

Ok.

>> btw, methods in Device calling a C side function like mdiobus_read,
>> mdiobus_write, etc which never touch phydev->lock. Note that the c
>> side functions in resume()/suspned() methods don't touch phydev->lock
>> too.
>> 
>> There are two types how the methods in Device changes the C side data.
>> 
>> 1. read/write/read_paged
>> 
>> They call the C side functions, mdiobus_read, mdiobus_write,
>> phy_read_paged, respectively.
>> 
>> phy_device has a pointer to mii_bus object. It has stats for
>> read/write. So everytime they are called, stats is updated.
> 
> I think for reading & updating some stats using `&self`
> should be fine. `write` should probably be `&mut self`.

Can you tell me why exactly you think in that way?

Firstly, you think that reading & updating some stats using `&self` should be fine.

What's the difference between read() and set_speed(), which you think, needs &mut self.

Because set_speed() updates the member in phy_device and read()
updates the object that phy_device points to?


Secondly, What's the difference between read() and write(), where you
think that read() is &self write() is &mut self.

read() is reading from hardware register. write() is writing a value
to hardware register. Both updates the object that phy_device points
to?


>> 2. the rest
>> 
>> The C side functions in the rest of methods in Device updates some
>> members in phy_device like set_speed() method does.
> 
> Those setter functions should be `&mut self`.

Ok.

>>> Since multiple `&self` references are allowed to coexist, you should
>>> use this for functions which perform their own serialization/do not
>>> require serialization.
>> 
>> just to be sure, the C side guarantees that only one reference exists.
> 
> I see, then the `from_raw` function should definitely return
> a `&mut Device`. Note that you can still call `&T` functions
> when you have a `&mut T`.

It already returns &mut Device so no change is necessary here, right?

unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self
{

If you want more additional comment on from_raw(), please let me know.


>>> If you cannot decide what certain function receivers should be, then
>>> we can help you, but I would need more info on what the C side is doing.
>> 
>> If you need more info on the C side, please let me know.
> 
> What about these functions?
> - resolve_aneg_linkmode
> - genphy_soft_reset
> - init_hw
> - start_aneg
> - genphy_read_status
> - genphy_update_link
> - genphy_read_lpa
> - genphy_read_abilities

As Andrew replied, all the functions update some member in phy_device.

FUJITA Tomonori Oct. 14, 2023, 11:18 p.m. UTC | #15

On Sat, 14 Oct 2023 19:00:46 +0200
Miguel Ojeda <[e-mail]> wrote:

> On Sat, Oct 14, 2023 at 4:13 AM Andrew Lunn <[e-mail]> wrote:
>>
>> To some extent, this is just a temporary location. Once the
>> restrictions of the build systems are solved, i expect this will move
>> into drivers/net/phy/Kconfig, inside the 'if PHYLIB'. However, i
>> agree, this should be under the Rust menu.
> 
> No, it is orthogonal to the build system restrictions.
> 
> In other words, the Kconfig entry could be moved there already. In
> fact, I would suggest so.

Andrew, if you prefer, I'll move RUST_PHYLIB_ABSTRACTIONS to
drivers/net/phy/Kconfig.

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index e55b71937f01..0d39b97a546c 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -66,6 +66,14 @@ config SFP
 	depends on HWMON || HWMON=n
 	select MDIO_I2C
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "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.
+
 comment "MII PHY device drivers"
 
 config AMD_PHY

Andrew Lunn Oct. 15, 2023, 3:47 p.m. UTC | #16

> Andrew, if you prefer, I'll move RUST_PHYLIB_ABSTRACTIONS to
> drivers/net/phy/Kconfig.
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index e55b71937f01..0d39b97a546c 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -66,6 +66,14 @@ config SFP
>  	depends on HWMON || HWMON=n
>  	select MDIO_I2C
>  
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "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.
> +

I'm nit picking, but i would actually put it between FIXED_PHY and
SFP. But otherwise, i'm happy with this. Putting it somewhere here is
the correct thing to do.

Thanks
	Andrew

Benno Lossin Oct. 17, 2023, 7:06 a.m. UTC | #17

On 15.10.23 00:39, FUJITA Tomonori wrote:
> On Sat, 14 Oct 2023 17:07:09 +0000
> Benno Lossin <[e-mail]> wrote:
>>> btw, methods in Device calling a C side function like mdiobus_read,
>>> mdiobus_write, etc which never touch phydev->lock. Note that the c
>>> side functions in resume()/suspned() methods don't touch phydev->lock
>>> too.
>>>
>>> There are two types how the methods in Device changes the C side data.
>>>
>>> 1. read/write/read_paged
>>>
>>> They call the C side functions, mdiobus_read, mdiobus_write,
>>> phy_read_paged, respectively.
>>>
>>> phy_device has a pointer to mii_bus object. It has stats for
>>> read/write. So everytime they are called, stats is updated.
>>
>> I think for reading & updating some stats using `&self`
>> should be fine. `write` should probably be `&mut self`.
> 
> Can you tell me why exactly you think in that way?
> 
> Firstly, you think that reading & updating some stats using `&self` should be fine.
> 
> What's the difference between read() and set_speed(), which you think, needs &mut self.
> 
> Because set_speed() updates the member in phy_device and read()
> updates the object that phy_device points to?

`set_speed` is entirely implemented on the Rust side and is not protected
by a lock. Since data races in Rust are UB, this function must be `&mut`,
in order to guarantee that no data races occur. This is the case, because
our `Opaque` forces you to use interior mutability and thus sidestep this
rule (modifying through a `&T`).

> 
> 
> Secondly, What's the difference between read() and write(), where you
> think that read() is &self write() is &mut self.

This is just the standard Rust way of using mutability. For reading one
uses `&self` and for writing `&mut self`. The only thing that is special
here is the stats that are updated. But I thought that it still could fit
Rust by the following pattern:
```rust
     pub struct TrackingReader {
         buf: [u8; 64],
         num_of_reads: Mutex<usize>,
     }

     impl TrackingReader {
         pub fn read(&self, idx: usize) -> u8 {
             *self.num_of_reads.lock() += 1;
             self.buf[idx]
         }
     }

```

And after taking a look at `mdiobus_read` I indeed found a mutex.

> read() is reading from hardware register. write() is writing a value
> to hardware register. Both updates the object that phy_device points
> to?

Indeed, I was just going with the standard way of suggesting `&self`
for reads, there are of course exceptions where `&mut self` would make
sense. That being said in this case both options are sound, since
the C side locks a mutex.

>>>> Since multiple `&self` references are allowed to coexist, you should
>>>> use this for functions which perform their own serialization/do not
>>>> require serialization.
>>>
>>> just to be sure, the C side guarantees that only one reference exists.
>>
>> I see, then the `from_raw` function should definitely return
>> a `&mut Device`. Note that you can still call `&T` functions
>> when you have a `&mut T`.
> 
> It already returns &mut Device so no change is necessary here, right?

Yes it already is correct.

> unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self
> {
> 
> If you want more additional comment on from_raw(), please let me know.
> 
> 
>>>> If you cannot decide what certain function receivers should be, then
>>>> we can help you, but I would need more info on what the C side is doing.
>>>
>>> If you need more info on the C side, please let me know.
>>
>> What about these functions?
>> - resolve_aneg_linkmode
>> - genphy_soft_reset
>> - init_hw
>> - start_aneg
>> - genphy_read_status
>> - genphy_update_link
>> - genphy_read_lpa
>> - genphy_read_abilities
> 
> As Andrew replied, all the functions update some member in phy_device.

Do all of these functions lock the `bus->mdio_lock`? If yes, then you
can just treat them like `read` or `write` (both `&self` and `&mut self`
will be sound) and use the standard Rust way of setting the mutability.
So if it changes some internal state, I would go with `&mut self`.

FUJITA Tomonori Oct. 17, 2023, 7:32 a.m. UTC | #18

On Tue, 17 Oct 2023 07:06:38 +0000
Benno Lossin <[e-mail]> wrote:

> On 15.10.23 00:39, FUJITA Tomonori wrote:
>> On Sat, 14 Oct 2023 17:07:09 +0000
>> Benno Lossin <[e-mail]> wrote:
>>>> btw, methods in Device calling a C side function like mdiobus_read,
>>>> mdiobus_write, etc which never touch phydev->lock. Note that the c
>>>> side functions in resume()/suspned() methods don't touch phydev->lock
>>>> too.
>>>>
>>>> There are two types how the methods in Device changes the C side data.
>>>>
>>>> 1. read/write/read_paged
>>>>
>>>> They call the C side functions, mdiobus_read, mdiobus_write,
>>>> phy_read_paged, respectively.
>>>>
>>>> phy_device has a pointer to mii_bus object. It has stats for
>>>> read/write. So everytime they are called, stats is updated.
>>>
>>> I think for reading & updating some stats using `&self`
>>> should be fine. `write` should probably be `&mut self`.
>> 
>> Can you tell me why exactly you think in that way?
>> 
>> Firstly, you think that reading & updating some stats using `&self` should be fine.
>> 
>> What's the difference between read() and set_speed(), which you think, needs &mut self.
>> 
>> Because set_speed() updates the member in phy_device and read()
>> updates the object that phy_device points to?
> 
> `set_speed` is entirely implemented on the Rust side and is not protected
> by a lock. Since data races in Rust are UB, this function must be `&mut`,
> in order to guarantee that no data races occur. This is the case, because
> our `Opaque` forces you to use interior mutability and thus sidestep this
> rule (modifying through a `&T`).

Understood.


>> Secondly, What's the difference between read() and write(), where you
>> think that read() is &self write() is &mut self.
> 
> This is just the standard Rust way of using mutability. For reading one
> uses `&self` and for writing `&mut self`. The only thing that is special
> here is the stats that are updated. But I thought that it still could fit
> Rust by the following pattern:
> ```rust
>      pub struct TrackingReader {
>          buf: [u8; 64],
>          num_of_reads: Mutex<usize>,
>      }
> 
>      impl TrackingReader {
>          pub fn read(&self, idx: usize) -> u8 {
>              *self.num_of_reads.lock() += 1;
>              self.buf[idx]
>          }
>      }
> 
> ```
> 
> And after taking a look at `mdiobus_read` I indeed found a mutex.

Yes, both read() and write() update the stats with mdiobus's lock.


>> read() is reading from hardware register. write() is writing a value
>> to hardware register. Both updates the object that phy_device points
>> to?
> 
> Indeed, I was just going with the standard way of suggesting `&self`
> for reads, there are of course exceptions where `&mut self` would make
> sense. That being said in this case both options are sound, since
> the C side locks a mutex.

I see. I use &mut self for both read() and write().


>>>>> If you cannot decide what certain function receivers should be, then
>>>>> we can help you, but I would need more info on what the C side is doing.
>>>>
>>>> If you need more info on the C side, please let me know.
>>>
>>> What about these functions?
>>> - resolve_aneg_linkmode
>>> - genphy_soft_reset
>>> - init_hw
>>> - start_aneg
>>> - genphy_read_status
>>> - genphy_update_link
>>> - genphy_read_lpa
>>> - genphy_read_abilities
>> 
>> As Andrew replied, all the functions update some member in phy_device.
> 
> Do all of these functions lock the `bus->mdio_lock`? If yes, then you
> can just treat them like `read` or `write` (both `&self` and `&mut self`
> will be sound) and use the standard Rust way of setting the mutability.
> So if it changes some internal state, I would go with `&mut self`.

They hold mdiobus's lock and update mdiobus stats. They also change
some internal state in phy_device without touching any lock (it's safe
since PHYLIB guarantes there is only one thread calling a callback for
one device).

I'll use &mut self for them.

Thanks a lot!

Benno Lossin Oct. 17, 2023, 7:41 a.m. UTC | #19

On 17.10.23 09:32, FUJITA Tomonori wrote:
> On Tue, 17 Oct 2023 07:06:38 +0000
> Benno Lossin <[e-mail]> wrote:
>>> Secondly, What's the difference between read() and write(), where you
>>> think that read() is &self write() is &mut self.
>>
>> This is just the standard Rust way of using mutability. For reading one
>> uses `&self` and for writing `&mut self`. The only thing that is special
>> here is the stats that are updated. But I thought that it still could fit
>> Rust by the following pattern:
>> ```rust
>>       pub struct TrackingReader {
>>           buf: [u8; 64],
>>           num_of_reads: Mutex<usize>,
>>       }
>>
>>       impl TrackingReader {
>>           pub fn read(&self, idx: usize) -> u8 {
>>               *self.num_of_reads.lock() += 1;
>>               self.buf[idx]
>>           }
>>       }
>>
>> ```
>>
>> And after taking a look at `mdiobus_read` I indeed found a mutex.
> 
> Yes, both read() and write() update the stats with mdiobus's lock.
> 
> 
>>> read() is reading from hardware register. write() is writing a value
>>> to hardware register. Both updates the object that phy_device points
>>> to?
>>
>> Indeed, I was just going with the standard way of suggesting `&self`
>> for reads, there are of course exceptions where `&mut self` would make
>> sense. That being said in this case both options are sound, since
>> the C side locks a mutex.
> 
> I see. I use &mut self for both read() and write().

I would recommend documenting this somewhere (why `read` is `&mut`), since
that is a bit unusual (why restrict something more than necessary?).

FUJITA Tomonori Oct. 17, 2023, 11:32 a.m. UTC | #20

On Tue, 17 Oct 2023 07:41:38 +0000
Benno Lossin <[e-mail]> wrote:

>>>> read() is reading from hardware register. write() is writing a value
>>>> to hardware register. Both updates the object that phy_device points
>>>> to?
>>>
>>> Indeed, I was just going with the standard way of suggesting `&self`
>>> for reads, there are of course exceptions where `&mut self` would make
>>> sense. That being said in this case both options are sound, since
>>> the C side locks a mutex.
>> 
>> I see. I use &mut self for both read() and write().
> 
> I would recommend documenting this somewhere (why `read` is `&mut`), since
> that is a bit unusual (why restrict something more than necessary?).

I added such at the top of the file.

Andrew Lunn Oct. 17, 2023, 12:38 p.m. UTC | #21

> > Because set_speed() updates the member in phy_device and read()
> > updates the object that phy_device points to?
> 
> `set_speed` is entirely implemented on the Rust side and is not protected
> by a lock.

With the current driver, all entry points into the driver are called
from the phylib core, and the core guarantees that the lock is
taken. So it should not matter if its entirely implemented in the Rust
side, somewhere up the call stack, the lock was taken.

> >> What about these functions?
> >> - resolve_aneg_linkmode
> >> - genphy_soft_reset
> >> - init_hw
> >> - start_aneg
> >> - genphy_read_status
> >> - genphy_update_link
> >> - genphy_read_lpa
> >> - genphy_read_abilities
> > 
> > As Andrew replied, all the functions update some member in phy_device.
> 
> Do all of these functions lock the `bus->mdio_lock`?

When accessing the hardware, yes.

The basic architecture is that at the bottom we have an MDIO bus, and
on top of that bus, we have a number of devices. The MDIO core will
serialise access to the bus, so only one device on the bus can be
accessed at once. The phylib core will serialise access to the PHY,
but when there are multiple PHYs, the phylib core will allow parallel
access to different PHYs.

In summary, the core of each layer protects the drivers using that
layer from multiple parallel accesses from above.

       Andrew

Benno Lossin Oct. 17, 2023, 2:04 p.m. UTC | #22

On 17.10.23 14:38, Andrew Lunn wrote:
>>> Because set_speed() updates the member in phy_device and read()
>>> updates the object that phy_device points to?
>>
>> `set_speed` is entirely implemented on the Rust side and is not protected
>> by a lock.
> 
> With the current driver, all entry points into the driver are called
> from the phylib core, and the core guarantees that the lock is
> taken. So it should not matter if its entirely implemented in the Rust
> side, somewhere up the call stack, the lock was taken.

Sure that might be the case, I am trying to guard against this future
problem:

     fn soft_reset(driver: &mut Driver) -> Result {
         let driver = driver
         thread::scope(|s| {
             let thread_a = s.spawn(|| {
                 for _ in 0..100_000_000 {
                     driver.set_speed(10);
                 }
             });
             let thread_b = s.spawn(|| {
                 for _ in 0..100_000_000 {
                     driver.set_speed(10);
                 }
             });
             thread_a.join();
             thread_b.join();
         });
         Ok(())
     }

This code spawns two new threads both of which can call `set_speed`,
since it takes `&self`. But this leads to a data race, since those
accesses are not serialized. I know that this is a very contrived
example, but you never when this will become reality, so we should
do the right thing now and just use `&mut self`, since that is exactly
what it is for.

Not that we do not even have a way to create threads on the Rust side
at the moment. But we should already be thinking about any possible
code pattern.

>>>> What about these functions?
>>>> - resolve_aneg_linkmode
>>>> - genphy_soft_reset
>>>> - init_hw
>>>> - start_aneg
>>>> - genphy_read_status
>>>> - genphy_update_link
>>>> - genphy_read_lpa
>>>> - genphy_read_abilities
>>>
>>> As Andrew replied, all the functions update some member in phy_device.
>>
>> Do all of these functions lock the `bus->mdio_lock`?
> 
> When accessing the hardware, yes.
> 
> The basic architecture is that at the bottom we have an MDIO bus, and
> on top of that bus, we have a number of devices. The MDIO core will
> serialise access to the bus, so only one device on the bus can be
> accessed at once. The phylib core will serialise access to the PHY,
> but when there are multiple PHYs, the phylib core will allow parallel
> access to different PHYs.
> 
> In summary, the core of each layer protects the drivers using that
> layer from multiple parallel accesses from above.
Thanks for this explanation, it really helps!

Greg Kroah-Hartman Oct. 17, 2023, 2:21 p.m. UTC | #23

On Tue, Oct 17, 2023 at 02:04:33PM +0000, Benno Lossin wrote:
> On 17.10.23 14:38, Andrew Lunn wrote:
> >>> Because set_speed() updates the member in phy_device and read()
> >>> updates the object that phy_device points to?
> >>
> >> `set_speed` is entirely implemented on the Rust side and is not protected
> >> by a lock.
> > 
> > With the current driver, all entry points into the driver are called
> > from the phylib core, and the core guarantees that the lock is
> > taken. So it should not matter if its entirely implemented in the Rust
> > side, somewhere up the call stack, the lock was taken.
> 
> Sure that might be the case, I am trying to guard against this future
> problem:
> 
>      fn soft_reset(driver: &mut Driver) -> Result {
>          let driver = driver
>          thread::scope(|s| {
>              let thread_a = s.spawn(|| {
>                  for _ in 0..100_000_000 {
>                      driver.set_speed(10);
>                  }
>              });
>              let thread_b = s.spawn(|| {
>                  for _ in 0..100_000_000 {
>                      driver.set_speed(10);
>                  }
>              });
>              thread_a.join();
>              thread_b.join();
>          });
>          Ok(())
>      }
> 
> This code spawns two new threads both of which can call `set_speed`,
> since it takes `&self`. But this leads to a data race, since those
> accesses are not serialized. I know that this is a very contrived
> example, but you never when this will become reality, so we should
> do the right thing now and just use `&mut self`, since that is exactly
> what it is for.

Kernel code is written for the use cases today, don't worry about
tomorrow, you can fix the issue tomorrow if you change something that
requires it.

And what "race" are you getting here?  You don't have threads in the
kernel :)

Also, if two things are setting the speed, wonderful, you get some sort
of value eventually, you have much bigger problems in your code as you
shouldn't have been doing that in the first place.

> Not that we do not even have a way to create threads on the Rust side
> at the moment.

Which is a good thing :)

> But we should already be thinking about any possible code pattern.

Again, no, deal with what we have today, kernel code is NOT
future-proof, that's not how we write this stuff.

If you really worry about a "split write" then us a lock, that's what
they are there for.  But that's not the issue here, so don't worry about
it.

thanks,

greg k-h

Benno Lossin Oct. 17, 2023, 2:32 p.m. UTC | #24

On 17.10.23 16:21, Greg KH wrote:
> On Tue, Oct 17, 2023 at 02:04:33PM +0000, Benno Lossin wrote:
>> On 17.10.23 14:38, Andrew Lunn wrote:
>>>>> Because set_speed() updates the member in phy_device and read()
>>>>> updates the object that phy_device points to?
>>>>
>>>> `set_speed` is entirely implemented on the Rust side and is not protected
>>>> by a lock.
>>>
>>> With the current driver, all entry points into the driver are called
>>> from the phylib core, and the core guarantees that the lock is
>>> taken. So it should not matter if its entirely implemented in the Rust
>>> side, somewhere up the call stack, the lock was taken.
>>
>> Sure that might be the case, I am trying to guard against this future
>> problem:
>>
>>       fn soft_reset(driver: &mut Driver) -> Result {
>>           let driver = driver
>>           thread::scope(|s| {
>>               let thread_a = s.spawn(|| {
>>                   for _ in 0..100_000_000 {
>>                       driver.set_speed(10);
>>                   }
>>               });
>>               let thread_b = s.spawn(|| {
>>                   for _ in 0..100_000_000 {
>>                       driver.set_speed(10);
>>                   }
>>               });
>>               thread_a.join();
>>               thread_b.join();
>>           });
>>           Ok(())
>>       }
>>
>> This code spawns two new threads both of which can call `set_speed`,
>> since it takes `&self`. But this leads to a data race, since those
>> accesses are not serialized. I know that this is a very contrived
>> example, but you never when this will become reality, so we should
>> do the right thing now and just use `&mut self`, since that is exactly
>> what it is for.
> 
> Kernel code is written for the use cases today, don't worry about
> tomorrow, you can fix the issue tomorrow if you change something that
> requires it.

The kind of coding style that (mis)-uses interior mutability is not
something that we can change over night. We should do it properly to
begin with.

> And what "race" are you getting here?  You don't have threads in the
> kernel :)

I chose threads, since I am a lot more familiar with that, but the
kernel also has workqueues which execute stuff concurrently (if I
remember correctly). We also have patches for bindings for the workqueue
so they are not that far away.

> Also, if two things are setting the speed, wonderful, you get some sort
> of value eventually, you have much bigger problems in your code as you
> shouldn't have been doing that in the first place.

This is not allowed in Rust, it is UB and will lead to bad things.

>> Not that we do not even have a way to create threads on the Rust side
>> at the moment.
> 
> Which is a good thing :)
> 
>> But we should already be thinking about any possible code pattern.
> 
> Again, no, deal with what we have today, kernel code is NOT
> future-proof, that's not how we write this stuff.

While I made my argument for future proofing, I think that we
should just be using the standard Rust stuff where it applies.

When you want to modify something, use `&mut T`, if not then use
`&T`. Only deviate from this if you have a good argument.

Miguel Ojeda Oct. 17, 2023, 3:03 p.m. UTC | #25

On Tue, Oct 17, 2023 at 4:21 PM Greg KH <[e-mail]> wrote:
>
> Again, no, deal with what we have today, kernel code is NOT
> future-proof, that's not how we write this stuff.

That would make the abstractions "unsound", i.e. UB could be
introduced from safe Rust code, which is what Rust aims to prevent.

It is not so much that we care about "unwritten code" (or out-of-tree
code), but rather that it prevents having UB in users of the
abstractions.

Put another way, there may be no code today that triggers UB, but
there could be, tomorrow, with a new driver. Or when somebody modifies
a module. The goal is to simply not allow broken users to compile to
begin with.

So if we allow unsound abstractions to be merged, then we are
essentially losing that "layer" of protection that Rust gives, and
thus one of its key advantages. Instead, if we manage to keep the
abstractions sound, then we can review Rust modules that do not use
`unsafe` and statically know that they are not introducing UB.

Cheers,
Miguel

Miguel Ojeda Oct. 17, 2023, 3:17 p.m. UTC | #26

On Tue, Oct 17, 2023 at 4:32 PM Benno Lossin <[e-mail]> wrote:
>
> This is not allowed in Rust, it is UB and will lead to bad things.

Yeah, and to be clear, data races are also UB in C.

Cheers,
Miguel

Boqun Feng Oct. 17, 2023, 4:13 p.m. UTC | #27

On Tue, Oct 17, 2023 at 02:32:07PM +0000, Benno Lossin wrote:
> On 17.10.23 16:21, Greg KH wrote:
> > On Tue, Oct 17, 2023 at 02:04:33PM +0000, Benno Lossin wrote:
> >> On 17.10.23 14:38, Andrew Lunn wrote:
> >>>>> Because set_speed() updates the member in phy_device and read()
> >>>>> updates the object that phy_device points to?
> >>>>
> >>>> `set_speed` is entirely implemented on the Rust side and is not protected
> >>>> by a lock.
> >>>
> >>> With the current driver, all entry points into the driver are called
> >>> from the phylib core, and the core guarantees that the lock is
> >>> taken. So it should not matter if its entirely implemented in the Rust
> >>> side, somewhere up the call stack, the lock was taken.
> >>
> >> Sure that might be the case, I am trying to guard against this future
> >> problem:
> >>
> >>       fn soft_reset(driver: &mut Driver) -> Result {
> >>           let driver = driver
> >>           thread::scope(|s| {
> >>               let thread_a = s.spawn(|| {
> >>                   for _ in 0..100_000_000 {
> >>                       driver.set_speed(10);
> >>                   }
> >>               });
> >>               let thread_b = s.spawn(|| {
> >>                   for _ in 0..100_000_000 {
> >>                       driver.set_speed(10);
> >>                   }
> >>               });
> >>               thread_a.join();
> >>               thread_b.join();
> >>           });
> >>           Ok(())
> >>       }
> >>
> >> This code spawns two new threads both of which can call `set_speed`,
> >> since it takes `&self`. But this leads to a data race, since those
> >> accesses are not serialized. I know that this is a very contrived
> >> example, but you never when this will become reality, so we should
> >> do the right thing now and just use `&mut self`, since that is exactly
> >> what it is for.
> > 
> > Kernel code is written for the use cases today, don't worry about
> > tomorrow, you can fix the issue tomorrow if you change something that
> > requires it.
> 
> The kind of coding style that (mis)-uses interior mutability is not
> something that we can change over night. We should do it properly to
> begin with.
> 

Agreed!

Sure, the bottom line is that kernel today works, but that's the bottom
line, right? In other words, nothing prevents us from a more reasonable
API with some future guards (we are not rush for anything). In fact, the
ponit of Rust is when you do abstraction on anything you should be clear
about the semantics (exclusive accesses, lock rules, etc.), so that such
semantics can be encoded in the type system (or unsafe requirement),
this is the bottom line of using Rust. Of course, there could be cases
where Rust cannot describe correctly (or easily) with the type system,
since kernel is a complicated beast. In that case, we can either 1) tell
Rust to improve or 2) make a sane work-around and move on. But no excuse
for not trying hard today if you're using Rust: if you don't have a
clear mind about the abstraction you're writting or cannot describe
it clearly, then you probably have bad Rust code, and bad Rust code same
as bad C code is what we try to avoid in kernel.

Also, for countless times, I heard people (usually maintainers) complain
someone about "no you use this (API) in a wrong way", "not future-proof"
seems to be a root of problems ;-) and having some sort of future guards
benefits everything.

> > And what "race" are you getting here?  You don't have threads in the
> > kernel :)
> 
> I chose threads, since I am a lot more familiar with that, but the
> kernel also has workqueues which execute stuff concurrently (if I
> remember correctly). We also have patches for bindings for the workqueue
> so they are not that far away.
> 
> > Also, if two things are setting the speed, wonderful, you get some sort
> > of value eventually, you have much bigger problems in your code as you
> > shouldn't have been doing that in the first place.
> 
> This is not allowed in Rust, it is UB and will lead to bad things.
> 
> >> Not that we do not even have a way to create threads on the Rust side
> >> at the moment.
> > 
> > Which is a good thing :)
> > 
> >> But we should already be thinking about any possible code pattern.
> > 
> > Again, no, deal with what we have today, kernel code is NOT
> > future-proof, that's not how we write this stuff.
> 
> While I made my argument for future proofing, I think that we
> should just be using the standard Rust stuff where it applies.
> 
> When you want to modify something, use `&mut T`, if not then use
> `&T`. Only deviate from this if you have a good argument.
> 

Right, so if the rule for C code is 1) working for the current users +
2) subsystem requirement, then for Rust code, there is simply an extra
rule: Be clear about the concept model of the API, and use Rust
abstraction correctly (mostly it's API soundness). Again, if you find
Rust doesn't work for your stuff, we are open to discuss, quite frankly
we know there could exist such a problem and are interesting to learn
from it (or throw it to Rust language community ;-)), but ignoring Rust
rules is not a good start.

Anyway, I just saw Tomo's v5, thanks!

Regards,
Boqun

> -- 
> Cheers,
> Benno
>

Greg Kroah-Hartman Oct. 17, 2023, 4:15 p.m. UTC | #28

On Tue, Oct 17, 2023 at 05:17:40PM +0200, Miguel Ojeda wrote:
> On Tue, Oct 17, 2023 at 4:32 PM Benno Lossin <[e-mail]> wrote:
> >
> > This is not allowed in Rust, it is UB and will lead to bad things.
> 
> Yeah, and to be clear, data races are also UB in C.

And to be clear, something we totally ignore in many places in the
kernel because it flat out does not matter at all.

Think about userspace writing 2 different values to the same sysfs file
at the same time, which the kernel driver will then attempt to save into
the same location at the "same" time.  Which one wins?  Who cares?
Userspace did something foolish and it doesn't matter, and writes are
pretty much "atomic" in that they do not split across memory locations
so it's not a real issue.

Same here with your "speed" value, it just doesn't matter, right?  One
will "win" and the other one will not, so what is the problem?  Same
thing would happen if you put a lock here, but a lock would be
pointless.

So yes, I agree, mark things in rust as "mut" if you are going to change
them, that's good, but attempting to prevent multiple writes at the same
time without a lock, that's not going to matter, if it did, you would
have used a lock :)

thanks,

greg k-h

Patch

diff --git a/init/Kconfig b/init/Kconfig
index 6d35728b94b2..0fc6f5568748 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1903,6 +1903,14 @@  config RUST
 
 	  If unsure, say N.
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "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 RUSTC_VERSION_TEXT
 	string
 	depends on RUST
diff --git a/rust/Makefile b/rust/Makefile
index 87958e864be0..f67e55945b36 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -331,6 +331,7 @@  quiet_cmd_bindgen = BINDGEN $@
       cmd_bindgen = \
 	$(BINDGEN) $< $(bindgen_target_flags) \
 		--use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \
+		--rustified-enum phy_state\
 		--no-debug '.*' \
 		-o $@ -- $(bindgen_c_flags_final) -DMODULE \
 		$(bindgen_target_cflags) $(bindgen_target_extra)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c91a3c24f607..ec4ee09a34ad 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 e8811700239a..0588422e273c 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(receiver_trait)]
@@ -36,6 +37,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..7b2a3a52ff25
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,679 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
+
+use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
+use core::marker::PhantomData;
+
+/// Corresponds to the kernel's `enum phy_state`.
+#[derive(PartialEq)]
+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,
+}
+
+/// Represents duplex mode.
+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`.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// This function can be called only in the callbacks in `phy_driver`. PHYLIB guarantees
+    /// the exclusive access for the duration of the lifetime `'a`.
+    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+        // `Device` type being transparent makes the cast ok.
+        unsafe { &mut *ptr.cast() }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn phy_id(&self) -> u32 {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).phy_id }
+    }
+
+    /// Gets the state of the PHY.
+    pub fn state(&self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let state = unsafe { (*phydev).state };
+        // FIXME: Here we trust that PHYLIB doesn't set a random value to this `state` enum.
+        // If PHYLIB has a bug to do so, it results in UB. We will use the safe code
+        // automatically generated from C enum once it becomes possible.
+        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,
+        }
+    }
+
+    /// Returns true if the link is up.
+    pub fn get_link(&self) -> bool {
+        const LINK_IS_UP: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.link() == LINK_IS_UP
+    }
+
+    /// Returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&self) -> bool {
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.autoneg() == bindings::AUTONEG_ENABLE
+    }
+
+    /// Returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&self) -> bool {
+        const AUTONEG_COMPLETED: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.autoneg_complete() == AUTONEG_COMPLETED
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&self, speed: u32) {
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let mut phydev = unsafe { *self.0.get() };
+        phydev.speed = speed as i32;
+    }
+
+    /// Sets duplex mode.
+    pub fn set_duplex(&self, mode: DuplexMode) {
+        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: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let mut phydev = unsafe { *self.0.get() };
+        phydev.duplex = v;
+    }
+
+    /// Reads a given C22 PHY register.
+    pub fn read(&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(&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(&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(&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(&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(&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(&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(&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(&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(&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(&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(&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(&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).
+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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+        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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+            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: Preconditions ensure `phydev` is valid.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+}
+
+/// An instance of a PHY driver.
+///
+/// Wraps the kernel's `struct phy_driver`.
+#[repr(transparent)]
+pub struct DriverType(Opaque<bindings::phy_driver>);
+
+/// Creates the kernel's `phy_driver` instance.
+///
+/// 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>() -> DriverType {
+    DriverType(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() }
+    }))
+}
+
+/// Corresponds to functions in `struct phy_driver`.
+///
+/// This is used to register a PHY driver.
+#[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.
+    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
+
+    /// Issues a PHY software reset.
+    fn soft_reset(_dev: &Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Probes the hardware to determine what abilities it has.
+    fn get_features(_dev: &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: &Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Determines the negotiated speed and duplex.
+    fn read_status(_dev: &Device) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Suspends the hardware, saving state if needed.
+    fn suspend(_dev: &Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Resumes the hardware, restoring state if needed.
+    fn resume(_dev: &Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD read function for reading a MMD register.
+    fn read_mmd(_dev: &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: &Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Callback for notification of link change.
+    fn link_change_notify(_dev: &Device) {}
+}
+
+/// Registration structure for a PHY driver.
+///
+/// # Invariants
+///
+/// All elements of the `drivers` slice are valid and currently registered
+/// to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: &'static [DriverType],
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    ///
+    /// # Safety
+    ///
+    /// The values of the `drivers` array must be initialized properly.
+    pub unsafe fn register(
+        module: &'static crate::ThisModule,
+        drivers: &'static [DriverType],
+    ) -> Result<Self> {
+        if drivers.is_empty() {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The safety requirements of the function ensure that `drivers` array are initialized properly.
+        // 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 safety requirements of the function and the success of `phy_drivers_register` ensure
+        // the invariants.
+        Ok(Registration { drivers })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `self.drivers` is valid.
+        unsafe {
+            bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32)
+        };
+    }
+}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Send for Registration {}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Sync for Registration {}
+
+/// Represents the kernel's `struct mdio_device_id`.
+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 as_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,
+        }
+    }
+}