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

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

On Sun, 24 Sep 2023 13:19:21 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> +/// Wraps the kernel's `struct phy_device`.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
> 
> This struct should have a type invariant saying that "`self.0` is
> always in a valid state.".

Added.


>> +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) }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn id(&mut self) -> u32 {
>> +        let phydev = Opaque::get(&self.0);
>> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> 
> These kinds of safety comments should be replaced by "`phydev` is
> pointing to a valid object by the type invariant of `Self`."
> Also for the other safety comments below.

Fixed all these comments. 


>> +    /// Reads a given C22 PHY register.
>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> 
> I have no knowledge of the driver and the device, but is
> it really fine to supply any 16 bit integer value here?

No, I assume that the C side returns an error in such case.


>> +        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)
>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
> 
> Not sure if this is nicer, but you could use
> `to_result(ret).map(|()| ret as u16)` instead.

I'm not sure either. I have no preference though.

>> +
>> +    /// Writes a given C22 PHY register.
>> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
>> +        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_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum as u32, val)
>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(())
>> +        }
> 
> Use `error::to_result` instead.

Fixed.

>> +    }
>> +
>> +    /// Reads a paged register.
>> +    pub fn read_paged(&mut self, page: u16, 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::phy_read_paged(phydev, page as i32, regnum as u32) };
>> +        if ret < 0 {
>> +            return Err(Error::from_errno(ret));
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
>> +    }
>> +
>> +    /// Resolves the advertisements into PHY settings.
>> +    pub fn resolve_aneg_linkmode(&mut self) {
>> +        let phydev = 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);
>> +        }
>> +    }
>> +
>> +    /// Executes software reset the PHY via BMCR_RESET bit.
>> +    pub fn genphy_soft_reset(&mut self) -> Result {
>> +        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::genphy_soft_reset(phydev) };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(())
>> +        }
> 
> Use `error::to_result` instead.

Fixed.

>> +    }
>> +
>> +    /// Initializes the PHY.
>> +    pub fn init_hw(&mut self) -> Result {
>> +        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::phy_init_hw(phydev) };
>> +        if ret != 0 {
>> +            return Err(Error::from_errno(ret));
>> +        } else {
>> +            Ok(())
>> +        }
> 
> Use `error::to_result` instead. And below...

Fixed all.

>> +/// An adapter for the registration of a PHY driver.
>> +pub struct Adapter<T: Driver> {
>> +    _p: PhantomData<T>,
>> +}
> 
> I think this type can be private.
> 
>> +
>> +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 {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::soft_reset(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn get_features_callback(
>> +        phydev: *mut bindings::phy_device,
>> +    ) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::get_features(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::suspend(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::resume(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn config_aneg_callback(
>> +        phydev: *mut bindings::phy_device,
>> +    ) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::config_aneg(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn read_status_callback(
>> +        phydev: *mut bindings::phy_device,
>> +    ) -> core::ffi::c_int {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::read_status(dev)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn match_phy_device_callback(
>> +        phydev: *mut bindings::phy_device,
>> +    ) -> core::ffi::c_int {
>> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +        let dev = unsafe { Device::from_raw(phydev) };
>> +        T::match_phy_device(dev) as i32
>> +    }
>> +
>> +    unsafe extern "C" fn read_mmd_callback(
>> +        phydev: *mut bindings::phy_device,
>> +        devnum: i32,
>> +        regnum: u16,
>> +    ) -> i32 {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
>> +            Ok(ret.into())
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn write_mmd_callback(
>> +        phydev: *mut bindings::phy_device,
>> +        devnum: i32,
>> +        regnum: u16,
>> +        val: u16,
>> +    ) -> i32 {
>> +        from_result(|| {
>> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +            let dev = unsafe { Device::from_raw(phydev) };
>> +            T::write_mmd(dev, devnum as u8, regnum, val)?;
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
>> +        // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> +        let dev = unsafe { Device::from_raw(phydev) };
>> +        T::link_change_notify(dev);
>> +    }
>> +
>> +    /// 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> {
> 
> In order to make `Adapter` a private type, you have
> to make this function standalone (and generic).
> Or create a standalone function that just calls this.

done, nice.


>> +/// 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",
>> +/// }
>> +/// ```
>> +#[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> {
> 
> Why do you use `$modname` as a generic parameter? Can't
> you just use `$modname` as the name of this struct?

I followed module_driver! macro and similr macros like
module_platform_driver! in rust branch; a device driver programmer can
define a struct for Module.


>> +            _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()),+,
>> +        ];
> 
> Not sure if a static is the best way to do this. You
> might also be able to just store this array inside
> of the `Module<$modname>` struct.

With that approach, we can't make the array static, right? I can't
figure out a way to handle lifetime of Registration and
Module<$modname>; Registration needs a pointer to the array for
its drop().

>> +        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.
> 
> This safety comment should address why you are allowed
> to access this mutable static here.

The Rust sides just passes static `DRIVERS` array to the C side, which
is used only in the C side with appropriate synchronization mechanism.

?