Message-ID: <6828ece4.050a0220.49f8b.ca57@mx.google.com>
Date: Sat, 17 May 2025 22:09:05 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Benno Lossin <lossin@...nel.org>
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 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?

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?

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

> >
> > #[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 
> 
> Not sure why this returns an `i32`, but we probably could have such a
> function as well (though I wouldn't use the vtable for that).
>

Mhh looking at some comments, maybe the module needs some extra love here
and there.

> 
> > 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);
> >        ...
> >     }
> > }
> 

-- 
	Ansuel

Powered by blists - more mailing lists