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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 19 Oct 2023 09:41:47 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
 tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com,
 benno.lossin@...ton.me, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network
 PHY drivers

On Wed, 18 Oct 2023 22:27:55 +0200
Andrew Lunn <andrew@...n.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.


>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
>> +    }
>> +
>> +    /// Writes a given C22 PHY register.
>> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
>> +        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.
>> +        to_result(unsafe {
>> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
>> +        })
>> +    }
>> +
>> +    /// Reads a paged register.
>> +    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> 
> From my reading of the code, read_paged also does not modify phydev.

__phy_read is called so I use &mut self like read().