Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

[Benno Lossin <benno.lossin@proton.me>]

On 19.10.23 02:41, FUJITA Tomonori wrote:
> On Wed, 18 Oct 2023 22:27:55 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> +    /// Reads a given C22 PHY register.
>>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        // So an FFI call with a valid pointer.
>>> +        let ret = unsafe {
>>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>
>> If i've understood the discussion about &mut, it is not needed here,
>> and for write. Performing a read/write does not change anything in
>> phydev. There was mention of statistics, but they are in the mii_bus
>> structure, which is pointed to by this structure, but is not part of
>> this structure.
> 
> If I understand correctly, he said that either (&self or &mut self) is
> fine for read().
> 
> https://lore.kernel.org/netdev/3469de1c-0e6f-4fe5-9d93-2542f87ffd0d@proton.me/
> 
> 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)
> 
> Since multiple `&self` references are allowed to coexist, you should
> use this for functions which perform their own serialization/do not
> require serialization.
> 
> 
> I applied the first case here.

I will try to explain things a bit more.

So this case is a bit difficult to figure out, because what is
going on is not really a pattern that is used in Rust.
We already have exclusive access to the `phy_device`, so in Rust
you would not need to lock anything to also have exclusive access to the
embedded `mii_bus`. In this sense, mutable references (`&mut T`) are
infectious.

Since C always locks the `mdio_lock` when we call the read & write
functions, we however could also just use a shared reference (`&T`)
for the function receiver, since the C side guarantees serialization.

Another reason for choosing `&mut self` here is the following: it is
easier to later change to `&self` compared to going with `&self` now
and changing to `&mut self` later. This is because if you have a `&mut T`
you can also call all of its `&T` functions, but not the other way around.
`&mut self` is as a receiver also more conservative, since it is more
strict as to where it can be called. So let's just go with that.

-- 
Cheers,
Benno