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

[Danilo Krummrich <dakr@kernel.org>]

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"

if you put identical doctests.

> 
> > 
> > +/// 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
> > 
> > 
> > 
> >