On 15.10.23 00:39, FUJITA Tomonori wrote:

On Sat, 14 Oct 2023 17:07:09 +0000
Benno Lossin [off-list ref] wrote:

quoted

quoted

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:

     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.

quoted

quoted

quoted

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.

quoted

quoted

quoted

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