Message-Id: <20231014.193231.787565106108242584.fujita.tomonori@gmail.com>
Date: Sat, 14 Oct 2023 19:32:31 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, 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 Sat, 14 Oct 2023 08:07:03 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

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

>From my understanding, the callers of suspend() try to call suspend()
for a device only once. They lock a device and get the current state
and update the sate, then unlock the device. If the state is a
paticular value, then call suspend(). suspend() and resume() are also
called where only one thread can access a device.


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

Thanks, I'll copy-and-paste it.


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

https://lore.kernel.org/netdev/20231011.231607.1747074555988728415.fujita.tomonori@gmail.com/T/#mb7d219b2e17d3f3e31a0d05697d91eb8205c5c6e

Hmm, I undertood that he suggested all mut.

Anyway,

phy_id()
state()
get_link()
is_autoneg_enabled()
is_autoneg_completed()

doesn't modify Self.

The rest modifies then need to be &mut self? Note that function like read_*
updates the C data structure.


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

I can't find the code does the similar. What exactly do you expect?
Like this?

/// Defines certain other features this PHY supports (like interrupts) for [`Driver`]'s `FLAGS`.
pub mod flags {

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

I think that Trevor suggested that name. Either works for me but I'll
go with your suggestion.

Powered by blists - more mailing lists