Date: Fri, 17 Nov 2023 09:39:03 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: fujita.tomonori@...il.com
Cc: andrew@...n.ch, benno.lossin@...ton.me, miguel.ojeda.sandonis@...il.com, 
	netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, tmgross@...ch.edu, 
	wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers

FUJITA Tomonori <fujita.tomonori@...il.com> writes:
> This patch adds abstractions to implement network PHY drivers; the
> driver registration and bindings for some of callback functions in
> struct phy_driver and many genphy_ functions.
> 
> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>

I promised Andrew to take a look at these patches at Plumbers. This
email contains the first part of my review.

In this email, I will bring up the question of how the safety comments
should be worded. I know that you've probably discussed this before, but
my opinion was asked for, and this is the main area where I think
there's room for improvement.

> +    /// # Safety
> +    ///
> +    /// This function must only be called from the callbacks in `phy_driver`.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {

This kind of safety comment where you say "must only be used by internal
code and nothing else" isn't great. It doesn't really help with checking
the correctness. It's usually better to document what is actually
required here, even if it shouldn't be called by non-internal code. I
recommend something along the lines of:

	# Safety
	
	For the duration of 'a, the pointer must point at a valid `phy_device`,
	and the caller must hold the X mutex.

Then in methods like this: (which are missing justification for why
there's no data race!)
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).phy_id }
you instead say:

	SAFETY: By the struct invariants, `phydev` points at a valid
	`phy_device`, and we hold the X lock, which gives us access to
	the `phy_id` field.

And you would also update the struct invariant accordingly:

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);





> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
> +// to the instance.

I used "X mutex" as an example for the synchronization mechanism in the
above snippets, but it sounds like its more complicated than that? Here
are some possible alternatives I could come up with:

Maybe we don't need synchronization when some operations can't happen?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held, or that there are no concurrent operations of type Y.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Maybe we have a separate case for when the device is being initialized
and nobody else has access yet?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the X
/// mutex is held, or that the reference has exclusive access to the
/// entire `phy_device`.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Maybe it is easier to just list the fields we need access to?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts exclusive
/// access to the following fields: phy_id, state, speed, duplex. And
/// read access to the following fields: link, autoneg_complete,
/// autoneg.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

Perhaps we want to avoid duplication with some existing C documentation?

/// # Invariants
///
/// Referencing a `phy_device` using this struct asserts that the user
/// is inside a Y scope as defined in Documentation/foo/bar.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);

But I don't know how these things are actually synchronized. Maybe
it is some sixth option. I would be happy to help draft these safety
comments once the actual synchronization mechanism is clear to me.

Or maybe you prefer to not do it this way, or to punt it for a later
patch series. I prefer to document these things in the above way, but
ultimately it is not up to me.

Alice

Powered by blists - more mailing lists