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

[Benno Lossin <benno.lossin@proton.me>]

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.

>> +            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.