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

Message-ID: <e8a440c7-d0a6-4a5e-97ff-a8bcde662583@lunn.ch>
Date: Mon, 15 Apr 2024 16:20:16 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org,
	tmgross@...ch.edu
Subject: Re: [PATCH net-next v1 2/4] rust: net::phy support C45 helpers

On Mon, Apr 15, 2024 at 07:46:59PM +0900, FUJITA Tomonori wrote:
> This patch adds the following helper functions for Clause 45:
> 
> - mdiobus_c45_read
> - mdiobus_c45_write
> - genphy_c45_read_status

We need to be very careful here, and try to avoid an issue we have in
the phylib core, if possible. C45 is a mix of two different things:

* The C45 MDIO bus protocol
* The C45 registers

You can access C45 registers using the C45 bus protocol. You can also
access C45 registers using C45 over C22. So there are two access
mechanisms to the C45 registers.

A PHY driver just wants to access C45 registers. It should not care
about what access mechanism is used, be it C45 bus protocol, or C45
over C22.

> +    /// 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.
> +        let ret = unsafe {
> +            bindings::mdiobus_c45_read(
> +                (*phydev).mdio.bus,
> +                (*phydev).mdio.addr,
> +                devad as i32,
> +                regnum.into(),
> +            )

So you have wrapped the C function mdiobus_c45_read(). This is going
to do a C45 bus protocol read. For this to work, the MDIO bus master
needs to support C45 bus protocol, and the PHY also needs to support
the C45 bus protocol. Not all MDIO bus masters and PHY devices
do. Some will need to use C45 over C22.

A PHY driver should know if a PHY device supports C45 bus protocol,
and if it supports C45 over C22. However, i PHY driver has no idea
what the bus master supports.

In phylib, we have a poorly defined phydev->is_c45. Its current
meaning is: "The device was found using the C45 bus protocol, not C22
protocol". One of the things phylib core then uses is_c45 for it to
direct the C functions phy_read_mmd() and phy_write_mmd() to perform a
C45 bus protocol access if true. If it is false, C45 over C22 is
performed instead. As a result, if a PHY is discovered using C22, C45
register access is then performed using C45 over C22, even thought the
PHY and bus might support C45 bus protocol. is_c45 is also used in
other places, e.g. to trigger auto-negotiation using C45 registers,
not C22 registers.

In summary, the C API is a bit of a mess.

For the Rust API we have two sensible choices:

1) It is the same mess as the C API, so hopefully one day we will fix
   both at the same time.

2) We define a different API which correctly separate C45 bus access
   from C45 registers.

How you currently defined the Rust API is neither of these.

       Andrew