Date: Sat, 21 Oct 2023 07:36:31 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <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, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

On 20.10.23 20:42, Andrew Lunn wrote:
>>> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
>>> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
>>> +//! a hardware register and updates the stats.
>>
>> I would use [`Device`] instead of `phy_device`, since the Rust reader
>> might not be aware what wraps `phy_device`.
> 
> We don't want to hide phy_device too much, since at the moment, the
> abstraction is very minimal. Anybody writing a driver is going to need
> a good understanding of the C code in order to find the helpers they
> need, and then add them to the abstraction. So i would say we need to
> explain the relationship between the C structure and the Rust
> structure, to aid developers.

There is a comment on `Device` that explains that it wraps `phy_device`.
Since [`Device`] is a link, readers who do not know what it means can
immediately click the link and find out. This is not possible with
`phy_device`, since you have to search the web for it, so I would
prefer to use the link.

>>> +    /// Returns true if the link is up.
>>> +    pub fn get_link(&self) -> bool {
>>
>> I still think this name should be changed. My response at [1] has not yet
>> been replied to. This has already been discussed before:
>> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
>> - https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/
>> - https://lore.kernel.org/rust-for-linux/CALNs47syMxiZBUwKLk3vKxzmCbX0FS5A37FjwUzZO9Fn-iPaoA@mail.gmail.com/
>>
>> And I want to suggest to change it to `is_link_up`.
>>
>> Reasons why I do not like the name:
>> - `get_`/`put_` are used for ref counting on the C side, I would like to
>>     avoid confusion.
>> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>>     such as a hashmap, so I expect the call to do some computation.
>> - getters in Rust usually are not prefixed with `get_`, but rather are
>>     just the name of the field.
>> - in this case I like the name `is_link_up` much better, since code becomes
>>     a lot more readable with that.
>> - I do not want this pattern as an example for other drivers.
>>
>> [1]: https://lore.kernel.org/rust-for-linux/f5878806-5ba2-d932-858d-dda3f55ceb67@proton.me/
>>
>>> +        const LINK_IS_UP: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>>> +        phydev.link() == LINK_IS_UP
>>> +    }
> 
> During the reviews we have had a lot of misunderstanding what this
> actually does, given its name. Some thought it poked around in
> registers to get the current state of the link. Some thought it
> triggered the PHY to establish a link. When in fact its just a dumb
> getter. And we have a few other dumb getters and setters.

IMO `is_link_up` would indicate that it is a dumb getter.

> So i would prefer something which indicates its a dumb getter. If the
> norm of Rust is just the field name, lets just use the field name. But
> we should do that for all the getters and setters. Is there a naming
> convention for things which take real actions?

For bool getters it would be the norm to use `is_` as the prefix, see
[1]. In this case I would say it is also natural to not use `is_link`,
but rather `is_link_up`, since the former sounds weird.

[1]: https://doc.rust-lang.org/std/?search=is_

> And maybe we need to add a comment: Get the current link state, as
> stored in the [`Device`]. Set the duplex value in [`Device`], etc.

Sure, we can document dumb getters explicitly, I would prefer to do:
Getter for the current link state. Setter for the duplex value.

I don't think that we need to link to `Device`, since the functions
are defined on that type.

-- 
Cheers,
Benno

Powered by blists - more mailing lists