netdev - Re: [PATCH net-next v8 1/4] rust: core abstractions for network PHY drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 Nov 2023 19:51:45 +0000
From: Gary Guo <gary@...yguo.net>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
 tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me,
 wedsonaf@...il.com, aliceryhl@...gle.com, boqun.feng@...il.com
Subject: Re: [PATCH net-next v8 1/4] rust: core abstractions for network PHY
 drivers

On Thu, 23 Nov 2023 14:04:09 +0900
FUJITA Tomonori <fujita.tomonori@...il.com> wrote:

> +    /// Reads a given C22 PHY register.
> +    // This function reads a hardware register and updates the stats so takes `&mut self`.
> +    pub fn read(&mut 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(&mut 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)
> +        })
> +    }

`read` and `write` are not very distinctive names, especially when
`read_paged` exists.

Maybe `mdio_{read,write}`?

> +/// Defines certain other features this PHY supports (like interrupts).
> +///
> +/// These flag values are used in [`Driver::FLAGS`].
> +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: This callback is called only in contexts
> +            // where we hold `phy_device->lock`, so the accessors on
> +            // `Device` are okay to call.
> +            let dev = unsafe { Device::from_raw(phydev) };
> +            T::soft_reset(dev)?;

Usually we want type safety by to the callback typed access to the
device's driver-private data, rather than just give it an arbitrary
`Device`. Any reason not to similar things here?

> +            Ok(0)
> +        })
> +    }
> +}
> +