Message-Id: <20231208.102842.1616218749853934366.fujita.tomonori@gmail.com>
Date: Fri, 08 Dec 2023 10:28:42 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu,
 miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com, aliceryhl@...gle.com,
 boqun.feng@...il.com
Subject: Re: [PATCH net-next v9 1/4] rust: core abstractions for network
 PHY drivers

On Thu, 07 Dec 2023 17:25:22 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> On 12/5/23 02:14, FUJITA Tomonori wrote:
>> @@ -0,0 +1,754 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2023 FUJITA Tomonori <fujita.tomonori@...il.com>
>> +
>> +//! Network PHY device.
>> +//!
>> +//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
>> +
>> +use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>> +
>> +use core::marker::PhantomData;
>> +
>> +/// PHY state machine states.
>> +///
>> +/// Corresponds to the kernel's [`enum phy_state`].
>> +///
>> +/// Some of PHY drivers access to the state of PHY's software state machine.
> 
> This sentence reads a bit weird, what are you trying to say?

It's copy of the PHY doc. For me, it means that if my PHY driver
doesn't need access to that state, I don't need to know anything about
this enum.


>> +/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
>> +#[derive(PartialEq, Eq)]
>> +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,
> 
> I took a look at `enum phy_state` and found that you only copied the
> first sentence of each state description, why is that?

I thought that the first sentence is enough but I'll copy the full
description if you prefer.


>> +/// A mode of Ethernet communication.
>> +///
>> +/// PHY drivers get duplex information from hardware and update the current state.
> 
> Are you trying to say that the driver automatically queries the
> hardware? You could express this more clearly.

It's the copy from the PHY doc. I assume that it's clear for driver
developers; your driver gets the information from the hardware and
updates the state via the APIs.


>> +pub enum DuplexMode {
>> +    /// PHY is in full-duplex mode.
>> +    Full,
>> +    /// PHY is in half-duplex mode.
>> +    Half,
>> +    /// PHY is in unknown duplex mode.
>> +    Unknown,
>> +}
>> +
>> +/// An instance of a PHY device.
>> +///
>> +/// Wraps the kernel's [`struct phy_device`].
>> +///
>> +/// A [`Device`] instance is created when a callback in [`Driver`] is executed. A PHY driver
>> +/// executes [`Driver`]'s methods during the callback.
>> +///
>> +/// # Invariants
>> +///
>> +/// Referencing a `phy_device` using this struct asserts that you are in
>> +/// a context where all methods defined on this struct are safe to call.
> 
> I know that Alice suggested this, but I reading it now, it sounds a
> bit weird. When reading this it sounds like a requirement for everyone
> using a `Device`. It would be better to phrase it so that it sounds like
> something that users of `Device` can rely upon.

I guess that every reviewer has their preferences. I don't think that
I can write a comment that makes every reviewer fully happy about.

For me, as Alice said, "at least it is correct". 


> Also, I would prefer for this invariant to be a simple one, for example:
> "The mutex of `self.0` is held".
> The only problem with that are the `resume` and `suspend` methods.
> Andrew mentioned that there is some tribal knowledge on this topic, but
> I don't see this written down anywhere here. I can't really suggest an
> improvement to invariant without knowing the whole picture.
> 
>> +/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
>> +// During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
>> +// unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
>> +// [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
>> +// the lock held, thus guaranteeing that [`Driver::resume`] has exclusive access to the instance.
>> +// [`Driver::resume`] and [`Driver::suspend`] also are called where only one thread can access
>> +// to the instance.
>> +#[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 'a, the pointer must point at a valid `phy_device`,
>> +    /// and the caller must be in a context where all methods defined on this struct
>> +    /// are safe to call.
>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>> +        let ptr = ptr.cast::<Self>();
>> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
>> +        // the duration of `'a`.
>> +        unsafe { &mut *ptr }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&self) -> u32 {
>> +        let phydev = self.0.get();
>> +        // SAFETY: The struct invariant ensures that we may access
>> +        // this field without additional synchronization.
> 
> At the moment the invariant only states that "all functions on
> `Device` are safe to call". It does not say anything about accessing
> fields. I hope this shows why I think the invariant is problematic.

The previous invariant was:

`self.0` is always in a valid state.

https://lore.kernel.org/netdev/20231026001050.1720612-1-fujita.tomonori@gmail.com/T/

It says accessing fields, right? For me, if so, the current invariant
suggested by Alice also says it.

Powered by blists - more mailing lists