On Tue, Dec 12, 2023 at 08:47:53AM +0900, FUJITA Tomonori wrote:

On Mon, 11 Dec 2023 15:40:33 -0800
Boqun Feng [off-list ref] wrote:

quoted

On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
[...]

quoted

quoted

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.

I would also mention that `(*phydev).mdio.addr` is smaller than
PHY_MAX_ADDR (per C side invariants in mdio maybe), since otherwise
mdiobus_read() would cause out-of-bound accesses at ->stats. The safety
comments are supposed to describe why calling the C function won't cause
memory safety issues..

(*phydev).mdio.addr is managed in the C side and Rust code doesn't

It's OK to rely on C side to give a correct addr value.

touch it (doesn't need to know anything about it). What safety comment
should be written here?

Basically, here Rust just does the same as C does in phy_read(), right?
So why phy_read() is implemented correctly, because C side maintains the
`(*phydev).mdio.addr` in that way. We ususally don't call it out in C
code, since it's obvious(TM), and there is no safe/unsafe boundary in C
side. But in Rust code, that matters. Yes, Rust doesn't control the
value of `(*phydev).mdio.addr`, but Rust chooses to trust C, in other
words, as long as C side holds the invariants, calling mdiobus_read() is
safe here. How about 

// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also `(*phydev).mdio` is totally maintained by C in a way
// that `(*phydev).mdio.bus` is a pointer to a valid `mii_bus` and
// `(*phydev).mdio.addr` is less than PHY_MAX_ADDR, so it's safe to call
// `mdiobus_read`.

?

Regards,
Boqun