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

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

On 12/13/23 00:01, FUJITA Tomonori wrote:
> On Tue, 12 Dec 2023 17:35:34 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> 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?
> 
> If phy_device points to a valid object, phy_device.mdio is valid.

Where is this documented?

> A mii_bus must exist before a phy device. A bus is scanned and then a
> phy device is found (so phy_device object is crated).
> 
> https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634

I know that this is the status. But for this to be useful as a
justification it must be written down somewhere where you can expect to
find it. Not some knowledge that "everyone just knows".
I would prefer to use the solution that I detailed in the other thread.

-- 
Cheers,
Benno