Message-ID: <98471d44-c267-4c80-ba54-82ab2563e465@proton.me>
Date: Tue, 17 Oct 2023 07:06:38 +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 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`).

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

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

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

Yes it already is correct.

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

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

-- 
Cheers,
Benno

Powered by blists - more mailing lists