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

Boqun Feng
2023-10-29T18:10:16Z

On Sun, Oct 29, 2023 at 09:48:41AM -0700, Boqun Feng wrote:
> On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> [...]
> >
> > The current code is fine from Rust perspective because the current
> > code copies phy_driver on stack and makes a reference to the copy, if
> > I undertand correctly.
> >
>
> I had the same thought Benno brought the issue on `&`, but unfortunately
> it's not true ;-) In the following code:
>
> let phydev = unsafe { *self.0.get() };
>
> , semantically the *whole* `bindings::phy_device` is being read, so if
> there is any modification (i.e. write) that may happen in the meanwhile,
> it's data race, and data races are UB (even in C).
>
> So both implementations have the problem because of the same cause.
>
> > It's not nice to create an 500-bytes object on stack. It turned out
> > that it's not so simple to avoid it.
>
> As you can see, copying is not the way to work around this.
>

An temporary solution is doing the #2 option from Benno, but in Rust and
open code it, like the following:

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 145d0407fe31..f5230ac48014 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -121,10 +121,10 @@ pub fn state(&self) -> DeviceState {
///
/// It returns true if the link is up.
pub fn is_link_up(&self) -> bool {
- const LINK_IS_UP: u32 = 1;
+ const LINK_IS_UP: u64 = 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
+ let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+ bit_field.get(14, 1) == LINK_IS_UP
}

/// Gets the current auto-negotiation configuration.
@@ -132,18 +132,18 @@ pub fn is_link_up(&self) -> bool {
/// It returns true if auto-negotiation is enabled.
pub fn is_autoneg_enabled(&self) -> bool {
// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
- let phydev = unsafe { *self.0.get() };
- phydev.autoneg() == bindings::AUTONEG_ENABLE
+ let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+ bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64
}

/// Gets the current auto-negotiation state.
///
/// It returns true if auto-negotiation is completed.
pub fn is_autoneg_completed(&self) -> bool {
- const AUTONEG_COMPLETED: u32 = 1;
+ const AUTONEG_COMPLETED: u64 = 1;
// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
- let phydev = unsafe { *self.0.get() };
- phydev.autoneg_complete() == AUTONEG_COMPLETED
+ let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
+ bit_field.get(15, 1) == AUTONEG_COMPLETED
}

/// Sets the speed of the PHY.

Of course, it's not maintainable in longer term since it relies on
hard-coding the bit offset of these bit fields. But I think it's best we
can do from Linux kernel side. It's up to Andrew and Miguel whether this
temporary solution is OK.

Regards,
Boqun