Date: Sat, 14 Oct 2023 08:07:03 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY drivers

On 14.10.23 09:22, FUJITA Tomonori wrote:
> On Fri, 13 Oct 2023 21:31:16 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>
>> In some other thread you mentioned that no lock is held for
>> `resume`/`suspend`, how does this interact with it?
> 
> The same quesiton, 4th time?

Yes, it is not clear to me from the code/safety comment alone why
this is safe. Please improve the comment such that that is the case.

> PHYLIB is implemented in a way that PHY drivers exlusively access to
> phy_device during the callbacks.

As I suggested in a previous thread, it would be extremely helpful
if you add a comment on the `phy` abstractions module that explains
how `PHYLIB` is implemented. Explain that it takes care of locking
and other safety related things.

>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> +        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
>>> +        // `Device` type being transparent makes the cast ok.
>>> +        unsafe { &mut *ptr.cast() }
>>
>> please refactor to
>>
>>       // CAST: ...
>>       let ptr = ptr.cast::<Self>();
>>       // SAFETY: ...
>>       unsafe { &mut *ptr }
> 
> I can but please tell the exactly comments for after CAST and SAFETY.
> 
> I can't find the description of CAST comment in
> Documentation/rust/coding-guidelines.rst. So please add why and how to
> avoid repeating the same review comment in the future.

I haven't had the time to finish my work on the standardization of
`SAFETY` (and also `CAST`) comments, but I am working on that.

        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
        let ptr = ptr.cast::<Self>();
        // SAFETY: by the function requirements the pointer is valid and we have unique access for
        // the duration of `'a`.
        unsafe { &mut *ptr }

>>> +    /// Returns true if auto-negotiation is completed.
>>> +    pub fn is_autoneg_completed(&self) -> bool {
>>> +        const AUTONEG_COMPLETED: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>>> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
>>> +    }
>>> +
>>> +    /// Sets the speed of the PHY.
>>> +    pub fn set_speed(&self, speed: u32) {
>>
>> This function modifies state, but is `&self`?
> 
> Boqun asked me to drop mut on v3 review and then you ask why on v4?
> Trying to find a way to discourage developpers to write Rust
> abstractions? :)
> 
> I would recommend the Rust reviewers to make sure that such would
> not happen. I really appreciate comments but inconsistent reviewing is
> painful.

I agree with Boqun. Before Boqun's suggestion all functions were
`&mut self`. Now all functions are `&self`. Both are incorrect. A
function that takes `&mut self` can modify the state of `Self`,
but it is weird for it to not modify anything at all. Such a
function also can only be called by a single thread (per instance
of `Self`) at a time. Functions with `&self` cannot modify the
state of `Self`, except of course with interior mutability. If
they do modify state with interior mutability, then they should
have a good reason to do that.

What I want you to do here is think about which functions should
be `&mut self` and which should be `&self`, since clearly just
one or the other is wrong here.

>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        // So an FFI call with a valid pointer.
>>> +        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
>>> +        if ret < 0 {
>>> +            Err(Error::from_errno(ret))
>>> +        } else {
>>> +            Ok(ret as u16)
>>> +        }
>>> +    }
>>
>> [...]
>>
>>> +}
>>> +
>>> +/// Defines certain other features this PHY supports (like interrupts).
>>
>> Maybe add a link where these flags can be used.
> 
> I already put the link to here in trait Driver.

I am asking about a link here, as it is a bit confusing when
you just stumble over this flag module here. It doesn't hurt
to link more.

>>> +pub struct Registration {
>>> +    drivers: &'static [DriverType],
>>> +}
>>> +
>>> +impl Registration {
>>> +    /// Registers a PHY driver.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// The values of the `drivers` array must be initialized properly.
>>
>> With the above change you do not need this (since all instances of
>> `DriverType` are always initialized). But I am not sure if it would be
> 
> Nice.
> 
> 
>> fine to call `phy_driver_register` multiple times with the same driver
>> without unregistering it first.
> 
> The second call `phy_driver_register` with the same drivers (without
> unregistered) returns an error. You don't need to worry.

I see, then it's fine.

>>> +    /// Get a `mask` as u32.
>>> +    pub const fn mask_as_int(&self) -> u32 {
>>> +        self.mask.as_int()
>>> +    }
>>> +
>>> +    // macro use only
>>> +    #[doc(hidden)]
>>> +    pub const fn as_mdio_device_id(&self) -> bindings::mdio_device_id {
>>
>> I would name this just `mdio_device_id`.
> 
> Either is fine by me. Please tell me why for future reference.

Functions starting with `as_` or `to_` in Rust generally indicate
some kind of conversion. `to_` functions generally take just `self`
by value and `as_` conversions take just `&self`/`&mut self`. See
`Option::as_ref` or `Option::as_mut`. This function is not really
a conversion, rather it is a getter.

-- 
Cheers,
Benno

Powered by blists - more mailing lists