Re: [PATCH net-next v10 1/4] rust: core abstractions for network PHY drivers

[Boqun Feng <boqun.feng@gmail.com>]

On Tue, Dec 12, 2023 at 10:46:50AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 16:49:39 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> >> 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