Date: Mon, 11 Dec 2023 22:11:15 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: alice@...l.io, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org,
	andrew@...n.ch, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com,
	benno.lossin@...ton.me, wedsonaf@...il.com, aliceryhl@...gle.com
Subject: Re: [PATCH net-next v10 1/4] rust: core abstractions for network PHY
 drivers

On Tue, Dec 12, 2023 at 01:04:10PM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 18:30:36 -0800
> Boqun Feng <boqun.feng@...il.com> wrote:
> 
> > 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@...il.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.
> 
> I meant that phydev points to a valid object, thus mdio and mdio.addr
> do too. But after reading you phy_read() comment, I suspect that you
> aren't talking about if mdio.addr points to valid memory or not. Your
> point is about the validity of calling mdiobus_read() with mdio.addr?
> 

Yes, I was talking about the safety comment for calling mdiobus_read().

> 
> >> 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
> 
> The C side people read the Rust code before changing the C code? Let's
> see. 
> 

Hmm... I usually won't call someone "C side people". I mean, the project
has C part and Rust part, but the community is one.

In case of myself, I write both C and Rust, if I'm going to change some
C side function, I may want to see the usage at Rust side, especially
whether my changes could break the safety, and safety comments may be
important.

> 
> > 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:
> 
> Yeah, I hope that the discussion about inline C functions would end
> with a solution.
> 
> 
> > // 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.
> > 
> > ?
> 
> Looks ok to me but after a quick look at in-tree Rust code, I can't
> find a comment like X is valid for the first argument in this C
> function. What I found are comments like X points to valid memory.

Hmm.. maybe "is valid" could be a confusing term, so the point is: if
`phydev` is pointing to a properly maintained struct phy_device, then an
open code of phy_read() should be safe. Maybe "..., which should be safe
as long as `phydev` points to a valid struct phy_device" ?

Regards,
Boqun

Powered by blists - more mailing lists