On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:

On Mon, 11 Dec 2023 16:49:39 -0800
Boqun Feng [off-list ref] wrote:

quoted

quoted

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`.

I thought that "`phydev` is pointing to a valid object by the type
invariant of `Self`." comment implies that "C side holds the invariants"

By the type invariant of `Self`, you mean:

/// # 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.

my read on that only tells me the context is guaranteed to be in a
driver callback, nothing has been said about all other invariants C side
should hold.

Do we need a comment about the C implementation details like
PHY_MAX_ADDR? It becomes harder to keep the comment sync with the C
side because the C code is changed any time.

Well, exactly, "the C code is changed any time", I thought having more
information in Rust helps people who is going to change the C side to
see whether they may break Rust side. Plus it's the safety comment, you
need to prove that it's safe to call the function, the function is
unsafe for a reason: there are inputs that may cause issues, and writing
the safety comment is a process to think and double check.

Maybe we can simplify this a little bit, since IIUC, you just want to
call phy_read() here, but due to that Rust cannot call inline C
functions directly, hence the open-code. How about:


// SAFETY: `phydev` points to valid object per the type invariant of
// `Self`, also the following just minics what `phy_read()` does in C
// side, which should be safe as long as `phydev` is valid.

?

Regards,
Boqun