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 10:11:57PM +0200, Andrew Lunn wrote:
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index c91a3c24f607..ec4ee09a34ad 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -8,6 +8,9 @@
> >  
> >  #include <kunit/test.h>
> >  #include <linux/errname.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/mdio.h>
> > +#include <linux/phy.h>
> >  #include <linux/slab.h>
> >  #include <linux/refcount.h>
> >  #include <linux/wait.h>
> 
> How does this scale when there are 200 subsystems using binding
> helpers?
> 
> > +/// Wraps the kernel's `struct phy_device`.
> > +#[repr(transparent)]
> > +pub struct Device(Opaque<bindings::phy_device>);
> > +
> > +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. Here it's supposed to  describe
"what's the safe usage of `from_raw` function".

> adjust link callback. It can then read from the phy_device
> object. There are also API calls the MAC can use to change values in
> the phydev structure. The phylib core will also modify values in
> phydev. In theory, the core will protect the phy_device structure
> using the phydev->lock mutex, although we might be missing a few
> locks/unlocks, and suspend/resume does not take these locks because of
> deadlock. A phy driver however should not need to touch this lock, it
> is taken before there is a call into the driver.
> 
[...]
> 
> > +
> > +    /// 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.

> > +    /// Reads a given PHY register.
> > +    pub fn read(&mut self, regnum: u32) -> Result<i32> {
[...]
> > +    /// Reads a given PHY register without the MDIO bus lock taken.
> > +    pub fn lockless_read(&mut self, regnum: u32) -> Result<i32> {
> > +        let phydev = Opaque::get(&self.0);
> > +        // SAFETY: This object is initialized by the `from_raw` constructor,
> > +        // so an FFI call with a valid pointer.
> > +        let ret =
> > +            unsafe { bindings::__mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum) };
> > +        if ret < 0 {
> > +            Err(Error::from_errno(ret))
> > +        } else {
> > +            Ok(ret)
> > +        }
> > +    }
> 
> Now we are getting into the hairy stuff. Locking is
> interesting. Rather than duplicate all the interesting stuff, can you
> just use the high level C functions? In fact, the driver you posted
> does not need anything other than phy_read() and phy_write(), so i
> would not even bother with these yet.
> 

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?

> > +    /// Resumes the PHY via BMCR_PDOWN bit.
> > +    pub fn resume(&mut self) -> Result {

Regards,
Boqun