Date: Sat, 14 Oct 2023 14:54:30 +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 12:32, FUJITA Tomonori wrote:
> 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.

Maybe explain this in the docs? In the future, when I will come
into contact with this again, I will probably have forgotten this
conversation, but the docs are permanent and can be re-read.

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

That remark seems to me to only apply to the return type of
`assume_locked` in that thread.

> Anyway,
> 
> phy_id()
> state()
> get_link()
> is_autoneg_enabled()
> is_autoneg_completed()
> 
> doesn't modify Self.

yes, these should all be `&self`.

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

What exactly does it update? In Rust there is interior mutability
which is used to implement mutexes. Interior mutability allows
you to modify values despite only having a `&T` (for more info
see [1]). Our `Opaque<T>` type uses this pattern as well (since
you get a `*mut T` from `&Opaque<T>`) and it is the job of the
abstraction writer to figure out what mutability to use.

[1]: https://doc.rust-lang.org/reference/interior-mutability.html

I have no idea what exactly `read_*` modifies on the C side.
Mapping C functions to `&self`, `&mut self` and other receiver types
is not obvious in all cases. I would focus more on the following aspect
of `&mut self` and `&self`:

Since `&mut self` is unique, only one thread per instance of `Self`
can call that function. So use this when the C side would use a lock.
(or requires that only one thread calls that code)

Since multiple `&self` references are allowed to coexist, you should
use this for functions which perform their own serialization/do not
require serialization.

If you cannot decide what certain function receivers should be, then
we can help you, but I would need more info on what the C side is doing.

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

IIRC you can directly link to the field:

     [`Driver::FLAGS`]

Also maybe split the sentence. So one idea would be:

     /// Defines certain other features this PHY supports (like interrupts).
     ///
     /// These flag values are used in [`Driver::FLAGS`].

-- 
Cheers,
Benno

Powered by blists - more mailing lists