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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqL674Mf5_QcWxEn-oStMx45_VsaQfbN-Qzh3jEtXdsyYA@mail.gmail.com>
Date: Wed, 30 Oct 2024 08:18:43 -0500
From: Rob Herring <robh@...nel.org>
To: Dirk Behme <dirk.behme@...bosch.com>
Cc: Danilo Krummrich <dakr@...nel.org>, 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, 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 Tue, Oct 29, 2024 at 4:19 AM Dirk Behme <dirk.behme@...bosch.com> 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)?

It's generally one per module, but it's one per type because it is one
type per driver. So platform (and most other) drivers can have $bus,
DT, and ACPI tables.

While you could have 1 module with N drivers, I don't think I've ever
seen that case and certainly not something we'd encourage. Perhaps it
is just not possible to disallow in C, but we can in rust? That may be
a benefit, not a limitation.

Rob