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 08:47:53AM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Dec 2023 15:40:33 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Tue, Dec 12, 2023 at 08:15:05AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> >> +    /// 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