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

[Wedson Almeida Filho <wedsonaf@gmail.com>]

On Sun, 24 Sept 2023 at 10:40, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 24.09.23 14:56, Wedson Almeida Filho wrote:
> > On Sun, 24 Sept 2023 at 03:49, FUJITA Tomonori
> >> +        let phydev = Opaque::get(&self.0);
> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> +        unsafe {
> >> +            (*phydev).speed = speed;
> >> +        }
> >> +    }
> >> +
> >> +    /// Sets duplex mode.
> >> +    pub fn set_duplex(&mut self, mode: DuplexMode) {
> >> +        let phydev = Opaque::get(&self.0);
> >> +        // SAFETY: This object is initialized by the `from_raw` constructor, so it's valid.
> >> +        unsafe {
> >> +            match mode {
> >> +                DuplexMode::Full => (*phydev).duplex = bindings::DUPLEX_FULL as i32,
> >> +                DuplexMode::Half => (*phydev).duplex = bindings::DUPLEX_HALF as i32,
> >> +                DuplexMode::Unknown => (*phydev).duplex = bindings::DUPLEX_UNKNOWN as i32,
> >> +            }
> >> +        }
> >
> > To avoid repeating `(*phydev).duplex =` and to reduce the amount of
> > code inside the unsafe block, I suggest you do something like this
> > here:
> >
> > let v = match mode {
> >      DuplexMode::Full => bindings::DUPLEX_FULL,
> >      DuplexMode::Half => bindings::DUPLEX_HALF,
> >      DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN,
> > };
> >
> > // SAFETY: This object is initialized by the `from_raw` constructor,
> > so it's valid.
> > unsafe { (*phydev).duplex = v};
> >
> >
> > In fact, for this enum, you wan to use #[repr(i32)] and assign values
> > from `bindings` so that you can avoid the match as convert using just
> > `mode as i32`. See for example:
> >
> > https://github.com/wedsonaf/linux/blob/rtarfs/rust/kernel/fs.rs#L71
> >
>
> I do not like `as`-style casts, they are error prone and no warning/error
> is generated if they are used incorrectly. They also make review harder,
> because I now have to look if the enum indeed has that correct repr.
> I think we should talk about this in one of our meetings.

Yes, let's discuss this with the rest of the team in a meeting.

I think the main advantage is that we avoid the big match statement
and additional code that it entails. Perhaps we could have a `TryFrom`
implementation that uses `as` and that's what callers use. Then we
only have to review/verify the `as` usage once when the enum is
defined.

Another option is to keep the big match statement if we can show that
the compiler can optimise it out.

>
> >> +            phy_id: <T>::PHY_ID,
> >> +            phy_id_mask: <T>::PHY_ID_MASK,
> >> +            soft_reset: if <T>::HAS_SOFT_RESET {
> >> +                Some(Self::soft_reset_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            get_features: if <T>::HAS_GET_FEATURES {
> >> +                Some(Self::get_features_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            match_phy_device: if <T>::HAS_MATCH_PHY_DEVICE {
> >> +                Some(Self::match_phy_device_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            suspend: if <T>::HAS_SUSPEND {
> >> +                Some(Self::suspend_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            resume: if <T>::HAS_RESUME {
> >> +                Some(Self::resume_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            config_aneg: if <T>::HAS_CONFIG_ANEG {
> >> +                Some(Self::config_aneg_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            read_status: if <T>::HAS_READ_STATUS {
> >> +                Some(Self::read_status_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            read_mmd: if <T>::HAS_READ_MMD {
> >> +                Some(Self::read_mmd_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            write_mmd: if <T>::HAS_WRITE_MMD {
> >> +                Some(Self::write_mmd_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            link_change_notify: if <T>::HAS_LINK_CHANGE_NOTIFY {
> >> +                Some(Self::link_change_notify_callback)
> >> +            } else {
> >> +                None
> >> +            },
> >> +            // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
> >> +            // sets `Option<&F>` to be `None`.
> >> +            ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
> >
> > I think you could just use bindings::phy_driver::default() here.
>
> That is not possible, since it is not a const function.

Ah, yeah, this is in const context. Nevermind.