Re: [RFC PATCH v1 1/4] rust: core abstractions for network PHY drivers

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Sep 13, 2023 at 11:05:31PM +0200, Andrew Lunn wrote:
> > > > +impl Device {
> > > > +    /// Creates a new [`Device`] instance from a raw pointer.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
> > > > +    /// may read or write to the `phy_device` object.
> > > 
> > > Well that is untrue. phylib passes phydev to the MAC driver, in its
> > 
> > Note that a "# Safety" section before an unsafe function is the "safety
> > requirement", i.e. the things that callers must obey the requirement in
> > order to call these unsafe functions.
> 
> So by lifetime, it means while it is inside this function?
> 

No ;-) Let's take a look at the function:

	pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
	    unsafe { &mut *(ptr as *mut Self) }
	}

"'a" is a lifetime notation in Rust, so usually we call it

	lifetime 'a

now, you can see the function returns a "&'a mut Self", (Self is Device
here), so it's: a mutable reference to Device whose lifetime is 'a,
until now 'a is still a lifetime variable. When the function is called,
you can think as a concrete lifetime is bound to that variable, for
example:

	{
		// create a mutable reference whose lifetime is '1
		// '1 is a concrete lifetime starts at here.
		let dev: &'1 mut Device = unsafe { Device::from_raw(..) };

		// '1 continues here
		<use dev as a mutable reference to Device>
		// '1 continues here
	} // "dev" is out of scope, so '1 ends here.

in above case, '1 is bound to 'a. A concrete lifetime can usually be
thought of as a region in the code.

Back to the safety requirement, it basically says the caller should
guarantee that as long as the return mutable reference is alive, the
underlying object (phy_device in memory) should be exclusively
accessable. For example, in the above case, it's the caller's
responsiblitly to guarantee "dev" is always a valid reference (during
lifetime '1).
			

Others may explain better than me, but the above is the conceptual model
that I use, hope it helps.


> > > > +    /// Executes software reset the PHY via BMCR_RESET bit.
> > > > +    pub fn soft_reset(&mut self) -> Result {
> > > 
> > > I suggest you keep to the genphy_ naming if possible, to make it clear
> > > this is using a generic PHY method.
> > > 
> > 
> > My opinion may not matter, but this function is actually
> > 
> > 	net::phy::Device::soft_reset()
> > 
> > , so it could be a step backwards if we use a prefix in a language that
> > support namespace ;-) But I'll leave it the people who will read the
> > code day-to-day.
> 
> So a bit of background. IEEE 802.3 defines how a well behaved PHY
> should work. We have a number of helper functions based on that
> specification, all starting with the prefix genphy_. We expect PHY
> drivers to use these helpers where possible. It helps reviewing driver
> submissions, in that if we see genphy_foo we know it is good quality
> core code. If it is not genphy_foo, we need to review it closely.
> 
> When reviewing a Rust PHY driver, i want the same hint. This is a
> wrapper around a trusted genphy_foo C function, so i don't need to
> look too closely at it.
> 

Understood ;-)

> > Agreed, these functions have no users. Also the wrappers doesn't reflect
> > the lock requirements, at least not in comments, after a quick look, I
> > suppose all the functions should be under phy_device->lock?
> 
> The core will take that locking before calling into the driver. So the
> driver never should need to touch phydev->lock. It is one of those
> rules of thumb:
> 
> 	Driver writers often don't understand locking, so try to
> 	ensure the core does all the locking, not the driver.

Make sense. I brought that up because I noticed that Fujita used all
"&mut self" (i.e. mutable references) for all the functions, and mutable
references mean exclusive accesses, which can usually be thought as
"must be called with locked held" (if the underlying object is avaiable
for multipl threads).

Regards,
Boqun

> 
>      Andrew