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

Andrew Lunn
2023-11-17T16:28:12Z

> >> /// # Invariants
> >> ///
> >> /// Referencing a `phy_device` using this struct asserts that the X
> >> /// mutex is held, or that the reference has exclusive access to the
> >> /// entire `phy_device`.
> >> #[repr(transparent)]
> >> pub struct Device(Opaque<bindings::phy_device>);
> >
> > You can never have exclusive access to the entire phy_device, because
> > it contains a mutex. Other threads can block on that mutex, which
> > involves changing the linked list in the mutex.
> >
> > But that is also a pretty common pattern, put the mutex inside the
> > structure it protects. So when you say 'exclusive access to the entire
> > `phy_device`' you actually mean excluding mutex, spinlocks, atomic
> > variables, etc?
>
> No, I really meant exclusive access to everything. This suggestion is
> where I guessed that the situation might be "we just created the
> phy_device, and haven't yet shared it with anyone, so it's okay to
> access it without the lock". But it sounds like that's not the case.

It is pretty unusual for a linux driver to actually create a
device. Some level of core code generally creates a basic device
structure and passes it to the probe function. The probe can then
setup members in the device, maybe allocate memory and assign it to
the device->priv member etc.

However, in the probe method, it should be safe to assume its not
globally visible yet, so you can be more relaxed about locking.

> >> /// # Invariants
> >> ///
> >> /// Referencing a `phy_device` using this struct asserts that the user
> >> /// is inside a Y scope as defined in Documentation/foo/bar.
> >> #[repr(transparent)]
> >> pub struct Device(Opaque<bindings::phy_device>);
> >
> > There is no such documentation that i know of, except it does get
> > repeated again and again on the mailling lists. Its tribal knowledge.
>
> Then, my suggestion would be to write down that tribal knowledge in the
> safety comments.

O.K, we can do that.

Andrew