[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d8b23faa-4041-4789-ae96-5d8bf87070ad@proton.me>
Date: Sat, 21 Oct 2023 12:13:32 +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, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers
On 21.10.23 13:36, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 11:21:12 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
>
>> On 21.10.23 12:27, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 08:37:08 +0000
>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>
>>>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>>>> On Sat, 21 Oct 2023 07:25:17 +0000
>>>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>>>
>>>>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>>>>> FUJITA Tomonori <fujita.tomonori@...il.com> wrote:
>>>>>>>
>>>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>>>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>>>>>>
>>>>>>>>> I would like to remove the mutable static variable and simplify
>>>>>>>>> the macro.
>>>>>>>>
>>>>>>>> How about adding DriverVTable array to Registration?
>>>>>>>>
>>>>>>>> /// Registration structure for a PHY driver.
>>>>>>>> ///
>>>>>>>> /// # Invariants
>>>>>>>> ///
>>>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>>>> pub struct Registration<const N: usize> {
>>>>>>>> drivers: [DriverVTable; N],
>>>>>>>> }
>>>>>>>>
>>>>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>>>> /// Registers a PHY driver.
>>>>>>>> pub fn register(
>>>>>>>> module: &'static crate::ThisModule,
>>>>>>>> drivers: [DriverVTable; N],
>>>>>>>> ) -> Result<Self> {
>>>>>>>> let mut reg = Registration { drivers };
>>>>>>>> let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>>>> // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>>>> // are initialized properly. So an FFI call with a valid pointer.
>>>>>>>> to_result(unsafe {
>>>>>>>> bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>>>> })?;
>>>>>>>> // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>>>> Ok(reg)
>>>>>>>> }
>>>>>>>> }
>>>>>>>
>>>>>>> Scratch this.
>>>>>>>
>>>>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>>>>> Module strcut doesn't work.
>>>>>>
>>>>>> Why does it not work? I tried it and it compiled fine for me.
>>>>>
>>>>> You can compile but the kernel crashes. The addresses of the callback
>>>>> functions are invalid.
>>>>
>>>> Can you please share your setup and the error? For me it booted
>>>> fine.
>>>
>>> You use ASIX PHY hardware?
>>
>> It seems I have configured something wrong. Can you share your testing
>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>> real hardware with a real ASIX PHY device?
>
> real hardware with real ASIX PHY device.
I see.
> Qemu supports a virtual PHY device?
I have no idea.
[...]
>> I think this is very weird, do you have any idea why this
>> could happen?
>
> DriverVtable is created on kernel stack, I guess.
But how does that invalidate the function pointers?
>> If you don't mind, could you try if the following changes
>> anything?
>
> I don't think it works. If you use const for DriverTable, DriverTable
> is placed on read-only pages. The C side modifies DriverVTable array
> so it does't work.
Did you try it? Note that I copy the `DriverVTable` into the Module
struct, so it will not be placed on a read-only page.
>> (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>> const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>> struct Module {
>> _drivers: [::kernel::net::phy::DriverVTable; N],
>> }
>>
>> $crate::prelude::module! {
>> type: Module,
>> $($f)*
>> }
>>
>> unsafe impl Sync for Module {}
>>
>> impl ::kernel::Module for Module {
>> fn init(module: &'static ThisModule) -> Result<Self> {
>> const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>> let mut m = Module {
>> _drivers: unsafe { core::ptr::read(&DRIVERS) },
>> };
>> let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>> ::kernel::error::to_result(unsafe {
>> kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>> })?;
>> Ok(m)
>> }
>> }
>>
>> and also the variation where you replace `const DRIVERS` with
>> `static DRIVERS`.
>
> Probably works. But looks like similar with the current code? This is
> simpler?
Just curious if it has to do with using `static` vs `const`.
--
Cheers,
Benno
Powered by blists - more mailing lists