linux-kernel - Re: [PATCH v3 15/16] rust: platform: add basic platform device / driver abstractions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6769b70c-19d9-4e20-85a6-96b55c83a45b@de.bosch.com>
Date: Tue, 29 Oct 2024 11:08:37 +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 10:55, Danilo Krummrich wrote:
> On Tue, Oct 29, 2024 at 10:50:11AM +0100, Danilo Krummrich wrote:
>> On Tue, Oct 29, 2024 at 10:19:08AM +0100, Dirk Behme wrote:
>>> 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)?
>>
>> No, you can still have as many as you want for the same file, you just have to
>> give them different identifier names -- you can't have two statics with the same
>> name in one file anyways.
>>
>> Well, I guess you somehow can (just like the doctests do), but it does make
>> sense to declare drivers in such a way.
>>
>> I think as long as we take care that separate Rust modules can't interfere with
>> each other it's good enough.
>>
>>>
>>> For example adding a second FooDriver example to platform.rs won't be
>>> possible?
>>
>> Not unless you change the identifier name unfortunately. But that might be
>> fixable by putting doctests in separate `mod $(DOCTEST) {}` blocks.
> 
> Another option would be to not only encode the module path, but also the line
> number:
> 
> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> index 5b1329fba528..7956edbbad52 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!(), "_", line!(), "_", 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()) };
> 
> This way you'll get
> 
>    "__mod_of__doctests_kernel_generated_3875_OF_TABLE_device_table"
>    "__mod_of__doctests_kernel_generated_3946_OF_TABLE_device_table"


Yes, if we want to avoid adding a unique name that makes sense and is a 
good "automatic" option :)

Thanks

Dirk