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

FUJITA Tomonori
2023-11-21T02:13:07Z

On Sun, 19 Nov 2023 05:06:23 -0600
Trevor Gross <tmgross@umich.edu> wrote:

>> +pub struct Registration {
>> + drivers: Pin<&'static mut [DriverVTable]>,
>> +}
>>
>> [...]
>>
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Send for Registration {}
>>
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Sync for Registration {}
>
> I don't think the impl here actually makes sense. `Registration` is a
> buffer of references to `DriverVTable`. That type isn't marked Sync so
> by the above rules, its references should not be either.
>
> Tomo, does this need to be Sync at all? Probably easiest to drop the
> impls if not, otherwise I think it is more correct to move them to
> `DriverVTable`. You may have had this before, I'm not sure if
> discussion made you change it at some point...

This needs to be Sync:

601 | pub struct Registration {
| ^^^^^^^^^^^^
note: required because it appears within the type `Module`
--> drivers/net/phy/foo_rust.rs:5:1
|
5 | / kernel::module_phy_driver! {
6 | | drivers: [Foo],
7 | | device_table: [
8 | | DeviceId::new_with_driver::<Foo>()
... |
13 | | license: "GPL",
14 | | }
| |_^
note: required by a bound in `kernel::Module`
--> /home/ubuntu/git/linux/rust/kernel/lib.rs:69:27
|
69 | pub trait Module: Sized + Sync {
| ^^^^ required by this bound in `Module`
= note: this error originates in the macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)

I'm not sure we discussed but making DriverVTable Sync works.

#[repr(transparent)]
pub struct DriverVTable(Opaque<bindings::phy_driver>);

// SAFETY: DriverVTable has no &self methods, so immutable references to it are useless.
unsafe impl Sync for DriverVTable {}

looks correct?