On Mon, 11 Dec 2023 22:46:01 +0100
Alice Ryhl [off-list ref] wrote:

On 12/11/23 00:49, FUJITA Tomonori wrote:

quoted

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.
This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
This patch enables unstable const_maybe_uninit_zeroed feature for
kernel crate to enable unsafe code to handle a constant value with
uninitialized data. With the feature, the abstractions can initialize
a phy_driver structure with zero easily; instead of initializing all
the members by hand. It's supposed to be stable in the not so distant
future.
Link: https://github.com/rust-lang/rust/pull/116218
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Overall looks fine to me. Just a few comments below that confuse me:

Thanks.

quoted

+    /// Gets the state of PHY state machine states.
+    pub fn state(&self) -> DeviceState {
+        let phydev = self.0.get();
+        // SAFETY: The struct invariant ensures that we may access
+        // this field without additional synchronization.
+        let state = unsafe { (*phydev).state };
+ // TODO: this conversion code will be replaced with automatically
generated code by bindgen
+        // when it becomes possible.
+        // better to call WARN_ONCE() when the state is out-of-range.

Did you mix up two comments here? This doesn't parse in my brain.

I'll remove the second comment because all we have to do here is using
bindgen.

quoted

+    /// Reads a given C22 PHY register.
+ // This function reads a hardware register and updates the stats so
takes `&mut self`.
+    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.

This sentence also doesn't parse in my brain. Perhaps "So it's just an
FFI call" or similar?

"So it's just an FFI call" looks good. I'll fix all the places that
use the same comment.