Re: [RFC PATCH v2 1/3] rust: core abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Tue, 26 Sep 2023 02:05:41 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Sun, Sep 24, 2023 at 2:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> +    /// may read or write to the `phy_device` object.
>> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        unsafe { &mut *(ptr as *mut Self) }
>> +    }
>> +
> 
> Prefer .cast() over `as *` because it makes it makes it harder to
> accidentally change mut/const

fixed.


>> +    /// Returns true if the PHY has no link.
>> +    pub fn get_link(&mut self) -> bool {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> +        unsafe { (*phydev).link() != 0 }
>> +    }
>> +
> 
> Maybe put a `const LINK_IS_UP: c_uint = 0` in that function to make
> the magic number a bit more clear.
> 
> Also, "true if phy has NO link" seems opposite to me - wouldn't it
> make more sense to return true if there is a link? This is opposite of
> C's internal representation but I think that's OK to avoid the double
> negative. Or even map it to an enum?

Oops, the comment is wrong. I updated the function like the following:

/// Returns true if the link is up.
pub fn get_link(&mut self) -> bool {
    const LINK_IS_UP: u32 = 1;
    let phydev = self.0.get();
    // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
    unsafe { (*phydev).link() == LINK_IS_UP }
}

>> +
>> +    /// Returns true if auto-negotiation is completed.
>> +    pub fn is_autoneg_completed(&mut self) -> bool {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
>> +        unsafe { (*phydev).autoneg_complete() != 0 }
>> +    }
>> +
> 
> Probably the same thing as above with a const

Fixed.

>> +    /// Reads a given C22 PHY register.
>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor,
>> +        // so an FFI call with a valid pointer.
>> +        let ret = unsafe {
>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32)
>> +        };
> 
> Prefer `from`/`into` instead of `as` for these lossless casts, it just
> gives an sanity check when refactoring. A few other functions that
> take `regnum` or `page` could use the same change.
> 
> I wonder if the there is any benefit to changing the u32 -> u16 casts
> into checked conversions and at least printing
> 
>> +    /// Resolves the advertisements into PHY settings.
>> +    pub fn resolve_aneg_linkmode(&mut self) {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor,
>> +        // so an FFI call with a valid pointer.
>> +        unsafe {
>> +            bindings::phy_resolve_aneg_linkmode(phydev);
>> +        }
>> +    }
> 
> Semicolon outside the { } for one liner unsafes so it formats more compact

Fixed this and the similar code.


>> +    /// Updates the link status.
>> +    pub fn genphy_update_link(&mut self) -> Result // ...
>> +
>> +    /// Reads Link partner ability.
>> +    pub fn genphy_read_lpa(&mut self) -> Result // ...
>> +
>> +    /// Reads PHY abilities.
>> +    pub fn genphy_read_abilities(&mut self) -> Result // ...
> 
> Do these functions need to be public? It seems like they are just used
> to update internal state, which is wrapped up into more user-facing
> API

As Andrew wrote, all genphy_ functions needs to be exported.


>> +/// An adapter for the registration of a PHY driver.
>> +pub struct Adapter<T: Driver> {
>> +    _p: PhantomData<T>,
>> +}
>> +
>> +impl<T: Driver> Adapter<T> {
>> +    /// Creates a new `Adapter` instance.
>> +    pub const fn new() -> Adapter<T> {
>> +        Self { _p: PhantomData }
>> +    }
>> +
>> +    unsafe extern "C" fn soft_reset_callback(
>> +        phydev: *mut bindings::phy_device,
>> +    ) -> core::ffi::c_int {
>> + [...]
> 
> What is the motivation for having an adapter as a type? Since it's
> just a dummy struct that has no runtime meaning, I think the code
> would be easier to follow if these functions were put in a module
> 
>     mod phy_registration_helpers {
>         unsafe extern "C" fn soft_reset_callback<T: Driver>(
>             phydev: *mut bindings::phy_device,
>         ) -> core::ffi::c_int { /* ... */ }
>         // ...
>     }

I tried to do in the similar way as rust branch.

As Benno suggested, I made Adapter struct private and
create_phy_driver() a function instead of method. Hopefully, the
changes makes the code clear.


>> +    /// Creates the kernel's `phy_driver` instance.
>> +    ///
>> +    /// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> +    pub const fn create_phy_driver() -> Opaque<bindings::phy_driver> {
> 
> This construction looks much better than v1 :)

:)

>> +/// Corresponds to functions in `struct phy_driver`.
>> +#[vtable]
>> +pub trait Driver {
>> +    /// Corresponds to `flags` in `struct phy_driver`.
>> +    const FLAGS: u32 = 0;
>> +    /// Corresponds to `name` in `struct phy_driver`.
>> +    const NAME: &'static CStr;
>> +    /// Corresponds to `phy_id` in `struct phy_driver`.
>> +    const PHY_ID: u32 = 0;
>> +    /// Corresponds to `phy_id_mask` in `struct phy_driver`.
>> +    const PHY_ID_MASK: u32 = 0;
> 
> This also seems better to me, thanks for updating. Could this just
> take a `DeviceMask`?

Surely, I think that we can use DeviceId here. Then a PHY driver code
would be more readable:

impl Driver for PhyAX88772C {
    const FLAGS: u32 = phy::flags::IS_INTERNAL;
    const NAME: &'static CStr = c_str!("Asix Electronics AX88772C");
    const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1881);


>> +/// Registration structure for a PHY driver.
>> +pub struct Registration {
>> +    module: &'static crate::ThisModule,
>> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
>> +}
> 
> This new structure without the generics looks much better, with one note below
> 
>> +impl Registration {
>> +    /// Creates a new `Registration` instance.
>> +    pub fn new(module: &'static crate::ThisModule) -> Self {
>> +        Registration {
>> +            module,
>> +            drivers: None,
>> +        }
>> +    }
>> +
>> +    /// Registers a PHY driver.
>> +    pub fn register(&mut self, drivers: &'static [Opaque<bindings::phy_driver>]) -> Result {
>> +        if drivers.len() > 0 {
>> +            // SAFETY: Just an FFI call.
>> +            let ret = unsafe {
>> +                bindings::phy_drivers_register(
>> +                    drivers[0].get(),
>> +                    drivers.len() as i32,
>> +                    self.module.0,
>> +                )
>> +            };
>> +            if ret != 0 {
>> +                return Err(Error::from_errno(ret));
>> +            }
>> +            self.drivers = Some(drivers);
>> +            Ok(())
>> +        } else {
>> +            Err(code::EINVAL)
>> +        }
>> +    }
>> +}
> 
> I don't think there's any need to have `new` and `register` broken
> out. This is actually a _bad_ pattern because the deregistration
> function gets called (via drop) even if it is never registered.
> 
> I believe that `phy_drivers_unregister` should check against this, but
> all the same I would strongly suggest:
> 
>     #[must_use]
>     pub fn register(
>         module: &'static crate::ThisModule,
>         drivers: &'static [Opaque<bindings::phy_driver>]
>     ) -> Result<Self>;
> 
> And no other function.

Sounds good, fixed.

> Note the `must_use` to make sure the handle is held rather than
> instantly dropped.

Nice.

>> +impl Drop for Registration {
>> +    fn drop(&mut self) {
>> +        if let Some(drv) = self.drivers.take() {
>> +            // SAFETY: Just an FFI call.
>> +            unsafe {
>> +                bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32);
>> +            }
>> +        }
>> +    }
>> +}
>> +
> 
> 
>> +const DEVICE_MASK_EXACT: u32 = !0;
>> +const DEVICE_MASK_MODEL: u32 = !0 << 4;
>> +const DEVICE_MASK_VENDOR: u32 = !0 << 10;
> 
> See my comment above about maybe having this be an enum

With changes to Driver trait, these consts are only inside
DeviceMask. I'm not sure it worths to make them enum. Please take a
look at the next version.


>> +enum DeviceMask {
>> +    Exact,
>> +    Model,
>> +    Vendor,
>> +    Custom(u32),
>> +}
> 
> LGTM
> 
>> +/// Declares a kernel module for PHYs drivers.
>> +///
>> +/// This creates a static array of `struct phy_driver] and registers it.
>> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
>> +/// for module loading into the module binary file.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +///     device_table: [
>> +///         DeviceId::new_with_driver::<PhyAX88772A>(),
>> +///         DeviceId::new_with_driver::<PhyAX88772C>(),
>> +///         DeviceId::new_with_driver::<PhyAX88796B>()
>> +///     ],
>> +///     type: RustAsixPhy,
>> +///     name: "rust_asix_phy",
>> +///     author: "Rust for Linux Contributors",
>> +///     description: "Rust Asix PHYs driver",
>> +///     license: "GPL",
>> +/// }
>> +/// ```
> 
> I'm glad this worked out, a single macro is nice.
> 
> Since almost all devices will just use their standard DeviceId, could
> that be done by default unless a specific ID is specified? Something
> like
> 
>     drivers: [
>         PhyAX88772A,
>         // register the same phy with a second ID
>         { phy: PhyAX88772A, id: some_other_device_id },
>         PhyAX88772C,
>         PhyAX88796B
>     ],
>     type: RustAsixPhy,
>     // ...
> 
> Though there may be a nicer looking syntax

I thought about something like that but macro would be
complicated and I can't came up with a nice syntax so I kinda prefer
to keep phy_driver and mdio_device_id loose coupling.


>> +#[macro_export]
>> +macro_rules! module_phy_driver {
>> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
>> +
>> +    (@count_devices $($x:expr),*) => {
>> +        0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> +    };
>> +
>> +    (@device_table $name:ident, [$($dev:expr),+]) => {
>> +        ::kernel::macros::paste! {
>> +            #[no_mangle]
>> +            static [<__mod_mdio__ $name _device_table>]: [kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1] =
>> +             [ $(kernel::bindings::mdio_device_id{phy_id: $dev.id, phy_id_mask: $dev.mask_as_int()}),+, kernel::bindings::mdio_device_id {phy_id: 0, phy_id_mask: 0}];
>> +        }
>> +    };
>> +
>> +    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => {
>> +        struct Module<$modname> {
>> +            _reg: kernel::net::phy::Registration,
>> +            _p: core::marker::PhantomData<$modname>,
>> +        }
>> +
>> +        type ModuleType = Module<$modname>;
>> +
>> +        $crate::prelude::module! {
>> +             type: ModuleType,
>> +             $($f)*
>> +        }
>> +
>> +        static mut DRIVERS: [kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+)] = [
>> +            $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+,
>> +        ];
>> +
>> +        impl kernel::Module for Module<$modname> {
>> +            fn init(module: &'static ThisModule) -> Result<Self> {
>> +                let mut reg: kernel::net::phy::Registration = kernel::net::phy::Registration::new(module);
>> +                // SAFETY: static `DRIVERS` array is used only in the C side.
>> +                unsafe {
>> +                    reg.register(&DRIVERS)?;
>> +                }
>> +
>> +                Ok(Module {
>> +                    _reg: reg,
>> +                    _p: core::marker::PhantomData,
>> +                })
>> +            }
>> +        }
>> +
>> +        $crate::module_phy_driver!(@device_table $modname, [$($dev),+]);
>> +    }
>> +}
> 
> These have to be hand formatted until rustfmt is able to handle macro
> bodies, just try to make it look a bit more like code
> 
>     (@device_table $name:ident, [$($dev:expr),+]) => {
>         ::kernel::macros::paste! {
>             #[no_mangle]
>             static [<__mod_mdio__ $name _device_table>]: [
>                 kernel::bindings::mdio_device_id;
>                 $crate::module_phy_driver!(@count_devices $($dev),+) + 1
>             ] = [
>                 $(kernel::bindings::mdio_device_id {
>                     phy_id: $dev.id,
>                     phy_id_mask: $dev.mask_as_int()
>                 }),+

I fixed the format but got the following error without ',' here.

error: expected one of `,`, `.`, `?`, `]`, or an operator, found
`kernel`
  --> drivers/net/phy/ax88796b_rust.rs:9:1
     |
     9  | / kernel::module_phy_driver! {
     10 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
     11 | |     device_table: [
     12 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
     ...  |
     20 | |     license: "GPL",
     21 | | }
        | | ^
	| | |
	| | expected one of `,`, `.`, `?`, `]`, or an operator
	| |_unexpected token
	|   help: missing `,`


>                 kernel::bindings::mdio_device_id {
>                     phy_id: 0,
>                     phy_id_mask: 0
>                 }
>             ];
>         }
>     };
> 
> and
> 
>         static mut DRIVERS: [
>             kernel::types::Opaque<kernel::bindings::phy_driver>;
>             $crate::module_phy_driver!(@count_devices $($driver),+)
>         ] = [
>             $(kernel::net::phy::Adapter::<$driver>::create_phy_driver()),+
>         ];
> 
> Note also that `$( ... ),+,` expands the same as `$( ... ),+` so the
> second is a bit cleaner without the redundant `,` (at least in my
> quick test this worked)


>> --
>> 2.34.1
>>
> 
> Thanks for all the updates. Overall the usability is going in the
> right direction since v1

Thanks a lot for reviewing!