From: Trevor Gross <tmgross@umich.edu>
To: FUJITA Tomonori <fujita.tomonori@gmail.com>
Cc: netdev@vger.kernel.org, rust-for-linux@vger.kernel.org,
andrew@lunn.ch, miguel.ojeda.sandonis@gmail.com, greg@kroah.com
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers
Date: Sat, 7 Oct 2023 01:06:04 -0400 [thread overview]
Message-ID: <CALNs47sdj2onJS3wFUVoONYL_nEgT+PTLTVuMLcmE6W6JgZAXA@mail.gmail.com> (raw)
In-Reply-To: <20231006094911.3305152-2-fujita.tomonori@gmail.com>
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
next prev parent reply other threads:[~2023-10-07 5:06 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-06 9:49 [PATCH v2 0/3] Rust abstractions for network PHY drivers FUJITA Tomonori
2023-10-06 9:49 ` [PATCH v2 1/3] rust: core " FUJITA Tomonori
2023-10-07 5:06 ` Trevor Gross [this message]
2023-10-07 10:58 ` FUJITA Tomonori
2023-10-07 11:17 ` Greg KH
2023-10-07 11:23 ` FUJITA Tomonori
2023-10-07 11:30 ` Greg KH
2023-10-07 22:33 ` FUJITA Tomonori
2023-10-08 6:19 ` Trevor Gross
2023-10-08 7:49 ` FUJITA Tomonori
2023-10-08 8:54 ` Trevor Gross
2023-10-08 9:02 ` FUJITA Tomonori
2023-10-08 9:58 ` Trevor Gross
2023-10-07 23:26 ` Trevor Gross
2023-10-07 14:47 ` Andrew Lunn
2023-10-08 5:41 ` Trevor Gross
2023-10-07 15:13 ` Andrew Lunn
2023-10-08 6:07 ` Trevor Gross
2023-10-08 14:28 ` FUJITA Tomonori
2023-10-09 3:07 ` Trevor Gross
2023-10-06 9:49 ` [PATCH v2 2/3] MAINTAINERS: add Rust PHY abstractions to the ETHERNET PHY LIBRARY FUJITA Tomonori
2023-10-06 9:49 ` [PATCH v2 3/3] net: phy: add Rust Asix PHY driver FUJITA Tomonori
2023-10-06 10:31 ` Greg KH
2023-10-06 13:53 ` FUJITA Tomonori
2023-10-06 14:12 ` Greg KH
2023-10-06 14:30 ` FUJITA Tomonori
2023-10-06 14:37 ` Greg KH
2023-10-06 14:40 ` Andrew Lunn
2023-10-06 14:35 ` Andrew Lunn
2023-10-06 15:26 ` FUJITA Tomonori
2023-10-06 15:57 ` Andrew Lunn
2023-10-06 16:21 ` FUJITA Tomonori
2023-10-06 16:55 ` Andrew Lunn
2023-10-06 23:54 ` FUJITA Tomonori
2023-10-07 0:20 ` Andrew Lunn
2023-10-07 7:41 ` FUJITA Tomonori
2023-10-07 7:19 ` Trevor Gross
2023-10-07 12:07 ` FUJITA Tomonori
2023-10-07 15:39 ` Andrew Lunn
2023-10-08 7:11 ` Trevor Gross
2023-10-07 15:35 ` Andrew Lunn
2023-10-08 7:17 ` Trevor Gross
2023-10-06 12:54 ` [PATCH v2 0/3] Rust abstractions for network PHY drivers Andrew Lunn
2023-10-06 14:09 ` FUJITA Tomonori
2023-10-06 14:47 ` Andrew Lunn
2023-10-06 23:37 ` Trevor Gross
2023-10-07 3:26 ` FUJITA Tomonori
2023-10-09 12:39 ` Miguel Ojeda
2023-10-07 0:42 ` Trevor Gross
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALNs47sdj2onJS3wFUVoONYL_nEgT+PTLTVuMLcmE6W6JgZAXA@mail.gmail.com \
--to=tmgross@umich.edu \
--cc=andrew@lunn.ch \
--cc=fujita.tomonori@gmail.com \
--cc=greg@kroah.com \
--cc=miguel.ojeda.sandonis@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=rust-for-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).