Re: [PATCH v3 14/16] rust: of: add `of::DeviceId` abstraction

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Oct 22, 2024 at 06:03:51PM -0500, Rob Herring wrote:
> On Tue, Oct 22, 2024 at 11:31:51PM +0200, Danilo Krummrich wrote:
> > `of::DeviceId` is an abstraction around `struct of_device_id`.
> > 
> > This is used by subsequent patches, in particular the platform bus
> > abstractions, to create OF device ID tables.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  MAINTAINERS                     |  1 +
> >  rust/bindings/bindings_helper.h |  1 +
> >  rust/kernel/lib.rs              |  1 +
> >  rust/kernel/of.rs               | 63 +++++++++++++++++++++++++++++++++
> >  4 files changed, 66 insertions(+)
> >  create mode 100644 rust/kernel/of.rs
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d9c512a3e72b..87eb9a7869eb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17340,6 +17340,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> >  F:	Documentation/ABI/testing/sysfs-firmware-ofw
> >  F:	drivers/of/
> >  F:	include/linux/of*.h
> > +F:	rust/kernel/of.rs
> >  F:	scripts/dtc/
> >  F:	tools/testing/selftests/dt/
> >  K:	of_overlay_notifier_
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index cd4edd6496ae..312f03cbdce9 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -15,6 +15,7 @@
> >  #include <linux/firmware.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/mdio.h>
> > +#include <linux/of_device.h>
> 
> Technically, you don't need this for *this* patch. You need 
> mod_devicetable.h for of_device_id. Best to not rely on implicit 
> includes. I've tried removing it and it still built, so I guess there is 
> another implicit include somewhere...

True, however mod_devicetable.h is already needed for a previous patch "rust:
pci: add basic PCI device / driver abstractions" already. So, I'll add it there
and remove the of_device.h include here.

> 
> >  #include <linux/pci.h>
> >  #include <linux/phy.h>
> >  #include <linux/refcount.h>
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 3ec690eb6d43..5946f59f1688 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -51,6 +51,7 @@
> >  pub mod list;
> >  #[cfg(CONFIG_NET)]
> >  pub mod net;
> > +pub mod of;
> >  pub mod page;
> >  pub mod prelude;
> >  pub mod print;
> > diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> > new file mode 100644
> > index 000000000000..a37629997974
> > --- /dev/null
> > +++ b/rust/kernel/of.rs
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Open Firmware abstractions.
> 
> s/Open Firmware/Devicetree/
> 
> Or keep both that prior versions of this code had. Most of DT/OF today 
> is not OpenFirmware.
> 
> > +//!
> > +//! C header: [`include/linux/of_*.h`](srctree/include/linux/of_*.h)
> 
> I haven't quite figured out how this gets used. I guess just a link in 
> documentation? I somewhat doubt this file is going to handle all DT
> abstractions. That might become quite long. Most of of_address.h and 
> of_irq.h I actively don't want to see Rust bindings for because they 
> are mainly used by higher level interfaces (e.g. platform dev 
> resources). There's a slew of "don't add new users" APIs which I need to 
> document. Also, the main header is of.h which wasn't included here.

I think for now it's indeed meaningless and we should just remove it.

If subsequent patches start adding abstractions for things like properties,
device nodes, etc. they can add it back in.

> 
> As of now, only the mod_devicetable.h header is used by this file, so I 
> think you should just put it until that changes. Maybe there would be 
> some savings if all of mod_devicetable.h was handled by 1 rust file?

AFAIK, in C we have all those device ID structs in mod_devicetable.h, such that
in can easily be included in scripts/mod/file2alias.c. But I think implementing
all Rust abstractions for those in a single file would be a bit odd. I'd rather
put them together with the corresponding bus abstractions. OF and ACPI may be a
bit of an exception.

> 
> > +
> > +use crate::{bindings, device_id::RawDeviceId, prelude::*};
> > +
> > +/// An open firmware device id.
> > +#[derive(Clone, Copy)]
> > +pub struct DeviceId(bindings::of_device_id);
> > +
> > +// SAFETY:
> > +// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and does not add
> > +//   additional invariants, so it's safe to transmute to `RawType`.
> > +// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
> > +unsafe impl RawDeviceId for DeviceId {
> > +    type RawType = bindings::of_device_id;
> > +
> > +    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
> > +
> > +    fn index(&self) -> usize {
> > +        self.0.data as _
> > +    }
> > +}
> > +
> > +impl DeviceId {
> > +    /// Create a new device id from an OF 'compatible' string.
> > +    pub const fn new(compatible: &'static CStr) -> Self {
> > +        let src = compatible.as_bytes_with_nul();
> > +        // Replace with `bindings::of_device_id::default()` once stabilized for `const`.
> > +        // SAFETY: FFI type is valid to be zero-initialized.
> > +        let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
> > +
> > +        let mut i = 0;
> > +        while i < src.len() {
> > +            of.compatible[i] = src[i] as _;
> > +            i += 1;
> > +        }
> 
> AFAICT, this loop will go away when C char maps to u8. Perhaps a note 
> to that extent or commented code implementing that.

That's true, I'll add a note.

> 
> > +
> > +        Self(of)
> > +    }
> > +
> > +    /// The compatible string of the embedded `struct bindings::of_device_id` as `&CStr`.
> > +    pub fn compatible<'a>(&self) -> &'a CStr {
> > +        // SAFETY: `self.compatible` is a valid `char` pointer.
> > +        unsafe { CStr::from_char_ptr(self.0.compatible.as_ptr()) }
> > +    }
> 
> I don't think we need this. The usage model is checking does a node's 
> compatible string(s) match a compatible in the table. Most of the time 
> we don't even need that. We just need the match data.

Right, I think I just added it for the sample driver, we can get rid of it.

> 
> Rob
>