Message-ID: <9d70de37-c5ed-4776-a00f-76888e1230aa@proton.me>
Date: Sat, 14 Oct 2023 17:07:09 +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 18:15, FUJITA Tomonori wrote:
> 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?

I would not put it in the `# Safety` section. Instead, put this
information on the phy module itself (the `//!` comments at the very
top of the file). I also would suggest to not take the above paragraph
word by word, but to improve and extend it.

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

The information in this paragraph would also fit nicely into the
phy module docs.

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

I think for reading & updating some stats using `&self`
should be fine. `write` should probably be `&mut self`.

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

Those setter functions should be `&mut self`.

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

I see, then the `from_raw` function should definitely return
a `&mut Device`. Note that you can still call `&T` functions
when you have a `&mut T`.

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

What about these functions?
- resolve_aneg_linkmode
- genphy_soft_reset
- init_hw
- start_aneg
- genphy_read_status
- genphy_update_link
- genphy_read_lpa
- genphy_read_abilities

-- 
Cheers,
Benno

Powered by blists - more mailing lists