Message-Id: <20231015.073929.156461103776360133.fujita.tomonori@gmail.com>
Date: Sun, 15 Oct 2023 07:39:29 +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 17:07:09 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

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

Sure, I'll try.


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

Ok.

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

Can you tell me why exactly you think in that way?

Firstly, you think that reading & updating some stats using `&self` should be fine.

What's the difference between read() and set_speed(), which you think, needs &mut self.

Because set_speed() updates the member in phy_device and read()
updates the object that phy_device points to?


Secondly, What's the difference between read() and write(), where you
think that read() is &self write() is &mut self.

read() is reading from hardware register. write() is writing a value
to hardware register. Both updates the object that phy_device points
to?


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

Ok.

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

It already returns &mut Device so no change is necessary here, right?

unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self
{

If you want more additional comment on from_raw(), please let me know.


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

As Andrew replied, all the functions update some member in phy_device.

Powered by blists - more mailing lists