| Message ID | 20240731042136.201327-3-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: add Applied Micro QT2025 PHY driver | expand |
On Wed, Jul 31, 2024 at 6:22 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Support phy_driver probe callback, used to set up device-specific > structures. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
On Wed, Jul 31, 2024 at 01:21:32PM +0900, FUJITA Tomonori wrote: > Support phy_driver probe callback, used to set up device-specific > structures. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > --- > rust/kernel/net/phy.rs | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > index fd40b703d224..99a142348a34 100644 > --- a/rust/kernel/net/phy.rs > +++ b/rust/kernel/net/phy.rs > @@ -338,6 +338,20 @@ impl<T: Driver> Adapter<T> { > }) > } > > + /// # Safety > + /// > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { > + from_result(|| { > + // SAFETY: This callback is called only in contexts > + // where we can exclusively access to `phy_device`, so the accessors on > + // `Device` are okay to call. This one is slightly different to other callbacks. probe is called without the mutex. Instead, probe is called before the device is published. So the comment is correct, but given how important Rust people take these SAFETY comments, maybe it should indicate it is different to others? Andrew
On Wed, Jul 31, 2024 at 2:24 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Wed, Jul 31, 2024 at 01:21:32PM +0900, FUJITA Tomonori wrote: > > Support phy_driver probe callback, used to set up device-specific > > structures. > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > --- > > rust/kernel/net/phy.rs | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs > > index fd40b703d224..99a142348a34 100644 > > --- a/rust/kernel/net/phy.rs > > +++ b/rust/kernel/net/phy.rs > > @@ -338,6 +338,20 @@ impl<T: Driver> Adapter<T> { > > }) > > } > > > > + /// # Safety > > + /// > > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. > > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { > > + from_result(|| { > > + // SAFETY: This callback is called only in contexts > > + // where we can exclusively access to `phy_device`, so the accessors on > > + // `Device` are okay to call. > > This one is slightly different to other callbacks. probe is called > without the mutex. Instead, probe is called before the device is > published. So the comment is correct, but given how important Rust > people take these SAFETY comments, maybe it should indicate it is > different to others? Interesting. Given that we don't hold the mutex, does that mean that some of the methods on Device are not safe to call in this context? Or is there something else that makes it okay to call them despite not holding the mutex? Alice
> > > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. > > > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { > > > + from_result(|| { > > > + // SAFETY: This callback is called only in contexts > > > + // where we can exclusively access to `phy_device`, so the accessors on > > > + // `Device` are okay to call. > > > > This one is slightly different to other callbacks. probe is called > > without the mutex. Instead, probe is called before the device is > > published. So the comment is correct, but given how important Rust > > people take these SAFETY comments, maybe it should indicate it is > > different to others? > > Interesting. Given that we don't hold the mutex, does that mean that > some of the methods on Device are not safe to call in this context? Or > is there something else that makes it okay to call them despite not > holding the mutex? probe is always the first method called on a device driver to match it to a device. Traditionally, if probe fails, the device is destroyed, since there is no driver to drive it. probe needs to complete successfully before the phy_device structure is published so a MAC driver can reference it. If it is not published, nothing can have access to it, so you don't need to worry about parallel activities on it. And a PHY driver does not need a probe function. Historically, probe was all about, can this driver drive this hardware. However, since we have ID registers in the hardware, we already know the driver can drive the hardware. So probe is now about setting up whatever needs setting up. For PHY drivers, there is often nothing, no local state needed, etc. So the probe is optional. Andrew
Thanks for the review! On Wed, 31 Jul 2024 14:32:18 +0200 Alice Ryhl <aliceryhl@google.com> wrote: >> > + /// # Safety >> > + /// >> > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. >> > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { >> > + from_result(|| { >> > + // SAFETY: This callback is called only in contexts >> > + // where we can exclusively access to `phy_device`, so the accessors on >> > + // `Device` are okay to call. >> >> This one is slightly different to other callbacks. probe is called >> without the mutex. Instead, probe is called before the device is >> published. So the comment is correct, but given how important Rust >> people take these SAFETY comments, maybe it should indicate it is >> different to others? > > Interesting. Given that we don't hold the mutex, does that mean that > some of the methods on Device are not safe to call in this context? Or > is there something else that makes it okay to call them despite not > holding the mutex? Before the callback, the device object was initialized properly by PHYLIB and no concurrent access so all the methods can be called safely (no kernel panic), I think. If the safety comment needs to updated, how about the following? SAFETY: This callback is called only in contexts where we can exclusively access to `phy_device` because it's not published yet, so the accessors on `Device` are okay to call.
On Thu, Aug 1, 2024 at 2:17 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Thanks for the review! > > On Wed, 31 Jul 2024 14:32:18 +0200 > Alice Ryhl <aliceryhl@google.com> wrote: > > >> > + /// # Safety > >> > + /// > >> > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. > >> > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { > >> > + from_result(|| { > >> > + // SAFETY: This callback is called only in contexts > >> > + // where we can exclusively access to `phy_device`, so the accessors on > >> > + // `Device` are okay to call. > >> > >> This one is slightly different to other callbacks. probe is called > >> without the mutex. Instead, probe is called before the device is > >> published. So the comment is correct, but given how important Rust > >> people take these SAFETY comments, maybe it should indicate it is > >> different to others? > > > > Interesting. Given that we don't hold the mutex, does that mean that > > some of the methods on Device are not safe to call in this context? Or > > is there something else that makes it okay to call them despite not > > holding the mutex? > > Before the callback, the device object was initialized properly by > PHYLIB and no concurrent access so all the methods can be called > safely (no kernel panic), I think. > > If the safety comment needs to updated, how about the following? > > SAFETY: This callback is called only in contexts where we can > exclusively access to `phy_device` because it's not published yet, so > the accessors on `Device` are okay to call. Yes, that sounds good to me. With the updated safety comment, feel free to include my Reviewed-by in your next version. Alice
On Wed, Jul 31, 2024 at 10:57 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > + /// `phydev` must be passed by the corresponding callback in `phy_driver`. > > > > + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { > > > > + from_result(|| { > > > > + // SAFETY: This callback is called only in contexts > > > > + // where we can exclusively access to `phy_device`, so the accessors on > > > > + // `Device` are okay to call. > > > > > > This one is slightly different to other callbacks. probe is called > > > without the mutex. Instead, probe is called before the device is > > > published. So the comment is correct, but given how important Rust > > > people take these SAFETY comments, maybe it should indicate it is > > > different to others? > > > > Interesting. Given that we don't hold the mutex, does that mean that > > some of the methods on Device are not safe to call in this context? Or > > is there something else that makes it okay to call them despite not > > holding the mutex? > > probe is always the first method called on a device driver to match it > to a device. Traditionally, if probe fails, the device is destroyed, > since there is no driver to drive it. probe needs to complete > successfully before the phy_device structure is published so a MAC > driver can reference it. If it is not published, nothing can have > access to it, so you don't need to worry about parallel activities on > it. > > And a PHY driver does not need a probe function. Historically, probe > was all about, can this driver drive this hardware. However, since we > have ID registers in the hardware, we already know the driver can > drive the hardware. So probe is now about setting up whatever needs > setting up. For PHY drivers, there is often nothing, no local state > needed, etc. So the probe is optional. Makes sense. Thanks for the explanation! Alice
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index fd40b703d224..99a142348a34 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -338,6 +338,20 @@ impl<T: Driver> Adapter<T> { }) } + /// # Safety + /// + /// `phydev` must be passed by the corresponding callback in `phy_driver`. + unsafe extern "C" fn probe_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int { + from_result(|| { + // SAFETY: This callback is called only in contexts + // where we can exclusively access to `phy_device`, so the accessors on + // `Device` are okay to call. + let dev = unsafe { Device::from_raw(phydev) }; + T::probe(dev)?; + Ok(0) + }) + } + /// # Safety /// /// `phydev` must be passed by the corresponding callback in `phy_driver`. @@ -511,6 +525,11 @@ pub const fn create_phy_driver<T: Driver>() -> DriverVTable { } else { None }, + probe: if T::HAS_PROBE { + Some(Adapter::<T>::probe_callback) + } else { + None + }, get_features: if T::HAS_GET_FEATURES { Some(Adapter::<T>::get_features_callback) } else { @@ -583,6 +602,11 @@ fn soft_reset(_dev: &mut Device) -> Result { kernel::build_error(VTABLE_DEFAULT_ERROR) } + /// Sets up device-specific structures during discovery. + fn probe(_dev: &mut Device) -> Result { + kernel::build_error(VTABLE_DEFAULT_ERROR) + } + /// Probes the hardware to determine what abilities it has. fn get_features(_dev: &mut Device) -> Result { kernel::build_error(VTABLE_DEFAULT_ERROR)
Support phy_driver probe callback, used to set up device-specific structures. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> --- rust/kernel/net/phy.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)