netdev - Re: [PATCH net-next v4 1/4] 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: Tue, 17 Oct 2023 07:41: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 17.10.23 09:32, FUJITA Tomonori wrote:
> On Tue, 17 Oct 2023 07:06:38 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
>>> 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().

I would recommend documenting this somewhere (why `read` is `&mut`), since
that is a bit unusual (why restrict something more than necessary?).

-- 
Cheers,
Benno