Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers
From: Andrew Lunn <andrew@lunn.ch>
Date: 2023-10-07 15:13:30
Also in:
rust-for-linux
The safety comment here still needs something like
with the exception of fields that are synchronized via the `lock` mutexI'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.
quoted
+ /// 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.
quoted
+ 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