Re: [PATCH net v3] rust: net: phy: fix off-by-one bit positions in device status accessors

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
rust: net: phy: fix off-by-one bit positions in device status accessors

This replaces the hand-written bitfield offsets in is_link_up(),
is_autoneg_enabled() and is_autoneg_completed() with the bindgen-generated
link(), autoneg() and autoneg_complete() accessors, so the Rust side no
longer depends on hard-coded bit numbers for struct phy_device.

> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 956cda573ddba..14dd1e6cb3802 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -124,38 +124,30 @@ pub fn state(&self) -> DeviceState {
>      ///
>      /// It returns true if the link is up.
>      pub fn is_link_up(&self) -> bool {
> -        const LINK_IS_UP: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(14, 1) == LINK_IS_UP
> +        unsafe { (*phydev).link() == 1 }
>      }

[Severity: Medium]
Bindgen emits the bitfield getters with the signature

  pub fn link(&self) -> ::core::ffi::c_uint

so writing (*phydev).link() auto-refs the receiver and materializes a
&bindings::phy_device covering the whole C struct, not just the bitfield
storage unit that the previous code projected with
&(*self.0.get())._bitfield_1.

Is forming that whole-struct shared reference allowed here? The pointer
comes from an Opaque, whose documented contract in rust/kernel/types.rs
says:

  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on
  ///   the Rust side.

and the Opaque example reads fields through the raw pointer
(unsafe { (*ptr).val }) instead of taking a reference.

Concurrent C-side writes into the same allocation look possible while a
PHY driver callback runs, none of them covered by phydev->lock:

drivers/net/phy/phy.c:phy_interrupt() {
	...
		phydev->irq_rerun = 1;
		disable_irq_nosync(irq);
	...
}

That is a hard IRQ context write to a bitfield member without taking
phydev->lock. In addition, the embedded state_queue delayed_work is written
by the timer softirq and workqueue core, the embedded mdio.dev kref is
bumped by get_device()/put_device() in phy_device.c, and the internals of
phydev->lock itself (wait_lock, wait_list) are written by contending
threads.

Would reading the bits through a C __rust_helper accessor in rust/helpers/,
or projecting to the bitfield unit via a raw-pointer path, keep the goal of
dropping the hand-written offsets without forming a reference to the whole
struct?

Also, the retained SAFETY comments still say "we may access this field",
which no longer matches what the code does. Should they be updated to
describe the reference that is actually created?

>  
>      /// Gets the current auto-negotiation configuration.
>      ///
>      /// It returns true if auto-negotiation is enabled.
>      pub fn is_autoneg_enabled(&self) -> bool {
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(13, 1) == u64::from(bindings::AUTONEG_ENABLE)
> +        unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE }
>      }
>  
>      /// Gets the current auto-negotiation state.
>      ///
>      /// It returns true if auto-negotiation is completed.
>      pub fn is_autoneg_completed(&self) -> bool {
> -        const AUTONEG_COMPLETED: u64 = 1;
> -        // TODO: the code to access to the bit field will be replaced with automatically
> -        // generated code by bindgen when it becomes possible.
> +        let phydev = self.0.get();
>          // SAFETY: The struct invariant ensures that we may access
>          // this field without additional synchronization.
> -        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> -        bit_field.get(15, 1) == AUTONEG_COMPLETED
> +        unsafe { (*phydev).autoneg_complete() == 1 }
>      }

[Severity: Low]
This isn't a bug introduced by this patch, but after the rewrite
is_link_up(), is_autoneg_enabled() and is_autoneg_completed() are one-line
forwarders to a binding call, and none of them carries #[inline].
rust/kernel is an abstraction crate rather than a leaf driver, so the
coding guidelines ask for #[inline] on small functions that just forward to
a binding call.

The neighbouring accessors in the same file (phy_id(), state(),
set_speed(), set_duplex()) are equally unannotated, so this is the existing
style of the file rather than something the patch changes. Would it be
worth adding the annotation to the three touched functions here, or
handling all of them in a separate cleanup?
-- 
pw-bot: cr