Message-ID: <69144cf1-6c07-868c-9577-e41db4c0cc75@proton.me>
Date: Sun, 25 Jun 2023 17:06:59 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, aliceryhl@...gle.com, andrew@...n.ch, miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH 1/5] rust: core abstractions for network device drivers

On 25.06.23 16:27, FUJITA Tomonori wrote:
> Hi,
>
> On Sun, 25 Jun 2023 09:52:53 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
>
>>>>> +/// Trait for device driver specific information.
>>>>> +///
>>>>> +/// This data structure is passed to a driver with the operations for `struct net_device`
>>>>> +/// like `struct net_device_ops`, `struct ethtool_ops`, `struct rtnl_link_ops`, etc.
>>>>> +pub trait DriverData {
>>>>> +    /// The object are stored in C object, `struct net_device`.
>>>>> +    type Data: ForeignOwnable + Send + Sync;
>>>>
>>>> Why is this an associated type? Could you not use
>>>> `D: ForeignOwnable + Send + Sync` everywhere instead?
>>>> I think this should be possible, since `DriverData` does not define
>>>> anything else.
>>>
>>> With that approach, is it possible to allow a device driver to define
>>> own data structure and functions taking the structure as aurgument
>>> (like DevOps structutre in the 5th patch)
>>>
>>
>> In the example both structs are empty so I am not really sure why it has
>> to be two types. Can't we do this:
>> ```
>> pub struct MyDriver {
>>       // Just some random fields...
>>       pub access_count: Cell<usize>,
>> }
>>
>>
>> impl DriverData for Box<MyDriver> {}
>>
>> // And then we could make `DeviceOperations: DriverData`.
>> // Users would then do this:
>>
>> #[vtable]
>> impl DeviceOperations for Box<MyDriver> {
>>       fn init(_dev: Device, data: &MyDriver) -> Result {
>>           data.access_count.set(0);
>>           Ok(())
>>       }
>>
>>       fn open(_dev: Device, data: &MyDriver) -> Result {
>>           data.access_count.set(data.access_count.get() + 1);
>>           Ok(())
>>       }
>> }
>> ```
>>
>> I think this would simplify things, because you do not have to carry the
>> extra associated type around (and have to spell out
>> `<D::Data as ForeignOwnable>` everywhere).
>
> I'm still not sure if I correctly understand what you try to do.
>
> If I define DeviceOperations in dev.rs like the following:
>
> #[vtable]
> pub trait DeviceOperations<D: ForeignOwnable + Send + Sync> {
>      /// Corresponds to `ndo_init` in `struct net_device_ops`.
>          fn init(_dev: &mut Device, _data: D::Borrowed<'_>) -> Result {
> 	        Ok(())
>          }
> }
>
> And the driver implmeents DeviceOperations like the folloing:
>
> #[vtable]
> impl<D: ForeignOwnable + Send + Sync> DeviceOperations<D> for Box<DriverData> {
>      fn init(_dev: &mut Device, _data: &DriverData) -> Result {
>              Ok(())
>      }
> }
>
> I got the following error:
>
> error[E0053]: method `init` has an incompatible type for trait
>    --> samples/rust/rust_net_dummy.rs:24:39
>     |
> 24 |     fn init(_dev: &mut Device, _data: &DriverData) -> Result {
>     |                                       ^^^^^^^^^^^
>     |                                       |
>     |                                       expected associated type, found `&DriverData`
>     |                                       help: change the parameter type to match the trait: `<D as ForeignOwnable>::Borrowed<'_>`
>     |
>     = note: expected signature `fn(&mut Device, <D as ForeignOwnable>::Borrowed<'_>) -> core::result::Result<_, _>`
>                found signature `fn(&mut Device, &DriverData) -> core::result::Result<_, _>`
>

I thought you could do this:
```
#[vtable]
pub trait DeviceOperations: ForeignOwnable + Send + Sync {
     /// Corresponds to `ndo_init` in `struct net_device_ops`.
     fn init(_dev: &mut Device, _data: Self::Borrowed<'_>) -> Result {
         Ok(())
     }
}

#[vtable]
impl DeviceOperations<D> for Box<DriverData> {
     fn init(_dev: &mut Device, _data: &DriverData) -> Result {
         Ok(())
     }
}
```

>>>>> +    const fn build_device_ops() -> &'static bindings::net_device_ops {
>>>>> +        &Self::DEVICE_OPS
>>>>> +    }
>>>>
>>>> Why does this function exist?
>>>
>>> To get const struct net_device_ops *netdev_ops.
>>
>> Can't you just use `&Self::DEVICE_OPS`?
>
> I think that it didn't work in the past but seems that it works
> now. I'll fix.
>
>
>>>>> +/// Corresponds to the kernel's `struct net_device_ops`.
>>>>> +///
>>>>> +/// A device driver must implement this. Only very basic operations are supported for now.
>>>>> +#[vtable]
>>>>> +pub trait DeviceOperations<D: DriverData> {
>>>>
>>>> Why is this trait generic over `D`? Why is this not `Self` or an associated
>>>> type?
>>>
>>> DriverData also used in EtherOperationsAdapter (the second patch) and
>>> there are other operations that uses DriverData (not in this patchset).
>>
>> Could you point me to those other things that use `DriverData`?
>
> net_device struct has some like tlsdev_ops, rtnl_link_ops.. A device
> driver might need to access to the private data via net_device in
> these operations.

In my mental model you can just implement the `TLSOperations` trait
alongside the `DeviceOperations` trait. But I would have to see the
API that will be used for that to be sure. I do not think that you need
to have different private data for each of those operations traits.

--
Cheers,
Benno

>
>
> thanks,

Powered by blists - more mailing lists