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

[Benno Lossin <benno.lossin@proton.me>]

On 12/12/23 14:02, FUJITA Tomonori wrote:
> 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())
> };

I still think you need to justify why `mdio.bus` is a pointer that you
can give to `midobus_read`. After looking at the C code, it seems like
that the pointer needs to point to a valid `struct mii_bus`.
This *could* just be an invariant of `struct phy_device` [1], but where
do we document that?

We could make an exception here and treat this differently until bindgen
can handle the `static inline` functions, but I am not so sure if we
want to have this as a general pattern. We need to discuss this more.


[1]: Technically it is a combination of the following invariants:
- the `mdio` field of `struct phy_device` is a valid `struct mido_device`
- the `bus` field of `struct mdio_device` is a valid pointer to a valid
  `struct mii_bus`.

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

This would be fine if we decide to go with the exception I detailed
above. Although instead of "open code" I would write "see implementation
of `phy_read()`".

-- 
Cheers,
Benno