[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d72e37e-9e27-4857-b0eb-0b1e98cc5610@de.bosch.com>
Date: Tue, 29 Oct 2024 10:19:08 +0100
From: Dirk Behme <dirk.behme@...bosch.com>
To: Danilo Krummrich <dakr@...nel.org>
CC: <gregkh@...uxfoundation.org>, <rafael@...nel.org>, <bhelgaas@...gle.com>,
<ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
<gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <benno.lossin@...ton.me>,
<tmgross@...ch.edu>, <a.hindborg@...sung.com>, <aliceryhl@...gle.com>,
<airlied@...il.com>, <fujita.tomonori@...il.com>, <lina@...hilina.net>,
<pstanner@...hat.com>, <ajanulgu@...hat.com>, <lyude@...hat.com>,
<robh@...nel.org>, <daniel.almeida@...labora.com>, <saravanak@...gle.com>,
<rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 15/16] rust: platform: add basic platform device /
driver abstractions
On 29.10.2024 09:50, Danilo Krummrich wrote:
> On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote:
>> On 28.10.2024 11:19, Danilo Krummrich wrote:
>>> On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote:
>>>>> +/// IdTable type for platform drivers.
>>>>> +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
>>>>> +
>>>>> +/// The platform driver trait.
>>>>> +///
>>>>> +/// # Example
>>>>> +///
>>>>> +///```
>>>>> +/// # use kernel::{bindings, c_str, of, platform};
>>>>> +///
>>>>> +/// struct MyDriver;
>>>>> +///
>>>>> +/// kernel::of_device_table!(
>>>>> +/// OF_TABLE,
>>>>> +/// MODULE_OF_TABLE,
>>>>
>>>> It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names
>>>> used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER
>>>> and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other
>>>> examples/samples in this patch series. Found that while using the *same*
>>>> somewhere else ;)
>>>
>>> I think the names by themselves are fine. They're local to the module. However,
>>> we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e.
>>> "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols.
>>>
>>> I think we somehow need to build the module name into the symbol name as well.
>>
>> Something like this?
>
> No, I think we should just encode the Rust module name / path, which should make
> this a unique symbol name.
>
> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> index 5b1329fba528..63e81ec2d6fd 100644
> --- a/rust/kernel/device_id.rs
> +++ b/rust/kernel/device_id.rs
> @@ -154,7 +154,7 @@ macro_rules! module_device_table {
> ($table_type: literal, $module_table_name:ident, $table_name:ident) => {
> #[rustfmt::skip]
> #[export_name =
> - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table")
> + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table")
> ]
> static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] =
> unsafe { core::mem::transmute_copy($table_name.raw_ids()) };
>
> For the doctests for instance this
>
> "__mod_of__OF_TABLE_device_table"
>
> becomes
>
> "__mod_of__doctests_kernel_generated_OF_TABLE_device_table".
What implies *one* OF/PCI_TABLE per path (file)?
For example adding a second FooDriver example to platform.rs won't be
possible?
+/// struct FooDriver;
+///
+/// kernel::of_device_table!(
+/// OF_TABLE,
+/// MODULE_OF_TABLE,
+/// <FooDriver as platform::Driver>::IdInfo,
+/// [
+/// (of::DeviceId::new(c_str!("test,rust-device2")), ())
+/// ]
+/// );
Best regards
Dirk
Powered by blists - more mailing lists