Message-Id: <20231014.162210.522439670437191285.fujita.tomonori@gmail.com>
Date: Sat, 14 Oct 2023 16:22:10 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, andrew@...n.ch,
 miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com,
 wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v4 1/4] rust: core abstractions for network
 PHY drivers

On Fri, 13 Oct 2023 21:31:16 +0000
Benno Lossin <benno.lossin@...ton.me> 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.

Powered by blists - more mailing lists