Date: Sun, 15 Oct 2023 01:15:02 +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 14:54:30 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

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

You meant adding this to the code?  like dding this to Device's #
Safety comment?


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

I guess that the rest are &mut self but let me continue to make sure.

I think that you already know that Device instance only was created in
the callbacks. Before the callbacks are called, PHYLIB holds
phydev->lock except for resume()/suspend(). As explained in the
previous mail, only one thread calls resume()/suspend().

btw, methods in Device calling a C side function like mdiobus_read,
mdiobus_write, etc which never touch phydev->lock. Note that the c
side functions in resume()/suspned() methods don't touch phydev->lock
too.

There are two types how the methods in Device changes the C side data.

1. read/write/read_paged

They call the C side functions, mdiobus_read, mdiobus_write,
phy_read_paged, respectively.

phy_device has a pointer to mii_bus object. It has stats for
read/write. So everytime they are called, stats is updated.

2. the rest

The C side functions in the rest of methods in Device updates some
members in phy_device like set_speed() method does.


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

just to be sure, the C side guarantees that only one reference exists.


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

If you need more info on the C side, please let me know.


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

Thanks, will do.

Powered by blists - more mailing lists