netdev - Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

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: <20240527.110031.1543730602683806299.fujita.tomonori@gmail.com>
Date: Mon, 27 May 2024 11:00:31 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

Hi,
Sorry about the long delay,

On Mon, 15 Apr 2024 23:25:22 -0400
Trevor Gross <tmgross@...ch.edu> wrote:

> On Mon, Apr 15, 2024 at 6:47 AM FUJITA Tomonori
> <fujita.tomonori@...il.com> wrote:
>> +    /// Reads a given C45 PHY register.
>> +    /// This function reads a hardware register and updates the stats so takes `&mut self`.
>> +    pub fn c45_read(&mut self, devad: u8, regnum: u16) -> Result<u16> {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        // So it's just an FFI call.
> 
> Depending on the response to Andrew's notes, these SAFETY comments
> will probably need to be updated to say why we know C45 is supported.

If a driver uses only-c45 API (RegC45Direct in the new patch), it returns
an error. We need to write something in SAFETY?


>> +        let ret = unsafe {
>> +            bindings::mdiobus_c45_read(
>> +                (*phydev).mdio.bus,
>> +                (*phydev).mdio.addr,
>> +                devad as i32,
> 
> This could probably also be from/.into()

Fixed.

>> +                regnum.into(),
>> +            )
>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
> 
> Could this be simplified with to_result?

to_result returns Ok(()). So I think that we need something
to_result_with_value().

I think that we discussed something like that ago:

https://lore.kernel.org/all/CANiq72=VaNtZ_B0ji94xFftb4Pctfep+4c2cNWkmCcKhpZY2kQ@mail.gmail.com/

A new function was introduced, which I missed?


>> +    }
>> +
>> +    /// Writes a given C45 PHY register.
>> +    pub fn c45_write(&mut self, devad: u8, 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 it's just an FFI call.
>> +        to_result(unsafe {
>> +            bindings::mdiobus_c45_write(
>> +                (*phydev).mdio.bus,
>> +                (*phydev).mdio.addr,
>> +                devad as i32,
> 
> Same as above

Fixed.