Message-Id: <D9Z3MWPWLMNF.2JS9XJ9U8C4H1@kernel.org>
Date: Sun, 18 May 2025 09:13:52 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Christian Marangi" <ansuelsmth@...il.com>
Cc: "FUJITA Tomonori" <fujita.tomonori@...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 May 17, 2025 at 10:09 PM CEST, Christian Marangi wrote:
> On Sat, May 17, 2025 at 09:02:51PM +0200, Benno Lossin wrote:
>> On Sat May 17, 2025 at 3:13 PM CEST, FUJITA Tomonori wrote:
>> > 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
>> > }
>> 
>> I wouldn't do it like this in Rust, instead this would be a "rustier"
>> function signature:
>> 
>>     fn match_device_id<T: Driver>(dev: &mut phy::Device) -> bool {
>>         // do the comparison with T::PHY_DEVICE_ID
>>         dev.id() == T::PHY_DEVICE_ID
>>     }
>> 
>> And then in the impls for Phy{A,B,C,D} do this:
>> 
>>     impl Driver for PhyA {
>>         fn match_phy_device(dev: &mut phy::Device) -> bool {
>>             match_device_id::<Self>(dev)
>>         }
>>     }
>> 
>
> My 2 cent about the discussion and I'm totally detached from how it
> works on Rust kernel code but shouldn't we try to keep parallel API and
> args between C and Rust?

It depends :) There are several things to consider in this from safety
to ease of use. Sometimes we diverge from the C implementation, because
it makes it safe. For example, from the very beginning, mutexes in Rust
have used guards, but those have only been recently-ish introduced in
C. Mutexes in Rust also store the value inside of them, making it
impossible to access the inner value without taking the lock. This makes
the API safer, but diverges from C.

Now in this case, we're talking about making an API more rusty and thus
diverging from the C side. One can argue for either side and we have to
strike a balance, but in the end it'll probably be up to each subsystem
how they will handle it. It's not as big of an argument as safety (which
we strive very hard for).

In this concrete case, a `DriverVTable` is just an adapter type that we
use to make a trait in Rust be compatible with a C vtable. From Rusts
perspective, the single origin of truth is the trait and the vtable is
derived from that. So from a Rust perspective it's much more natural to
use the trait than the vtable. (This might also end up being more
performant, since we statically know which ID the driver supports and
the C side needs to dynamically look it up.)

> I know maybe some thing doesn't make sense from C to Rust but doesn't
> deviates from C code introduce more confusion when something need to be
> ported from C to Rust?

Ideally we have people that work on both sides and can bridge that gap.
If not, then having a Rust expert and a C expert work together has
worked out great in the past. I don't see this becoming a big problem
unless one of the sides is poorly maintained, which could also happen in
C.

> Again no idea if this apply so I'm just curious about this.

No worries, glad to hear your perspective.

---
Cheers,
Benno

Powered by blists - more mailing lists