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

FUJITA Tomonori
2023-10-07T10:59:01Z

On Sat, 7 Oct 2023 01:06:04 -0400
Trevor Gross <tmgross@umich.edu> wrote:

> On Fri, Oct 6, 2023 at 5:49 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>> +/// 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>);
>
> 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>);

>> +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.

>> + /// Gets the id of the PHY.
>> + pub fn phy_id(&mut self) -> u32 {
>> + let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> + unsafe { (*phydev).phy_id }
>> + }
>> +
>> + /// Gets the state of the PHY.
>> + pub fn state(&mut self) -> DeviceState {
>> + let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> + let state = unsafe { (*phydev).state };
>> + match state {
>> + bindings::phy_state::PHY_DOWN => DeviceState::Down,
>> + bindings::phy_state::PHY_READY => DeviceState::Ready,
>> + bindings::phy_state::PHY_HALTED => DeviceState::Halted,
>> + bindings::phy_state::PHY_ERROR => DeviceState::Error,
>> + bindings::phy_state::PHY_UP => DeviceState::Up,
>> + bindings::phy_state::PHY_RUNNING => DeviceState::Running,
>> + bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
>> + bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
>> + }
>> + }
>
> Could you add a comment like `// FIXME:enum-cast` or something? Then
> when we have a better solution for enums handling we can revise this.

Added.

>> + /// 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.

>> + /// 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.

> 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.

macro_rules! make_phydev_callback {
($fn_name: ident) => {
::kernel::macros::paste! {
/// # Safety
///
/// `phydev` must be passed by callback functions in `phy_driver`.
unsafe extern "C" fn [<$fn_name _callback>] (
phydev: *mut bindings::phy_device
) -> core::ffi::c_int {
from_result(|| {
// SAFETY: Preconditions ensure `phydev` is valid.
let dev = unsafe { Device::from_raw(phydev) };
T::$fn_name(dev)?;
Ok(0)
})
}
}
};
}

>> + unsafe extern "C" fn read_mmd_callback(
>> + phydev: *mut bindings::phy_device,
>> + devnum: i32,
>> + regnum: u16,
>> + ) -> i32 {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + let ret = T::read_mmd(dev, devnum as u8, regnum)?;
>> + Ok(ret.into())
>> + })
>> + }
>
> Since your're reading a bus, it probably doesn't hurt to do a quick
> check when converting
>
> let devnum_u8 = u8::try_from(devnum).(|_| {
> warn_once!("devnum {devnum} exceeds u8 limits");
> code::EINVAL
> })?
> // ...

Sure.

>> + unsafe extern "C" fn write_mmd_callback(
>> + phydev: *mut bindings::phy_device,
>> + devnum: i32,
>> + regnum: u16,
>> + val: u16,
>> + ) -> i32 {
>> + from_result(|| {
>> + // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
>> + let dev = unsafe { Device::from_raw(phydev) };
>> + T::write_mmd(dev, devnum as u8, regnum, val)?;
>> + Ok(0)
>> + })
>> + }
>
> Same as above with the conversion errors
>
>
>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>> +pub const fn create_phy_driver<T: Driver>() -> Opaque<bindings::phy_driver> {
>> + Opaque::new(bindings::phy_driver {
>> + name: T::NAME.as_char_ptr() as *mut i8,
>
> `.cast_mut()`, just makes the mutability change more clear

Done.

> I guess the C side could technically be `const char *name`
>
>> + // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
>> + // sets `Option<&F>` to be `None`.
>> + ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
>> + })
>> +}
>
> Btw I double checked and this should be OK to use, hopefully will be
> stable in the near future
> https://github.com/rust-lang/rust/pull/116218

Thanks!

>> +/// 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.

?

>> +/// # Examples
>> +///
>> +/// ```ignore
>> +///
>> +/// use kernel::net::phy::{self, DeviceId, Driver};
>> +/// use kernel::prelude::*;
>> +///
>> +/// kernel::module_phy_driver! {
>> +/// drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>> +/// device_table: [
>> +/// DeviceId::new_with_driver::<PhyAX88772A>(),
>> +/// DeviceId::new_with_driver::<PhyAX88772C>(),
>> +/// DeviceId::new_with_driver::<PhyAX88796B>()
>> +/// ],
>> +/// type: RustAsixPhy,
>> +/// name: "rust_asix_phy",
>> +/// author: "Rust for Linux Contributors",
>> +/// description: "Rust Asix PHYs driver",
>> +/// license: "GPL",
>> +/// }
>> +/// ```
>
> I can't find the discussion we had about this, but you said you have
> the `type` parameter to be consistent with `module!`, correct?

No, `driver!` in rust branch, which is used by platform, amba, etc.

https://github.com/Rust-for-Linux/linux/blob/rust/samples/rust/rust_platform.rs

> I think that it is more important to be consistent with C's
> `MODULE_PHY_DRIVER` where you don't need to specify anything extra,
> since the module doesn't do anything else. And I think it is less
> confusing for users if they don't wonder why they need to define a
> type they never use.
>
> Why not just remove the field and create an internal type based on
> `name` for now? We can always make it an optional field later on if it
> turns out there is a use case.

Sure, I'll try. I have no preference and driver! macro isn't in
upstream.