| Message ID | 20231006094911.3305152-2-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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? > +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 with the exception of fields that are synchronized via the `lock` mutex > + /// 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. > + /// 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 }; > + /// 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? 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); > + 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 })? // ... > + 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 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 > +/// 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? > +/// # 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? 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. - Trevor
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.
On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote: > > 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}"); NEVER call WARN() on user input, as you now just rebooted the machine and caused a DoS (and syzbot will start to spam you with reports.) Remember, the majority of Linux systems in the world run with panic-on-warn enabled, so any user path that can cause this, will be used to cause problems. thanks, greg k-h
On Sat, 7 Oct 2023 13:17:13 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote: >> > 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}"); > > NEVER call WARN() on user input, as you now just rebooted the machine > and caused a DoS (and syzbot will start to spam you with reports.) Trevor uses `user` as the user of this function, which is a PHY driver.
On Sat, Oct 07, 2023 at 08:23:24PM +0900, FUJITA Tomonori wrote: > On Sat, 7 Oct 2023 13:17:13 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Sat, Oct 07, 2023 at 07:58:57PM +0900, FUJITA Tomonori wrote: > >> > 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}"); > > > > NEVER call WARN() on user input, as you now just rebooted the machine > > and caused a DoS (and syzbot will start to spam you with reports.) > > Trevor uses `user` as the user of this function, which is a PHY driver. > Ok, same thing in a way, just do a dev_warn() and return an error, no need to do a full traceback splat at all. thanks, greg k-h
> > + /// 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 After reading the thread, we first have a terminology problem. In the kernel world, 'user input' generally means from user space. And user space should never be trusted, but user space should also not be allowed to bring the system to its knees. Return -EINVAL to userspace is the correct thing to do and keep going. Don't do a kernel splat because the user passed 42 as a speed, not 10. However, what Trevor was meaning is that whoever called set_speed() passed an invalid value. But what are valid values? We have this file which devices the KAPI https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1883 says: /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. and we also have #define SPEED_UNKNOWN -1 and there is a handy little helper: static inline int ethtool_validate_speed(__u32 speed) { return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN; } so if you want to validate speed, call this helper. However, this is a kernel driver, and we generally trust kernel drivers. There is not much we can do except trust them. And passing an invalid speed here is very unlikely to cause the kernel to explode sometime later. Andrew
> The safety comment here still needs something like > > with the exception of fields that are synchronized via the `lock` mutex I'm not sure that really adds much useful information. Which values are protected by the lock? More importantly, which are not protected by the lock? As a general rule of thumb, driver writers don't understand locking. Yes, there are some which do, but many don't. So the workaround to that is make it so they don't need to understand locking. All the locking happens in the core. The exception is suspend and resume, which are called without the lock. So if i was to add a comment about locking, i would only put a comment on those two. > > + /// 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? phy_init_hw can be called multiple times. It used by drivers as a work around to broken hardware/firmware to get the device back into a good state. It is also used during resume, since often the PHY looses its settings when suspended. > > + 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 > })? I would actually say this is the wrong place to do that. Such checks should happen in the core, so it checks all drivers, not just the current one Rust driver. Feel free to submit a C patch adding this. Andrew
On Sat, 07 Oct 2023 19:58:57 +0900 (JST) FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: >>> +/// # 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. To create an internal type based on `name`, we need to unstringify `name`? I can't find a easy way to do it.
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
On Sat, Oct 7, 2023 at 10:48 AM Andrew Lunn <andrew@lunn.ch> wrote: > After reading the thread, we first have a terminology problem. In the > kernel world, 'user input' generally means from user space. And user > space should never be trusted, but user space should also not be > allowed to bring the system to its knees. Return -EINVAL to userspace > is the correct thing to do and keep going. Don't do a kernel splat > because the user passed 42 as a speed, not 10. Yes, sorry about the terminology. I was indeed referring to a user of this function and am still figuring out the failure modes (thanks also Greg for the corrections). > However, what Trevor was meaning is that whoever called set_speed() > passed an invalid value. But what are valid values? > > We have this file which devices the KAPI > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1883 > > says: > > /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. > > and we also have > > #define SPEED_UNKNOWN -1 > > and there is a handy little helper: > > static inline int ethtool_validate_speed(__u32 speed) > { > return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN; > } > > so if you want to validate speed, call this helper. > > However, this is a kernel driver, and we generally trust kernel > drivers. There is not much we can do except trust them. And passing an > invalid speed here is very unlikely to cause the kernel to explode > sometime later. > > Andrew I didn't mean the suggestion to be a way of handling untrusted input, just a means of providing a fallback value with module author feedback if something > INT_MAX winds up getting passed somehow. Agreed that this is more than necessary for such a trivial situation, the original is of course fine.
On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > The safety comment here still needs something like > > > > with the exception of fields that are synchronized via the `lock` mutex > > I'm not sure that really adds much useful information. Which values > are protected by the lock? More importantly, which are not protected > by the lock? > > As a general rule of thumb, driver writers don't understand > locking. Yes, there are some which do, but many don't. So the > workaround to that is make it so they don't need to understand > locking. All the locking happens in the core. > > The exception is suspend and resume, which are called without the > lock. So if i was to add a comment about locking, i would only put a > comment on those two. This doesn't get used by driver implementations, it's only used within the abstractions here. I think anyone who needs the details can refer to the C side, I just suggested to note the locking caveat based on your second comment at https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/ Fujita - since this doesn't get exposed, could this be pub(crate)?) > > 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? > > phy_init_hw can be called multiple times. It used by drivers as a work > around to broken hardware/firmware to get the device back into a good > state. It is also used during resume, since often the PHY looses its > settings when suspended. Great, thank you for the clarification > > > + 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 > > })? > > I would actually say this is the wrong place to do that. Such checks > should happen in the core, so it checks all drivers, not just the > current one Rust driver. Feel free to submit a C patch adding this. > > Andrew I guess it does that already: https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556 Fujita, I think we started doing comments when we know that lossy/bitwise `as` casts are correct. Maybe just leave the code as-is but add // CAST: the C side verifies devnum < 32 ?
On Sat, Oct 7, 2023 at 6:33 PM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > To create an internal type based on `name`, we need to unstringify > `name`? I can't find a easy way to do it. I think you should just be able to do it with `paste!` macro_rules! module_phy_driver { (name: $name:expr) => { paste::paste! { #[allow(non_camel_case_types)] struct [<$name _ty>]; } } } // creates struct `demo_driver_ty` module_phy_driver! { name: "demo_driver" }
On Sun, 8 Oct 2023 02:19:46 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Sat, Oct 7, 2023 at 6:33 PM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> To create an internal type based on `name`, we need to unstringify >> `name`? I can't find a easy way to do it. > > I think you should just be able to do it with `paste!` > > macro_rules! module_phy_driver { > (name: $name:expr) => { > paste::paste! { > #[allow(non_camel_case_types)] > struct [<$name _ty>]; > } > } > } > > // creates struct `demo_driver_ty` > module_phy_driver! { > name: "demo_driver" > } > I realized that we don't need `name`. The name of struct doesn't matter so I use `Module`. I tried to use `name` for the name of device_table however the variable name of the table isn't embeded into the module binary so it doesn't matter. FYI, I use paste! but got the following error: = help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier = note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info) #[macro_export] macro_rules! module_phy_driver { (@replace_expr $_t:tt $sub:expr) => {$sub}; (@count_devices $($x:expr),*) => { 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))* }; (@device_table $name:tt, [$($dev:expr),+]) => { ::kernel::macros::paste! { #[no_mangle] static [<__mod_mdio__ $name _device_table>]: [ kernel::bindings::mdio_device_id; $crate::module_phy_driver!(@count_devices $($dev),+) + 1 ] = [ $(kernel::bindings::mdio_device_id { phy_id: $dev.id, phy_id_mask: $dev.mask_as_int() }),+, kernel::bindings::mdio_device_id { phy_id: 0, phy_id_mask: 0 } ]; } }; (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], name: $name:tt, $($f:tt)*) => { struct Module { _reg: kernel::net::phy::Registration, } $crate::prelude::module! { type: Module, name: $name, $($f)* } static mut DRIVERS: [ kernel::types::Opaque<kernel::bindings::phy_driver>; $crate::module_phy_driver!(@count_devices $($driver),+) ] = [ $(kernel::net::phy::create_phy_driver::<$driver>()),+ ]; impl kernel::Module for Module { fn init(module: &'static ThisModule) -> Result<Self> { // SAFETY: static `DRIVERS` array is used only in the C side. let mut reg = unsafe { kernel::net::phy::Registration::register(module, &DRIVERS) }?; Ok(Module { _reg: reg, }) } } $crate::module_phy_driver!(@device_table $name, [$($dev),+]); } }
On Sun, Oct 8, 2023 at 3:49 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > I realized that we don't need `name`. The name of struct doesn't > matter so I use `Module`. I tried to use `name` for the name of > device_table however the variable name of the table isn't embeded into > the module binary so it doesn't matter. > > FYI, I use paste! but got the following error: > > = help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier > = note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the > macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info) Too bad, this seems to be a limitation of our paste macro compared to the published one. What you have with `Module` seems fine so I think you don't need it? I'll submit a patch to fix paste sometime this week, don't wait on it though of course.
On Sun, 8 Oct 2023 04:54:52 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Sun, Oct 8, 2023 at 3:49 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> I realized that we don't need `name`. The name of struct doesn't >> matter so I use `Module`. I tried to use `name` for the name of >> device_table however the variable name of the table isn't embeded into >> the module binary so it doesn't matter. >> >> FYI, I use paste! but got the following error: >> >> = help: message: `"__mod_mdio__\"rust_asix_phy\"_device_table"` is not a valid identifier >> = note: this error originates in the macro `$crate::module_phy_driver` which comes from the expansion of the >> macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info) > > Too bad, this seems to be a limitation of our paste macro compared to > the published one. What you have with `Module` seems fine so I think > you don't need it? Yeah, now I don't use paste! in PHY bindings.
On Sun, Oct 8, 2023 at 5:03 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Yeah, now I don't use paste! in PHY bindings. Even better! The fix was small so I sent it anyway, https://lore.kernel.org/rust-for-linux/20231008094816.320424-1-tmgross@umich.edu/T/#u
On Sun, 8 Oct 2023 02:07:44 -0400 Trevor Gross <tmgross@umich.edu> wrote: > On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@lunn.ch> wrote: >> >> > The safety comment here still needs something like >> > >> > with the exception of fields that are synchronized via the `lock` mutex >> >> I'm not sure that really adds much useful information. Which values >> are protected by the lock? More importantly, which are not protected >> by the lock? >> >> As a general rule of thumb, driver writers don't understand >> locking. Yes, there are some which do, but many don't. So the >> workaround to that is make it so they don't need to understand >> locking. All the locking happens in the core. >> >> The exception is suspend and resume, which are called without the >> lock. So if i was to add a comment about locking, i would only put a >> comment on those two. > > This doesn't get used by driver implementations, it's only used within > the abstractions here. I think anyone who needs the details can refer > to the C side, I just suggested to note the locking caveat based on > your second comment at > https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/ > > Fujita - since this doesn't get exposed, could this be pub(crate)?) Device? I don't think so. If we make Device pub(crate), we need to make trait Driver pub(crate) too. >> > > + 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 >> > })? >> >> I would actually say this is the wrong place to do that. Such checks >> should happen in the core, so it checks all drivers, not just the >> current one Rust driver. Feel free to submit a C patch adding this. >> >> Andrew > > I guess it does that already: > https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556 > > Fujita, I think we started doing comments when we know that > lossy/bitwise `as` casts are correct. Maybe just leave the code as-is > but add > > // CAST: the C side verifies devnum < 32 Ok. As I commented on the RFC reviewing, I don't think that we need try_from conversion for values from PHYLIB. Implementing bindings for untrusted stuff doesn't make sense. https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/ On the other hand, I think that it might worth to use try_from for set_speed() because its about the bindings and Rust PHY drivers. However, I leave it alone since likely setting a wrong value doesn't break anything.
On Sun, Oct 8, 2023 at 10:28 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Fujita - since this doesn't get exposed, could this be pub(crate)?) > > Device? I don't think so. If we make Device pub(crate), we need to > make trait Driver pub(crate) too. Not `Device`, just `pub fn from_raw`. I think it is fine as-is (I see you sent a new version) > > Fujita, I think we started doing comments when we know that > > lossy/bitwise `as` casts are correct. Maybe just leave the code as-is > > but add > > > > // CAST: the C side verifies devnum < 32 > > Ok. As I commented on the RFC reviewing, I don't think that we > need try_from conversion for values from PHYLIB. Implementing bindings > for untrusted stuff doesn't make sense. > > https://lore.kernel.org/all/20230926.101928.767176570707357116.fujita.tomonori@gmail.com/ > > On the other hand, I think that it might worth to use try_from for > set_speed() because its about the bindings and Rust PHY drivers. > However, I leave it alone since likely setting a wrong value doesn't > break anything. Agreed, thanks for the followup
diff --git a/init/Kconfig b/init/Kconfig index 6d35728b94b2..4b4e3df1658d 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1889,6 +1889,7 @@ config RUST depends on !GCC_PLUGINS depends on !RANDSTRUCT depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE + depends on PHYLIB=y select CONSTRUCTORS help Enables Rust support in the kernel. diff --git a/rust/Makefile b/rust/Makefile index 87958e864be0..f67e55945b36 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -331,6 +331,7 @@ quiet_cmd_bindgen = BINDGEN $@ cmd_bindgen = \ $(BINDGEN) $< $(bindgen_target_flags) \ --use-core --with-derive-default --ctypes-prefix core::ffi --no-layout-tests \ + --rustified-enum phy_state\ --no-debug '.*' \ -o $@ -- $(bindgen_c_flags_final) -DMODULE \ $(bindgen_target_cflags) $(bindgen_target_extra) 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> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e8811700239a..f9883bde4459 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -14,6 +14,7 @@ #![no_std] #![feature(allocator_api)] #![feature(coerce_unsized)] +#![feature(const_maybe_uninit_zeroed)] #![feature(dispatch_from_dyn)] #![feature(new_uninit)] #![feature(receiver_trait)] @@ -36,6 +37,7 @@ pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit; +pub mod net; pub mod prelude; pub mod print; mod static_assert; diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs new file mode 100644 index 000000000000..fbb6d9683012 --- /dev/null +++ b/rust/kernel/net.rs @@ -0,0 +1,5 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Networking. + +pub mod phy; diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs new file mode 100644 index 000000000000..d06a83ad8a8a --- /dev/null +++ b/rust/kernel/net/phy.rs @@ -0,0 +1,706 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Network PHY device. +//! +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h). + +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque}; +use core::marker::PhantomData; + +/// Corresponds to the kernel's `enum phy_state`. +#[derive(PartialEq)] +pub enum DeviceState { + /// PHY device and driver are not ready for anything. + Down, + /// PHY is ready to send and receive packets. + Ready, + /// PHY is up, but no polling or interrupts are done. + Halted, + /// PHY is up, but is in an error state. + Error, + /// PHY and attached device are ready to do work. + Up, + /// PHY is currently running. + Running, + /// PHY is up, but not currently plugged in. + NoLink, + /// PHY is performing a cable test. + CableTest, +} + +/// Represents duplex mode. +pub enum DuplexMode { + /// Full-duplex mode + Half, + /// Half-duplex mode + Full, + /// Unknown + Unknown, +} + +/// 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() } + } + + /// 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, + } + } + + /// Returns true if the link is up. + pub fn get_link(&mut self) -> bool { + const LINK_IS_UP: u32 = 1; + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).link() == LINK_IS_UP } + } + + /// Returns true if auto-negotiation is enabled. + pub fn is_autoneg_enabled(&mut self) -> bool { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE } + } + + /// Returns true if auto-negotiation is completed. + pub fn is_autoneg_completed(&mut self) -> bool { + const AUTONEG_COMPLETED: u32 = 1; + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).autoneg_complete() == AUTONEG_COMPLETED } + } + + /// 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 }; + } + + /// Sets duplex mode. + pub fn set_duplex(&mut self, mode: DuplexMode) { + let phydev = self.0.get(); + let v = match mode { + DuplexMode::Full => bindings::DUPLEX_FULL as i32, + DuplexMode::Half => bindings::DUPLEX_HALF as i32, + DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32, + }; + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).duplex = v }; + } + + /// Reads a given C22 PHY register. + pub fn read(&mut self, regnum: u16) -> Result<u16> { + 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. + let ret = unsafe { + bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into()) + }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Writes a given C22 PHY register. + pub fn write(&mut self, regnum: u16, val: u16) -> 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::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val) + }) + } + + /// Reads a paged register. + pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> { + 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. + let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Resolves the advertisements into PHY settings. + pub fn resolve_aneg_linkmode(&mut self) { + 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. + unsafe { bindings::phy_resolve_aneg_linkmode(phydev) }; + } + + /// 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) }) + } + + /// Starts auto-negotiation. + pub fn start_aneg(&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_start_aneg(phydev) }) + } + + /// Resumes the PHY via BMCR_PDOWN bit. + pub fn genphy_resume(&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_resume(phydev) }) + } + + /// Suspends the PHY via BMCR_PDOWN bit. + pub fn genphy_suspend(&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_suspend(phydev) }) + } + + /// Checks the link status and updates current link state. + pub fn genphy_read_status(&mut self) -> Result<u16> { + 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. + let ret = unsafe { bindings::genphy_read_status(phydev) }; + if ret < 0 { + Err(Error::from_errno(ret)) + } else { + Ok(ret as u16) + } + } + + /// Updates the link status. + pub fn genphy_update_link(&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_update_link(phydev) }) + } + + /// Reads Link partner ability. + pub fn genphy_read_lpa(&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_read_lpa(phydev) }) + } + + /// Reads PHY abilities. + pub fn genphy_read_abilities(&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_read_abilities(phydev) }) + } +} + +/// Defines certain other features this PHY supports (like interrupts). +pub mod flags { + /// PHY is internal. + pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL; + /// PHY needs to be reset after the refclk is enabled. + pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN; + /// Polling is used to detect PHY status changes. + pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST; + /// Don't suspend. + pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND; +} + +/// An adapter for the registration of a PHY driver. +struct Adapter<T: Driver> { + _p: PhantomData<T>, +} + +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) + }) + } + + unsafe extern "C" fn get_features_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::get_features(dev)?; + Ok(0) + }) + } + + unsafe extern "C" fn suspend_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::suspend(dev)?; + Ok(0) + }) + } + + unsafe extern "C" fn resume_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::resume(dev)?; + Ok(0) + }) + } + + unsafe extern "C" fn config_aneg_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::config_aneg(dev)?; + Ok(0) + }) + } + + unsafe extern "C" fn read_status_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::read_status(dev)?; + Ok(0) + }) + } + + unsafe extern "C" fn match_phy_device_callback( + phydev: *mut bindings::phy_device, + ) -> core::ffi::c_int { + // SAFETY: The C API guarantees that `phydev` is valid while this function is running. + let dev = unsafe { Device::from_raw(phydev) }; + T::match_phy_device(dev) as i32 + } + + 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()) + }) + } + + 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) + }) + } + + unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) { + // SAFETY: The C API guarantees that `phydev` is valid while this function is running. + let dev = unsafe { Device::from_raw(phydev) }; + T::link_change_notify(dev); + } +} + +/// 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, + flags: T::FLAGS, + phy_id: T::PHY_DEVICE_ID.id, + phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), + soft_reset: if T::HAS_SOFT_RESET { + Some(Adapter::<T>::soft_reset_callback) + } else { + None + }, + get_features: if T::HAS_GET_FEATURES { + Some(Adapter::<T>::get_features_callback) + } else { + None + }, + match_phy_device: if T::HAS_MATCH_PHY_DEVICE { + Some(Adapter::<T>::match_phy_device_callback) + } else { + None + }, + suspend: if T::HAS_SUSPEND { + Some(Adapter::<T>::suspend_callback) + } else { + None + }, + resume: if T::HAS_RESUME { + Some(Adapter::<T>::resume_callback) + } else { + None + }, + config_aneg: if T::HAS_CONFIG_ANEG { + Some(Adapter::<T>::config_aneg_callback) + } else { + None + }, + read_status: if T::HAS_READ_STATUS { + Some(Adapter::<T>::read_status_callback) + } else { + None + }, + read_mmd: if T::HAS_READ_MMD { + Some(Adapter::<T>::read_mmd_callback) + } else { + None + }, + write_mmd: if T::HAS_WRITE_MMD { + Some(Adapter::<T>::write_mmd_callback) + } else { + None + }, + link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY { + Some(Adapter::<T>::link_change_notify_callback) + } else { + None + }, + // 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() } + }) +} + +/// Corresponds to functions in `struct phy_driver`. +/// +/// This is used to register a PHY driver. +#[vtable] +pub trait Driver { + /// Defines certain other features this PHY supports. + const FLAGS: u32 = 0; + /// The friendly name of this PHY type. + const NAME: &'static CStr; + /// This driver only works for PHYs with IDs which match this field. + const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0); + + /// Issues a PHY software reset. + fn soft_reset(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Probes the hardware to determine what abilities it has. + fn get_features(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Returns true if this is a suitable driver for the given phydev. + /// If not implemented, matching is based on [`PHY_DEVICE_ID`]. + fn match_phy_device(_dev: &mut Device) -> bool { + false + } + + /// Configures the advertisement and resets auto-negotiation + /// if auto-negotiation is enabled. + fn config_aneg(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Determines the negotiated speed and duplex. + fn read_status(_dev: &mut Device) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Suspends the hardware, saving state if needed. + fn suspend(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Resumes the hardware, restoring state if needed. + fn resume(_dev: &mut Device) -> Result { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD read function for reading a MMD register. + fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> { + Err(code::ENOTSUPP) + } + + /// Overrides the default MMD write function for writing a MMD register. + fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result { + Err(code::ENOTSUPP) + } + + /// Callback for notification of link change. + fn link_change_notify(_dev: &mut Device) {} +} + +/// Registration structure for a PHY driver. +/// +/// # Invariants +/// +/// The `drivers` points to an array of `struct phy_driver`, which is +/// registered to the kernel via `phy_drivers_register`. +pub struct Registration { + drivers: Option<&'static [Opaque<bindings::phy_driver>]>, +} + +impl Registration { + /// Registers a PHY driver. + #[must_use] + pub fn register( + module: &'static crate::ThisModule, + drivers: &'static [Opaque<bindings::phy_driver>], + ) -> Result<Self> { + if drivers.len() == 0 { + return Err(code::EINVAL); + } + // SAFETY: `drivers` has static lifetime and used only in the C side. + to_result(unsafe { + bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0) + })?; + Ok(Registration { + drivers: Some(drivers), + }) + } +} + +impl Drop for Registration { + fn drop(&mut self) { + if let Some(drv) = self.drivers.take() { + // SAFETY: The type invariants guarantee that self.drivers is valid. + unsafe { bindings::phy_drivers_unregister(drv[0].get(), drv.len() as i32) }; + } + } +} + +// SAFETY: `Registration` does not expose any of its state across threads. +unsafe impl Send for Registration {} + +// SAFETY: `Registration` does not expose any of its state across threads. +unsafe impl Sync for Registration {} + +/// Represents the kernel's `struct mdio_device_id`. +pub struct DeviceId { + /// Corresponds to `phy_id` in `struct mdio_device_id`. + pub id: u32, + mask: DeviceMask, +} + +impl DeviceId { + /// Creates a new instance with the exact match mask. + pub const fn new_with_exact_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Exact, + } + } + + /// Creates a new instance with the model match mask. + pub const fn new_with_model_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Model, + } + } + + /// Creates a new instance with the vendor match mask. + pub const fn new_with_vendor_mask(id: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Vendor, + } + } + + /// Creates a new instance with a custom match mask. + pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self { + DeviceId { + id, + mask: DeviceMask::Custom(mask), + } + } + + /// Creates a new instance from [`Driver`]. + pub const fn new_with_driver<T: Driver>() -> Self { + T::PHY_DEVICE_ID + } + + /// Get a mask as u32. + pub const fn mask_as_int(self) -> u32 { + self.mask.as_int() + } +} + +enum DeviceMask { + Exact, + Model, + Vendor, + Custom(u32), +} + +impl DeviceMask { + const MASK_EXACT: u32 = !0; + const MASK_MODEL: u32 = !0 << 4; + const MASK_VENDOR: u32 = !0 << 10; + + const fn as_int(self) -> u32 { + match self { + DeviceMask::Exact => Self::MASK_EXACT, + DeviceMask::Model => Self::MASK_MODEL, + DeviceMask::Vendor => Self::MASK_VENDOR, + DeviceMask::Custom(mask) => mask, + } + } +} + +/// 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. +/// +/// # 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", +/// } +/// ``` +#[macro_export] +macro_rules! module_phy_driver { + (@replace_expr $_t:tt $sub:expr) => {$sub}; + + (@count_devices $($x:expr),*) => { + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))* + }; + + (@device_table $name:ident, [$($dev:expr),+]) => { + ::kernel::macros::paste! { + #[no_mangle] + static [<__mod_mdio__ $name _device_table>]: [ + kernel::bindings::mdio_device_id; + $crate::module_phy_driver!(@count_devices $($dev),+) + 1 + ] = [ + $(kernel::bindings::mdio_device_id { + phy_id: $dev.id, + phy_id_mask: $dev.mask_as_int() + }),+, + kernel::bindings::mdio_device_id { + phy_id: 0, + phy_id_mask: 0 + } + ]; + } + }; + + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], type: $modname:ident, $($f:tt)*) => { + struct Module<$modname> { + _reg: kernel::net::phy::Registration, + _p: core::marker::PhantomData<$modname>, + } + + type ModuleType = Module<$modname>; + + $crate::prelude::module! { + type: ModuleType, + $($f)* + } + + static mut DRIVERS: [ + kernel::types::Opaque<kernel::bindings::phy_driver>; + $crate::module_phy_driver!(@count_devices $($driver),+) + ] = [ + $(kernel::net::phy::create_phy_driver::<$driver>()),+ + ]; + + impl kernel::Module for Module<$modname> { + fn init(module: &'static ThisModule) -> Result<Self> { + // SAFETY: static `DRIVERS` array is used only in the C side. + let mut reg = unsafe { kernel::net::phy::Registration::register(module, &DRIVERS) }?; + + Ok(Module { + _reg: reg, + _p: core::marker::PhantomData, + }) + } + } + + $crate::module_phy_driver!(@device_table $modname, [$($dev),+]); + } +}
This patch adds abstractions to implement network PHY drivers; the driver registration and bindings for some of callback functions in struct phy_driver and many genphy_ functions. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- init/Kconfig | 1 + rust/Makefile | 1 + rust/bindings/bindings_helper.h | 3 + rust/kernel/lib.rs | 2 + rust/kernel/net.rs | 5 + rust/kernel/net/phy.rs | 706 ++++++++++++++++++++++++++++++++ 6 files changed, 718 insertions(+) create mode 100644 rust/kernel/net.rs create mode 100644 rust/kernel/net/phy.rs