Date: Wed, 11 Oct 2023 23:16:07 +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, greg@...ah.com, tmgross@...ch.edu
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network
 PHY drivers

On Mon, 09 Oct 2023 12:19:54 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

I skipped the topics that you've already discussed with Andrew.

> On 09.10.23 03:39, 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_BINDINGS.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> ---
>>   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          | 733 ++++++++++++++++++++++++++++++++
>>   6 files changed, 754 insertions(+)
>>   create mode 100644 rust/kernel/net.rs
>>   create mode 100644 rust/kernel/net/phy.rs

(snip)

>> +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.cast() }
> 
> Missing `SAFETY` comment.

Added:

// SAFETY: The safety requirements guarantee the validity of the dereference, while the
// `Device` type being transparent makes the cast ok.


>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&mut 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(&mut 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: enum-cast
>> +        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(&mut self) -> bool {
> 
> I would call this function `is_link_up`.
> 
>> +        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 }
> 
> Can you move the call to `link` and the `==` operation out
> of the `unsafe` block? They are safe operations. (also do
> that below where possible)

Sure, fixed.


>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
> 
> Missing '`'.

Fixed.


>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
> 
> Since it is a reference you do not need to explicitly state
> that it points to an array of `struct phy_driver`. Instead I would
> suggest the following invariant:
> 
> All elements of the `drivers` slice are valid and currently registered
> to the kernel via `phy_drivers_register`.

Surely, makes sense. 


>> +pub struct Registration {
>> +    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
> 
> Why is this an `Option`?

Oops, removed; leftover of older version.


>> +}
>> +
>> +impl Registration {
>> +    /// Registers a PHY driver.
>> +    #[must_use]
>> +    pub fn register(
>> +        module: &'static crate::ThisModule,
>> +        drivers: &'static [Opaque<bindings::phy_driver>],
>> +    ) -> Result<Self> {
>> +        if drivers.len() == 0 {
>> +            return Err(code::EINVAL);
>> +        }
>> +        // SAFETY: `drivers` has static lifetime and used only in the C side.
>> +        to_result(unsafe {
>> +            bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
>> +        })?;
> 
> This `register` function seems to assume that the values of the
> `drivers` array are initialized and otherwise also considered valid.
> So please change that or make this function `unsafe`.

Understood.


>> +        Ok(Registration {
> 
> Please add an `INVARIANT` comment similar to a `SAFETY` comment
> that explains why the invariant is upheld.

Added.


>> +#[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 [$($dev:expr),+]) => {
>> +        #[no_mangle]
>> +        static __mod_mdio__phydev_device_table: [
> 
> Shouldn't this have a unique name? If we define two different
> phy drivers with this macro we would have a symbol collision?
> 
>> +            kernel::bindings::mdio_device_id;
> 
> Please use absolute paths in macros:
> `::kernel::bindings::mdio_device_id` (also below).

Updated.


>> +            $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),+], $($f:tt)*) => {
>> +        struct Module {
>> +            _reg: kernel::net::phy::Registration,
>> +        }
>> +
>> +        $crate::prelude::module! {
>> +             type: Module,
>> +             $($f)*
>> +        }
>> +
>> +        static mut DRIVERS: [
>> +            kernel::types::Opaque<kernel::bindings::phy_driver>;
>> +            $crate::module_phy_driver!(@count_devices $($driver),+)
>> +        ] = [
>> +            $(kernel::net::phy::create_phy_driver::<$driver>()),+
>> +        ];
>> +
>> +        impl kernel::Module for Module {
>> +            fn init(module: &'static ThisModule) -> Result<Self> {
>> +                // SAFETY: static `DRIVERS` array is used only in the C side.
> 
> In order for this SAFETY comment to be correct, you need to ensure
> that nobody else can access the `DRIVERS` static. You can do that by
> placing both the `static mut DRIVERS` and the `impl ::kernel::Module
> for Module` items inside of a `const _: () = {}`, so like this:
> 
>      const _: () = {
>          static mut DRIVERS: [...] = ...;
>          impl ::kernel::Module for Module { ... }
>      };
> 
> You can also mention this in the SAFETY comment.

Great, that's exactly what to be needed here. Thanks a lot!

Powered by blists - more mailing lists