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

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Mon, 11 Dec 2023 22:11:15 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> > // 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" ?

As Alice suggested, I updated the comment. The current comment is:

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call.
let ret = unsafe {
    bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
};

If phy_read() is called here, I assume that you are happy about the
above comment. The way to call mdiobus_read() here is safe because it
just an open code of phy_read(). Simply adding it works for you?

// SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
// So it's just an FFI call, open code of `phy_read()`.