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

Boqun Feng
2023-10-28T19:06:37Z

On Sat, Oct 28, 2023 at 04:39:08PM +0000, Benno Lossin wrote:
> On 28.10.23 18:09, FUJITA Tomonori wrote:
> > On Sat, 28 Oct 2023 16:53:30 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >
> >>>> We need to be careful here, since doing this creates a reference
> >>>> `&bindings::phy_device` which asserts that it is immutable. That is not
> >>>> the case, since the C side might change it at any point (this is the
> >>>> reason we wrap things in `Opaque`, since that allows mutatation even
> >>>> through sharde references).
> >>>
> >>> You meant that the C code might modify it independently anytime, not
> >>> the C code called the Rust abstractions might modify it, right?
> >>
> >> The whole locking model is base around that not happening. Things
> >> should only change with the lock held. I you make a call into the C

Here is my understanding, I treat references in Rust as a special
pointer, so having a `&bindings::phy_device` means the *entire* object
is immutable unless the fields are interior mutable, for example,
behind a `UnsafeCell` or `Opaque`, examples of interior mutable types
are atomic and locks.

Now since C doesn't have the "interior mutable" concept or these types,
so when bindgen generates the `bindings::phy_device`, none of the fields
of a lock or atomic is marked as `UnsafeCell` or `Opaque`. That's why
`Opaque` is needed for defining `Device`:

pub struct Device(Opaque<bindings::phy_device);

`Opaque` basically does two things:

1) tell compilers that the underlying content may be modified (of
course in some sort of serialization) when there exists a
`&Device` or `&mut Device`.

2) only provide `*mut bindings::phy_device`, so accessing the
underlying object has to use unsafe.

Now let's look back into struct phy_device, it does have a few locks
in it, and at least even with phydev->lock held, the content of
phydev->lock itself can be changed (e.g tick locks), hence it breaks the
requirement of the existence of a `&bindings::phy_device`.

Of course, if we can define our own `bindings::phy_device` or ask
bindgen to automatically add `Opaque` to certain types, then
`&bindings::phy_device` is still possible to use.

If we are OK to not use `&bindings::phy_device` then Benno's proposal in
bindgen is one way to work with this.

Regards,
Boqun

> >> side, then yes, it can and will change it. So you should not cache a
> >> value over a C call.
> >
> > Yeah, I understand that. But if I understand Benno correctly, from
> > Rust perspective, such might happen.
>
> Yes, that is what I meant. Sure the C side might never modify the
> value, but this is not good enough for Rust. It must somehow be ensured
> that it never is modified, in order for us to rely on it.
>
> --
> Cheers,
> Benno
>
>