Re: [RFC PATCH v3 1/3] rust: core abstractions for network PHY drivers

[Trevor Gross <tmgross@umich.edu>]

Linking in some context, discussion is somewhat fragmented across this
patch series. This is an old set anyway, the latest is at
https://lore.kernel.org/rust-for-linux/ZSQXqX2%2Flhf5ICZP@gpd/T/#t -
but I don't think anything reviewed has changed.

On Tue, Oct 10, 2023 at 3:19 PM Wedson Almeida Filho <wedsonaf@gmail.com> wrote:
> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&mut self) -> bool {
> > +        const LINK_IS_UP: u32 = 1;
>
> Where is this coming from?
>
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        unsafe { (*phydev).link() == LINK_IS_UP }
>
> Wouldn't it be more future-proof to do (*phydev).link() != 0? That's
> what the C side does.

I suggested it as more clear hint about the magic value, logic could
potentially be flipped though

https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/

> > +    /// Sets the speed of the PHY.
> > +    pub fn set_speed(&mut self, speed: u32) {
> > +        let phydev = self.0.get();
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        unsafe { (*phydev).speed = speed as i32 };
> > +    }
>
> Should we perhaps saturate `speed` to `i32::MAX` so that it doesn't
> become negative?

https://lore.kernel.org/rust-for-linux/96800001-5d19-4b48-b43e-0cfbeccb48c1@lunn.ch/

I agree that saturating probably isn't a bad idea here, but the C side
does validate it

> > +/// ```ignore
> > +///
> > +/// use kernel::net::phy::{self, DeviceId, Driver};
> > +/// use kernel::prelude::*;
> > +///
> > +/// kernel::module_phy_driver! {
> > +///     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
> > +///     device_table: [
> > +///         DeviceId::new_with_driver::<PhyAX88772A>(),
> > +///         DeviceId::new_with_driver::<PhyAX88772C>(),
> > +///         DeviceId::new_with_driver::<PhyAX88796B>()
> > +///     ],
>
> I don't like the fact that we need to repeat names in `driver` and
> `device_table`. A single list should suffice, no?

I agree, but there needs to be a way to use a driver more than once
with different IDs. See
https://lore.kernel.org/rust-for-linux/CALNs47vyW+A5uwCQ-ug03373xj+QMehx=1pXZ8ViS2jZ8KhfxQ@mail.gmail.com/
and the followup, maybe you have a better suggestion