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

[Danilo Krummrich <dakr@kernel.org>]

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".

> 
> 
> Subject: [PATCH] rust: device: Add the module name to the symbol name
> 
> Make the symbol name unique by adding the module name to avoid
> duplicate symbol errors like
> 
> ld.lld: error: duplicate symbol: __mod_of__OF_TABLE_device_table
> >>> defined at doctests_kernel_generated.ff18649a828ae8c4-cgu.0
> >>> rust/doctests_kernel_generated.o:(__mod_of__OF_TABLE_device_table) in
> archive vmlinux.a
> >>> defined at rust_driver_platform.2308c4225c4e08b3-cgu.0
> >>>            samples/rust/rust_driver_platform.o:(.rodata+0x5A8) in
> archive vmlinux.a
> make[2]: *** [scripts/Makefile.vmlinux_o:65: vmlinux.o] Error 1
> make[1]: *** [Makefile:1154: vmlinux_o] Error 2
> 
> __mod_of__OF_TABLE_device_table is too generic. Add the module name.
> 
> Proposed-by: Danilo Krummrich <dakr@kernel.org>
> Link: https://lore.kernel.org/rust-for-linux/Zx9lFG1XKnC_WaG0@pollux/
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  rust/kernel/device_id.rs             | 4 ++--
>  rust/kernel/of.rs                    | 4 ++--
>  rust/kernel/pci.rs                   | 5 +++--
>  rust/kernel/platform.rs              | 1 +
>  samples/rust/rust_driver_pci.rs      | 1 +
>  samples/rust/rust_driver_platform.rs | 1 +
>  6 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> index 5b1329fba528..231f34362da9 100644
> --- a/rust/kernel/device_id.rs
> +++ b/rust/kernel/device_id.rs
> @@ -151,10 +151,10 @@ fn info(&self, index: usize) -> &U {
>  /// Create device table alias for modpost.
>  #[macro_export]
>  macro_rules! module_device_table {
> -    ($table_type: literal, $module_table_name:ident, $table_name:ident) =>
> {
> +    ($table_type: literal, $device_name: 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, "__", stringify!($table_name),
> "_", $device_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()) };
> diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> index a37629997974..77679c30638c 100644
> --- a/rust/kernel/of.rs
> +++ b/rust/kernel/of.rs
> @@ -51,13 +51,13 @@ pub fn compatible<'a>(&self) -> &'a CStr {
>  /// Create an OF `IdTable` with an "alias" for modpost.
>  #[macro_export]
>  macro_rules! of_device_table {
> -    ($table_name:ident, $module_table_name:ident, $id_info_type: ty,
> $table_data: expr) => {
> +    ($device_name: literal, $table_name:ident, $module_table_name:ident,
> $id_info_type: ty, $table_data: expr) => {
>          const $table_name: $crate::device_id::IdArray<
>              $crate::of::DeviceId,
>              $id_info_type,
>              { $table_data.len() },
>          > = $crate::device_id::IdArray::new($table_data);
> 
> -        $crate::module_device_table!("of", $module_table_name,
> $table_name);
> +        $crate::module_device_table!("of", $device_name,
> $module_table_name, $table_name);
>      };
>  }
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 58f7d9c0045b..806d192b9600 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -176,14 +176,14 @@ fn index(&self) -> usize {
>  /// Create a PCI `IdTable` with its alias for modpost.
>  #[macro_export]
>  macro_rules! pci_device_table {
> -    ($table_name:ident, $module_table_name:ident, $id_info_type: ty,
> $table_data: expr) => {
> +    ($device_name: literal, $table_name:ident, $module_table_name:ident,
> $id_info_type: ty, $table_data: expr) => {
>          const $table_name: $crate::device_id::IdArray<
>              $crate::pci::DeviceId,
>              $id_info_type,
>              { $table_data.len() },
>          > = $crate::device_id::IdArray::new($table_data);
> 
> -        $crate::module_device_table!("pci", $module_table_name,
> $table_name);
> +        $crate::module_device_table!("pci", $device_name,
> $module_table_name, $table_name);
>      };
>  }
> 
> @@ -197,6 +197,7 @@ macro_rules! pci_device_table {
>  /// struct MyDriver;
>  ///
>  /// kernel::pci_device_table!(
> +///     "MyDriver",
>  ///     PCI_TABLE,
>  ///     MODULE_PCI_TABLE,
>  ///     <MyDriver as pci::Driver>::IdInfo,
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index a926233a789f..fcdd3c5da0e5 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -118,6 +118,7 @@ macro_rules! module_platform_driver {
>  /// struct MyDriver;
>  ///
>  /// kernel::of_device_table!(
> +///     "MyDriver",
>  ///     OF_TABLE,
>  ///     MODULE_OF_TABLE,
>  ///     <MyDriver as platform::Driver>::IdInfo,
> diff --git a/samples/rust/rust_driver_pci.rs
> b/samples/rust/rust_driver_pci.rs
> index d24dc1fde9e8..6ee570b59233 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -31,6 +31,7 @@ struct SampleDriver {
>  }
> 
>  kernel::pci_device_table!(
> +    "SampleDriver",
>      PCI_TABLE,
>      MODULE_PCI_TABLE,
>      <SampleDriver as pci::Driver>::IdInfo,
> diff --git a/samples/rust/rust_driver_platform.rs
> b/samples/rust/rust_driver_platform.rs
> index fd7a5ad669fe..9dfbe3b9932b 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -11,6 +11,7 @@ struct SampleDriver {
>  struct Info(u32);
> 
>  kernel::of_device_table!(
> +    "SampleDriver",
>      OF_TABLE,
>      MODULE_OF_TABLE,
>      <SampleDriver as platform::Driver>::IdInfo,
> -- 
> 2.46.2
>