Date: Tue, 17 Oct 2023 16:32:49 +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 Tue, 17 Oct 2023 07:06:38 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> On 15.10.23 00:39, FUJITA Tomonori wrote:
>> On Sat, 14 Oct 2023 17:07:09 +0000
>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>> 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?
> 
> `set_speed` is entirely implemented on the Rust side and is not protected
> by a lock. Since data races in Rust are UB, this function must be `&mut`,
> in order to guarantee that no data races occur. This is the case, because
> our `Opaque` forces you to use interior mutability and thus sidestep this
> rule (modifying through a `&T`).

Understood.


>> Secondly, What's the difference between read() and write(), where you
>> think that read() is &self write() is &mut self.
> 
> This is just the standard Rust way of using mutability. For reading one
> uses `&self` and for writing `&mut self`. The only thing that is special
> here is the stats that are updated. But I thought that it still could fit
> Rust by the following pattern:
> ```rust
>      pub struct TrackingReader {
>          buf: [u8; 64],
>          num_of_reads: Mutex<usize>,
>      }
> 
>      impl TrackingReader {
>          pub fn read(&self, idx: usize) -> u8 {
>              *self.num_of_reads.lock() += 1;
>              self.buf[idx]
>          }
>      }
> 
> ```
> 
> And after taking a look at `mdiobus_read` I indeed found a mutex.

Yes, both read() and write() update the stats with mdiobus's lock.


>> read() is reading from hardware register. write() is writing a value
>> to hardware register. Both updates the object that phy_device points
>> to?
> 
> Indeed, I was just going with the standard way of suggesting `&self`
> for reads, there are of course exceptions where `&mut self` would make
> sense. That being said in this case both options are sound, since
> the C side locks a mutex.

I see. I use &mut self for both read() and write().


>>>>> 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.
> 
> Do all of these functions lock the `bus->mdio_lock`? If yes, then you
> can just treat them like `read` or `write` (both `&self` and `&mut self`
> will be sound) and use the standard Rust way of setting the mutability.
> So if it changes some internal state, I would go with `&mut self`.

They hold mdiobus's lock and update mdiobus stats. They also change
some internal state in phy_device without touching any lock (it's safe
since PHYLIB guarantes there is only one thread calling a callback for
one device).

I'll use &mut self for them.

Thanks a lot!

Powered by blists - more mailing lists