Message-ID: <CALNs47saY2AaBjp8XEadDfAw1+Su5+aszu_07RB=9X+rTW+_pg@mail.gmail.com>
Date: Sat, 7 Oct 2023 19:26:54 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, 
	miguel.ojeda.sandonis@...il.com, greg@...ah.com
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

On Sat, Oct 7, 2023 at 6:59 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
> > Can you just add `An instance of a PHY` to the docs for reference?
>
> You meant something like?
>
> /// An instance of a PHY device.
> /// Wraps the kernel's `struct phy_device`.
> ///
> /// # Invariants
> ///
> /// `self.0` is always in a valid state.
> #[repr(transparent)]
> pub struct Device(Opaque<bindings::phy_device>);

Seems good to me. I know that largely we want users to refer to the C
docs, but I think a small hint is good.

Fwiw they can be on the same line, Markdown combines them

> >> +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.
> >> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> >> +        unsafe { &mut *ptr.cast() }
> >> +    }
> >
> > The safety comment here still needs something like
>
> You meant the following?
>
> /// 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 with the exception of fields that are
> /// synchronized via the `lock` mutex.
>
> What this means? We use Device only when an exclusive access is
> gurannteed by device->lock. As discussed before, resume/suspend are
> called without device->lock locked but still drivers assume an
> exclusive access.

This was in follow up to one of the notes on the RFC patches, I'll
reply more to Andrew's comment about this

>
> >> +    /// Sets the speed of the PHY.
> >> +    pub fn set_speed(&mut self, speed: u32) {
> >> +        let phydev = self.0.get();
> >> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> >> +        unsafe { (*phydev).speed = speed as i32 };
> >> +    }
> >
> > Since we're taking user input, it probably doesn't hurt to do some
> > sort of sanity check rather than casting. Maybe warn once then return
> > the biggest nowrapping value
> >
> >     let speed_i32 = i32::try_from(speed).unwrap_or_else(|_| {
> >         warn_once!("excessive speed {speed}");
> >         i32::MAX
> >     })
> >     unsafe { (*phydev).speed = speed_i32 };
>
> warn_once() is available? I was thinking about adding it after the PHY
> patchset.
>
> I'll change set_speed to return Result.

Nevermind, I guess we don't have `warn_once`. Andrew mentioned
something about potentially lossy conversions, I'll follow up there.

> >> +    /// Executes software reset the PHY via BMCR_RESET bit.
> >> +    pub fn genphy_soft_reset(&mut self) -> Result {
> >> +        let phydev = self.0.get();
> >> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> >> +        // So an FFI call with a valid pointer.
> >> +        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
> >> +    }
> >> +
> >> +    /// Initializes the PHY.
> >> +    pub fn init_hw(&mut self) -> Result {
> >> +        let phydev = self.0.get();
> >> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> >> +        // so an FFI call with a valid pointer.
> >> +        to_result(unsafe { bindings::phy_init_hw(phydev) })
> >> +    }
> >
> > Andrew, are there any restrictions about calling phy_init_hw more than
> > once? Or are there certain things that you are not allowed to do until
> > you call that function?
>
> From quick look, you can call it multiple times.

Thanks, no worries in that case


> > If so, maybe a simple typestate would make sense here
> >
> >> +impl<T: Driver> Adapter<T> {
> >> +    unsafe extern "C" fn soft_reset_callback(
> >> +        phydev: *mut bindings::phy_device,
> >> +    ) -> core::ffi::c_int {
> >> +        from_result(|| {
> >> +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> >> +            let dev = unsafe { Device::from_raw(phydev) };
> >> +            T::soft_reset(dev)?;
> >> +            Ok(0)
> >> +        })
> >> +    }
> >
> > All of these functions need a `# Safety` doc section, you could
> > probably just say to follow `Device::from_raw`'s rules. And then you
> > can update the comments to say caller guarantees preconditions
> >
> > If you care to, these functions are so similar that you could just use
> > a macro to make your life easier
> >
> >     macro_rules! make_phydev_callback{
> >         ($fn_name:ident, $c_fn_name:ident) => {
> >             /// ....
> >             /// # Safety
> >             /// `phydev` must be valid and registered
> >             unsafe extern "C" fn $fn_name(
> >                 phydev: *mut ::bindings::phy_device
> >             ) -> $ret_ty {
> >                 from_result(|| {
> >                     // SAFETY: Preconditions ensure `phydev` is valid and
> >                     let dev = unsafe { Device::from_raw(phydev) };
> >                     T::$c_fn_name(dev)?;
> >                     Ok(0)
> >                 }
> >             }
> >         }
> >     }
> >
> >     make_phydev_callback!(get_features_callback, get_features);
> >     make_phydev_callback!(suspend_callback, suspend);
>
> Looks nice. I use the following macro.

Miguel mentioned on Zulip that we try to avoid macros (I did not know
this), so I guess it's fine if redundant.

https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.60bool.3A.3Athen.60.20helper/near/395398830

> >> +/// Declares a kernel module for PHYs drivers.
> >> +///
> >> +/// This creates a static array of `struct phy_driver` and registers it.
> >> +/// This also corresponds to the kernel's MODULE_DEVICE_TABLE macro, which embeds the information
> >> +/// for module loading into the module binary file.
> >
> > Could you add information about the relationship between drivers and
> > device_table?
>
> device_table needs to have PHY Ids that one of the drivers can handle.
>
> ?

I think something like "Every driver needs an entry in device_table"
is fine, just make it less easy to miss

Thanks for the followup, all of these were very minor

Powered by blists - more mailing lists