linux-kernel - Re: [net-next PATCH v10 7/7] rust: net::phy sync with match_phy

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: <20250517.221313.1252217275580085717.fujita.tomonori@gmail.com>
Date: Sat, 17 May 2025 22:13:13 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: lossin@...nel.org
Cc: fujita.tomonori@...il.com, ansuelsmth@...il.com, andrew+netdev@...n.ch,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, hkallweit1@...il.com, linux@...linux.org.uk,
 florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
 kabel@...nel.org, andrei.botila@....nxp.com, tmgross@...ch.edu,
 ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...nel.org, aliceryhl@...gle.com, dakr@...nel.org,
 sd@...asysnail.net, michael@...sekall.de, daniel@...rotopia.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [net-next PATCH v10 7/7] rust: net::phy sync with
 match_phy_device C changes

On Sat, 17 May 2025 10:06:13 +0200
"Benno Lossin" <lossin@...nel.org> wrote:

>>> I looked at the `phy.rs` file again and now I'm pretty sure the above
>>> code is wrong. `Self` can be implemented on any type (even types like
>>> `Infallible` that do not have any valid bit patterns, since it's an
>>> empty enum). The abstraction for `bindings::phy_driver` is
>>> `DriverVTable` not an object of type `Self`, so you should cast to that
>>> pointer instead.
>>
>> Yeah.
>>
>> I don't want to delay this patchset due to Rust side changes so
>> casting a pointer to bindings::phy_driver to DriverVTable is ok but
>> the following signature doesn't look useful for Rust phy drivers:
>>
>> fn match_phy_device(_dev: &mut Device, _drv: &DriverVTable) -> bool
>>
>> struct DriverVTable is only used to create an array of
>> bindings::phy_driver for C side, and it doesn't provide any
>> information to the Rust driver.
> 
> Yeah, but we could add accessor functions that provide that information.

Yes. I thought that implementation was one of the options as well but
realized it makes sense because inside match_phy_device() callback, a
driver might call a helper function that takes a pointer to
bindings::phy_driver (please see below for details).


> Although that doesn't really make sense at the moment, see below.
>
>> In match_phy_device(), for example, a device driver accesses to
>> PHY_DEVICE_ID, which the Driver trait provides. I think we need to
>> create an instance of the device driver's own type that implements the
>> Driver trait and make it accessible.
> 
> I think that's wrong, nothing stops me from implementing `Driver` for an
> empty enum and that can't be instantiated. The reason that one wants to
> have this in C is because the same `match` function is used for
> different drivers (or maybe devices? I'm not too familiar with the
> terminology). In Rust, you must implement the match function for a
> single PHY_DEVICE_ID only, so maybe we don't need to change the
> signature at all?

I'm not sure I understand the last sentence. The Rust PHY abstraction
allows one module to support multiple drivers. So we can could the
similar trick that the second patch in this patchset does.

fn match_device_id(dev: &mut phy::Device, drv: &phy::DriverVTable) -> bool {
    // do comparison workking for three drivers
}

#[vtable]
impl Driver for PhyA {
    ...
    fn match_phy_device(dev: &mut phy::Device, drv: &phy::DriverVTable) -> bool {
        match_device_id(dev, drv)
    }
}

#[vtable]
impl Driver for PhyB {
    ...
    fn match_phy_device(dev: &mut phy::Device, drv: &phy::DriverVTable) -> bool {
        match_device_id(dev, drv)
    }
}

#[vtable]
impl Driver for PhyC {
    ...
    fn match_phy_device(dev: &mut phy::Device, drv: &phy::DriverVTable) -> bool {
        match_device_id(dev, drv)
    }
}


The other use case, as mentioned above, is when using the generic helper
function inside match_phy_device() callback. For example, the 4th
patch in this patchset adds genphy_match_phy_device():

int genphy_match_phy_device(struct phy_device *phydev,
                           const struct phy_driver *phydrv)

We could add a wrapper for this function as phy::Device's method like

impl Device {
    ...
    pub fn genphy_match_phy_device(&self, drv: &phy::DriverVTable) -> i32 

Then a driver could do something like 5th patch in the patchset:

#[vtable]
impl Driver for PhyD {
    ...
    fn match_phy_device(dev: &mut phy::Device, drv: &phy::DriverVTable) -> bool {
       let val = dev.genphy_match_phy_device(drv);
       ...
    }
}