Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

Trevor Gross
2023-10-07T23:27:08Z

On Sat, Oct 7, 2023 at 6:59 AM FUJITA Tomonori
<fujita.tomonori@gmail.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