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] [day] [month] [year] [list]

Message-Id: <20231201.144402.323448332510672095.fujita.tomonori@gmail.com>
Date: Fri, 01 Dec 2023 14:44:02 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: gary@...yguo.net
Cc: fujita.tomonori@...il.com, 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

Hi,

On Wed, 29 Nov 2023 19:51:45 +0000
Gary Guo <gary@...yguo.net> wrote:

> 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.

IIRC, these names are based on the IEEE spec and clear for PHY developers.

read/write: access a C22 register.
read_paged/write_paged: access a paged register.


>> +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?

Because two Rust PHY drivers that I implemented don't need such.