public inbox for rust-for-linux@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rust: ACPI: fix missing match data for PRP0001
@ 2026-04-01 14:06 Markus Probst
  2026-04-01 18:32 ` Greg Kroah-Hartman
  2026-04-04 22:07 ` Danilo Krummrich
  0 siblings, 2 replies; 12+ messages in thread
From: Markus Probst @ 2026-04-01 14:06 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman
  Cc: linux-kernel, linux-acpi, rust-for-linux, driver-core,
	Markus Probst

Export `acpi_of_match_device` function and use it to match the of device
table against ACPI PRP0001 in Rust.

This fixes id_info being None on ACPI PRP0001 devices.

Using `device_get_match_data` is not possible, because Rust stores an
index in the of device id instead of a data pointer.

Fixes: 7a718a1f26d1 ("rust: driver: implement `Adapter`")
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS            |  1 +
 drivers/acpi/bus.c     |  7 +++---
 include/linux/acpi.h   | 11 ++++++++++
 rust/helpers/acpi.c    |  8 +++++++
 rust/helpers/helpers.c |  1 +
 rust/kernel/driver.rs  | 58 ++++++++++++++++++++++++++++++++++++++------------
 6 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index c3fe46d7c4bc..3a7b3b5f2a28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -302,6 +302,7 @@ F:	include/linux/acpi.h
 F:	include/linux/fwnode.h
 F:	include/linux/fw_table.h
 F:	lib/fw_table.c
+F:	rust/helpers/acpi.c
 F:	rust/kernel/acpi.rs
 F:	tools/power/acpi/
 
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 2ec095e2009e..cd02f04cf685 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -831,9 +831,9 @@ const struct acpi_device *acpi_companion_match(const struct device *dev)
  * identifiers and a _DSD object with the "compatible" property, use that
  * property to match against the given list of identifiers.
  */
-static bool acpi_of_match_device(const struct acpi_device *adev,
-				 const struct of_device_id *of_match_table,
-				 const struct of_device_id **of_id)
+bool acpi_of_match_device(const struct acpi_device *adev,
+			  const struct of_device_id *of_match_table,
+			  const struct of_device_id **of_id)
 {
 	const union acpi_object *of_compatible, *obj;
 	int i, nval;
@@ -866,6 +866,7 @@ static bool acpi_of_match_device(const struct acpi_device *adev,
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(acpi_of_match_device);
 
 static bool acpi_of_modalias(struct acpi_device *adev,
 			     char *modalias, size_t len)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 4d2f0bed7a06..1cf23edcbfbb 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -736,6 +736,10 @@ const struct acpi_device_id *acpi_match_acpi_device(const struct acpi_device_id
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev);
 
+bool acpi_of_match_device(const struct acpi_device *adev,
+			  const struct of_device_id *of_match_table,
+			  const struct of_device_id **of_id);
+
 const void *acpi_device_get_match_data(const struct device *dev);
 extern bool acpi_driver_match_device(struct device *dev,
 				     const struct device_driver *drv);
@@ -965,6 +969,13 @@ static inline const struct acpi_device_id *acpi_match_device(
 	return NULL;
 }
 
+static inline bool acpi_of_match_device(const struct acpi_device *adev,
+					const struct of_device_id *of_match_table,
+					const struct of_device_id **of_id)
+{
+	return false;
+}
+
 static inline const void *acpi_device_get_match_data(const struct device *dev)
 {
 	return NULL;
diff --git a/rust/helpers/acpi.c b/rust/helpers/acpi.c
new file mode 100644
index 000000000000..f2aa00ec99c2
--- /dev/null
+++ b/rust/helpers/acpi.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/acpi.h>
+
+__rust_helper struct acpi_device *rust_helper_to_acpi_device_node(struct fwnode_handle *fwnode)
+{
+	return to_acpi_device_node(fwnode);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index a3c42e51f00a..8ecd1580aa69 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -9,6 +9,7 @@
 
 #define __rust_helper
 
+#include "acpi.c"
 #include "atomic.c"
 #include "atomic_ext.c"
 #include "auxiliary.c"
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index 36de8098754d..f03421bb2046 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -96,13 +96,18 @@
 
 use crate::{
     acpi,
-    device,
+    device::{
+        self,
+        property::FwNode, //
+    },
     of,
     prelude::*,
     types::Opaque,
     ThisModule, //
 };
 
+use core::ptr;
+
 /// Trait describing the layout of a specific device driver.
 ///
 /// This trait describes the layout of a specific driver structure, such as `struct pci_driver` or
@@ -329,35 +334,60 @@ fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
     ///
     /// If this returns `None`, it means there is no match with an entry in the [`of::IdTable`].
     fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
-        #[cfg(not(CONFIG_OF))]
+        let table = Self::of_id_table()?;
+
+        #[cfg(not(any(CONFIG_OF, CONFIG_ACPI)))]
         {
-            let _ = dev;
-            None
+            let _ = (dev, table);
         }
 
         #[cfg(CONFIG_OF)]
         {
-            let table = Self::of_id_table()?;
-
             // SAFETY:
             // - `table` has static lifetime, hence it's valid for read,
             // - `dev` is guaranteed to be valid while it's alive, and so is `dev.as_raw()`.
             let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), dev.as_raw()) };
 
-            if raw_id.is_null() {
-                None
-            } else {
+            if !raw_id.is_null() {
                 // SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id`
                 // and does not add additional invariants, so it's safe to transmute.
                 let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
 
-                Some(
-                    table.info(<of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(
-                        id,
-                    )),
-                )
+                return Some(table.info(
+                    <of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id),
+                ));
+            }
+        }
+
+        #[cfg(CONFIG_ACPI)]
+        {
+            let mut raw_id = ptr::null();
+
+            // SAFETY: `dev.fwnode().as_raw()` is a pointer to a valid `fwnode_handle`. A null
+            // pointer will be passed through the function.
+            let adev = unsafe {
+                bindings::to_acpi_device_node(dev.fwnode().map_or(ptr::null_mut(), FwNode::as_raw))
+            };
+
+            // SAFETY:
+            // - `adev` is a valid pointer to `acpi_device` or is null. It is guaranteed to be
+            //   valid as long as `dev` is alive.
+            // - `table` has static lifetime, hence it's valid for read.
+            if unsafe { bindings::acpi_of_match_device(adev, table.as_ptr(), &raw mut raw_id) } {
+                // SAFETY:
+                // - the function returns true, therefore `raw_id` has been set to a pointer to a
+                //   valid `of_device_id`.
+                // - `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id`
+                //   and does not add additional invariants, so it's safe to transmute.
+                let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
+
+                return Some(table.info(
+                    <of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id),
+                ));
             }
         }
+
+        None
     }
 
     /// Returns the driver's private data from the matching entry of any of the ID tables, if any.

---
base-commit: b6c7d19951061de1be08fb8f5714e630b24bcc1c
change-id: 20260401-rust_acpi_prp0001-a2971543b555


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 14:06 [PATCH] rust: ACPI: fix missing match data for PRP0001 Markus Probst
@ 2026-04-01 18:32 ` Greg Kroah-Hartman
  2026-04-01 18:46   ` Markus Probst
  2026-04-04 22:07 ` Danilo Krummrich
  1 sibling, 1 reply; 12+ messages in thread
From: Greg Kroah-Hartman @ 2026-04-01 18:32 UTC (permalink / raw)
  To: Markus Probst
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, linux-acpi,
	rust-for-linux, driver-core

On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
> Export `acpi_of_match_device` function and use it to match the of device
> table against ACPI PRP0001 in Rust.
> 
> This fixes id_info being None on ACPI PRP0001 devices.
> 
> Using `device_get_match_data` is not possible, because Rust stores an
> index in the of device id instead of a data pointer.

I'm confused, why are we open-coding this in the rust layer?  What do we
need to change in the C side to make both layers be able to call the
same function instead?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 18:32 ` Greg Kroah-Hartman
@ 2026-04-01 18:46   ` Markus Probst
  2026-04-01 22:15     ` Danilo Krummrich
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Probst @ 2026-04-01 18:46 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, linux-acpi,
	rust-for-linux, driver-core

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Wed, 2026-04-01 at 20:32 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
> > Export `acpi_of_match_device` function and use it to match the of device
> > table against ACPI PRP0001 in Rust.
> > 
> > This fixes id_info being None on ACPI PRP0001 devices.
> > 
> > Using `device_get_match_data` is not possible, because Rust stores an
> > index in the of device id instead of a data pointer.
> 
> I'm confused, why are we open-coding this in the rust layer?  What do we
> need to change in the C side to make both layers be able to call the
> same function instead?
No commit message I have seen has explained why it was done this way. I
don't think we would need to change anything on the C side.

Thanks
- Markus Probst

> 
> thanks,
> 
> greg k-h

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 18:46   ` Markus Probst
@ 2026-04-01 22:15     ` Danilo Krummrich
  2026-04-02 12:31       ` Greg Kroah-Hartman
  2026-04-04 21:23       ` Gary Guo
  0 siblings, 2 replies; 12+ messages in thread
From: Danilo Krummrich @ 2026-04-01 22:15 UTC (permalink / raw)
  To: Markus Probst, Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, linux-acpi, rust-for-linux,
	driver-core

On Wed Apr 1, 2026 at 8:46 PM CEST, Markus Probst wrote:
> On Wed, 2026-04-01 at 20:32 +0200, Greg Kroah-Hartman wrote:
>> On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
>> > Export `acpi_of_match_device` function and use it to match the of device
>> > table against ACPI PRP0001 in Rust.
>> > 
>> > This fixes id_info being None on ACPI PRP0001 devices.
>> > 
>> > Using `device_get_match_data` is not possible, because Rust stores an
>> > index in the of device id instead of a data pointer.
>> 
>> I'm confused, why are we open-coding this in the rust layer?  What do we
>> need to change in the C side to make both layers be able to call the
>> same function instead?
> No commit message I have seen has explained why it was done this way. I
> don't think we would need to change anything on the C side.

The Rust code stores an index into the array the contains the actual device ID
info in the driver_data field of a device ID instead of a raw pointer to the
device ID info.

The reason for this is that it was the only way to build this in a way that
results in an API that is convinient and obvious to use for drivers when
declaring the device ID table, can be evaluated in const context (i.e. at
compile time), and does not rely on unstable language features. Fulfilling all
three of those requirements at the same was a rather tricky one.

The unfortunate consequence is that device_get_match_data() does not give us a
pointer to the actual device ID info, but it gives us the index of the device ID
info in the device ID table.

The problem is that this does not really help, because now we know the index,
but not which table it belongs to.

I.e. we wouldn't know whether to call

	Self::acpi_id_table().info(index)

or

	Self::of_id_table().info(index)

to obtain the actual device ID info.

So, unfortunately, I think we have to open code this for now.

But I think this is still a minor inconvinience for being able to fulfill the
requirements mentioned above.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 22:15     ` Danilo Krummrich
@ 2026-04-02 12:31       ` Greg Kroah-Hartman
  2026-04-04 21:23       ` Gary Guo
  1 sibling, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2026-04-02 12:31 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Markus Probst, Rafael J. Wysocki, Len Brown, Miguel Ojeda,
	Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	linux-acpi, rust-for-linux, driver-core

On Thu, Apr 02, 2026 at 12:15:07AM +0200, Danilo Krummrich wrote:
> On Wed Apr 1, 2026 at 8:46 PM CEST, Markus Probst wrote:
> > On Wed, 2026-04-01 at 20:32 +0200, Greg Kroah-Hartman wrote:
> >> On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
> >> > Export `acpi_of_match_device` function and use it to match the of device
> >> > table against ACPI PRP0001 in Rust.
> >> > 
> >> > This fixes id_info being None on ACPI PRP0001 devices.
> >> > 
> >> > Using `device_get_match_data` is not possible, because Rust stores an
> >> > index in the of device id instead of a data pointer.
> >> 
> >> I'm confused, why are we open-coding this in the rust layer?  What do we
> >> need to change in the C side to make both layers be able to call the
> >> same function instead?
> > No commit message I have seen has explained why it was done this way. I
> > don't think we would need to change anything on the C side.
> 
> The Rust code stores an index into the array the contains the actual device ID
> info in the driver_data field of a device ID instead of a raw pointer to the
> device ID info.
> 
> The reason for this is that it was the only way to build this in a way that
> results in an API that is convinient and obvious to use for drivers when
> declaring the device ID table, can be evaluated in const context (i.e. at
> compile time), and does not rely on unstable language features. Fulfilling all
> three of those requirements at the same was a rather tricky one.
> 
> The unfortunate consequence is that device_get_match_data() does not give us a
> pointer to the actual device ID info, but it gives us the index of the device ID
> info in the device ID table.
> 
> The problem is that this does not really help, because now we know the index,
> but not which table it belongs to.
> 
> I.e. we wouldn't know whether to call
> 
> 	Self::acpi_id_table().info(index)
> 
> or
> 
> 	Self::of_id_table().info(index)
> 
> to obtain the actual device ID info.
> 
> So, unfortunately, I think we have to open code this for now.
> 
> But I think this is still a minor inconvinience for being able to fulfill the
> requirements mentioned above.

Ok, that makes more sense, thanks for the detail.  Perhaps some of that
could go into the changelog :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 22:15     ` Danilo Krummrich
  2026-04-02 12:31       ` Greg Kroah-Hartman
@ 2026-04-04 21:23       ` Gary Guo
  2026-04-04 21:32         ` Danilo Krummrich
  2026-04-04 21:33         ` Markus Probst
  1 sibling, 2 replies; 12+ messages in thread
From: Gary Guo @ 2026-04-04 21:23 UTC (permalink / raw)
  To: Danilo Krummrich, Markus Probst, Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, linux-acpi, rust-for-linux,
	driver-core

On Wed Apr 1, 2026 at 11:15 PM BST, Danilo Krummrich wrote:
> On Wed Apr 1, 2026 at 8:46 PM CEST, Markus Probst wrote:
>> On Wed, 2026-04-01 at 20:32 +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
>>> > Export `acpi_of_match_device` function and use it to match the of device
>>> > table against ACPI PRP0001 in Rust.
>>> > 
>>> > This fixes id_info being None on ACPI PRP0001 devices.
>>> > 
>>> > Using `device_get_match_data` is not possible, because Rust stores an
>>> > index in the of device id instead of a data pointer.
>>> 
>>> I'm confused, why are we open-coding this in the rust layer?  What do we
>>> need to change in the C side to make both layers be able to call the
>>> same function instead?
>> No commit message I have seen has explained why it was done this way. I
>> don't think we would need to change anything on the C side.
>
> The Rust code stores an index into the array the contains the actual device ID
> info in the driver_data field of a device ID instead of a raw pointer to the
> device ID info.
>
> The reason for this is that it was the only way to build this in a way that
> results in an API that is convinient and obvious to use for drivers when
> declaring the device ID table, can be evaluated in const context (i.e. at
> compile time), and does not rely on unstable language features. Fulfilling all
> three of those requirements at the same was a rather tricky one.
>
> The unfortunate consequence is that device_get_match_data() does not give us a
> pointer to the actual device ID info, but it gives us the index of the device ID
> info in the device ID table.
>
> The problem is that this does not really help, because now we know the index,
> but not which table it belongs to.
>
> I.e. we wouldn't know whether to call
>
> 	Self::acpi_id_table().info(index)
>
> or
>
> 	Self::of_id_table().info(index)
>
> to obtain the actual device ID info.
>
> So, unfortunately, I think we have to open code this for now.
>
> But I think this is still a minor inconvinience for being able to fulfill the
> requirements mentioned above.

I think there might be a chance that we can use const_refs_to_static to actually
put pointer there. Of course, doing so is probably still quite tricky with
feature support given all const generics hackery that we're doing :)

I might have a go when I have time.

BTW, if most drivers use driver_data of ID as pointers, why is it defined as
kernel_ulong_t instead of just `void*`?

Best,
Gary

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-04 21:23       ` Gary Guo
@ 2026-04-04 21:32         ` Danilo Krummrich
  2026-04-05  5:47           ` Greg Kroah-Hartman
  2026-04-04 21:33         ` Markus Probst
  1 sibling, 1 reply; 12+ messages in thread
From: Danilo Krummrich @ 2026-04-04 21:32 UTC (permalink / raw)
  To: Gary Guo
  Cc: Markus Probst, Greg Kroah-Hartman, Rafael J. Wysocki, Len Brown,
	Miguel Ojeda, Boqun Feng, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	linux-acpi, rust-for-linux, driver-core

On Sat Apr 4, 2026 at 11:23 PM CEST, Gary Guo wrote:
> BTW, if most drivers use driver_data of ID as pointers, why is it defined as
> kernel_ulong_t instead of just `void*`?

I think that's because the ID tables are exported to userspace via
scripts/mod/file2alias.c. If it would be void *, then there could be a mismatch
in size when cross compiling.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-04 21:23       ` Gary Guo
  2026-04-04 21:32         ` Danilo Krummrich
@ 2026-04-04 21:33         ` Markus Probst
  2026-04-04 21:38           ` Danilo Krummrich
  2026-04-04 22:08           ` Gary Guo
  1 sibling, 2 replies; 12+ messages in thread
From: Markus Probst @ 2026-04-04 21:33 UTC (permalink / raw)
  To: Gary Guo, Danilo Krummrich, Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, linux-acpi, rust-for-linux,
	driver-core

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]

On Sat, 2026-04-04 at 22:23 +0100, Gary Guo wrote:
> On Wed Apr 1, 2026 at 11:15 PM BST, Danilo Krummrich wrote:
> > On Wed Apr 1, 2026 at 8:46 PM CEST, Markus Probst wrote:
> > > On Wed, 2026-04-01 at 20:32 +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Apr 01, 2026 at 02:06:25PM +0000, Markus Probst wrote:
> > > > > Export `acpi_of_match_device` function and use it to match the of device
> > > > > table against ACPI PRP0001 in Rust.
> > > > > 
> > > > > This fixes id_info being None on ACPI PRP0001 devices.
> > > > > 
> > > > > Using `device_get_match_data` is not possible, because Rust stores an
> > > > > index in the of device id instead of a data pointer.
> > > > 
> > > > I'm confused, why are we open-coding this in the rust layer?  What do we
> > > > need to change in the C side to make both layers be able to call the
> > > > same function instead?
> > > No commit message I have seen has explained why it was done this way. I
> > > don't think we would need to change anything on the C side.
> > 
> > The Rust code stores an index into the array the contains the actual device ID
> > info in the driver_data field of a device ID instead of a raw pointer to the
> > device ID info.
> > 
> > The reason for this is that it was the only way to build this in a way that
> > results in an API that is convinient and obvious to use for drivers when
> > declaring the device ID table, can be evaluated in const context (i.e. at
> > compile time), and does not rely on unstable language features. Fulfilling all
> > three of those requirements at the same was a rather tricky one.
> > 
> > The unfortunate consequence is that device_get_match_data() does not give us a
> > pointer to the actual device ID info, but it gives us the index of the device ID
> > info in the device ID table.
> > 
> > The problem is that this does not really help, because now we know the index,
> > but not which table it belongs to.
> > 
> > I.e. we wouldn't know whether to call
> > 
> > 	Self::acpi_id_table().info(index)
> > 
> > or
> > 
> > 	Self::of_id_table().info(index)
> > 
> > to obtain the actual device ID info.
> > 
> > So, unfortunately, I think we have to open code this for now.
> > 
> > But I think this is still a minor inconvinience for being able to fulfill the
> > requirements mentioned above.
> 
> I think there might be a chance that we can use const_refs_to_static to actually
> put pointer there. Of course, doing so is probably still quite tricky with
> feature support given all const generics hackery that we're doing :)
> 
> I might have a go when I have time.
What does this mean for this patch series?

Will it be merged in the meantime or should we wait for your rewrite?
> 
> BTW, if most drivers use driver_data of ID as pointers, why is it defined as
> kernel_ulong_t instead of just `void*`?
The return type of `device_get_match_data` is `const void *`.
`of_device_id->data` has type `const void *`.
`acpi_device_id->driver_data` and `pci_device_id->driver_data` has type
kernel_ulong_t.

Kernel doc of pci_device_id:
"
Data private to the driver.
Most drivers don't need to use driver_data field.
Best practice is to use driver_data as an index
into a static list of equivalent device types,
instead of using it as a pointer.
"
suggests it was intended for use as index in the first place.

Thanks
- Markus Probst

> 
> Best,
> Gary

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-04 21:33         ` Markus Probst
@ 2026-04-04 21:38           ` Danilo Krummrich
  2026-04-04 22:08           ` Gary Guo
  1 sibling, 0 replies; 12+ messages in thread
From: Danilo Krummrich @ 2026-04-04 21:38 UTC (permalink / raw)
  To: Markus Probst
  Cc: Gary Guo, Greg Kroah-Hartman, Rafael J. Wysocki, Len Brown,
	Miguel Ojeda, Boqun Feng, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	linux-acpi, rust-for-linux, driver-core

On Sat Apr 4, 2026 at 11:33 PM CEST, Markus Probst wrote:
> What does this mean for this patch series?
>
> Will it be merged in the meantime or should we wait for your rewrite?

It will be merged, but you may want to adjust the changelog as asked by Greg
first.

Thanks,
Danilo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-01 14:06 [PATCH] rust: ACPI: fix missing match data for PRP0001 Markus Probst
  2026-04-01 18:32 ` Greg Kroah-Hartman
@ 2026-04-04 22:07 ` Danilo Krummrich
  1 sibling, 0 replies; 12+ messages in thread
From: Danilo Krummrich @ 2026-04-04 22:07 UTC (permalink / raw)
  To: Markus Probst
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, linux-kernel, linux-acpi,
	rust-for-linux, driver-core

On Wed Apr 1, 2026 at 4:06 PM CEST, Markus Probst wrote:
> @@ -866,6 +866,7 @@ static bool acpi_of_match_device(const struct acpi_device *adev,
>  
>  	return false;
>  }
> +EXPORT_SYMBOL_GPL(acpi_of_match_device);

We only use this from Rust core code, so I'd rather avoid exporting this symbol.

We can work around the compiler inlining this into modules with this indirection
instead:

+#[inline(never)]
+#[allow(clippy::missing_safety_doc)]
+#[must_use]
+unsafe fn acpi_of_match_device(
+    adev: *const bindings::acpi_device,
+    of_match_table: *const bindings::of_device_id,
+    of_id: *mut *const bindings::of_device_id,
+) -> bool {
+    // SAFETY: Safety requirements are the same as `bindings::acpi_device_id`.
+    unsafe { bindings::acpi_of_match_device(adev, of_match_table, of_id) }
+}
+
 /// The bus independent adapter to match a drivers and a devices.
 ///
 /// This trait should be implemented by the bus specific adapter, which represents the connection
@@ -373,7 +385,7 @@ fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
             // - `adev` is a valid pointer to `acpi_device` or is null. It is guaranteed to be
             //   valid as long as `dev` is alive.
             // - `table` has static lifetime, hence it's valid for read.
-            if unsafe { bindings::acpi_of_match_device(adev, table.as_ptr(), &raw mut raw_id) } {
+            if unsafe { acpi_of_match_device(adev, table.as_ptr(), &raw mut raw_id) } {
                 // SAFETY:
                 // - the function returns true, therefore `raw_id` has been set to a pointer to a
                 //   valid `of_device_id`.

Please see also [1].

[1] https://patch.msgid.link/20260213220718.82835-6-dakr@kernel.org

> @@ -329,35 +334,60 @@ fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
>      ///
>      /// If this returns `None`, it means there is no match with an entry in the [`of::IdTable`].
>      fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> -        #[cfg(not(CONFIG_OF))]
> +        let table = Self::of_id_table()?;
> +
> +        #[cfg(not(any(CONFIG_OF, CONFIG_ACPI)))]
>          {
> -            let _ = dev;
> -            None
> +            let _ = (dev, table);
>          }
>  
>          #[cfg(CONFIG_OF)]
>          {
> -            let table = Self::of_id_table()?;
> -
>              // SAFETY:
>              // - `table` has static lifetime, hence it's valid for read,
>              // - `dev` is guaranteed to be valid while it's alive, and so is `dev.as_raw()`.
>              let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), dev.as_raw()) };
>  
> -            if raw_id.is_null() {
> -                None
> -            } else {
> +            if !raw_id.is_null() {
>                  // SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id`
>                  // and does not add additional invariants, so it's safe to transmute.
>                  let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
>  
> -                Some(
> -                    table.info(<of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(
> -                        id,
> -                    )),
> -                )
> +                return Some(table.info(
> +                    <of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id),
> +                ));
> +            }
> +        }
> +
> +        #[cfg(CONFIG_ACPI)]
> +        {
> +            let mut raw_id = ptr::null();
> +
> +            // SAFETY: `dev.fwnode().as_raw()` is a pointer to a valid `fwnode_handle`. A null
> +            // pointer will be passed through the function.
> +            let adev = unsafe {
> +                bindings::to_acpi_device_node(dev.fwnode().map_or(ptr::null_mut(), FwNode::as_raw))
> +            };

Please only pass the final pointer to bindings::to_acpi_device_node() to keep
unsafe {} sections as short as possible.

> +
> +            // SAFETY:
> +            // - `adev` is a valid pointer to `acpi_device` or is null. It is guaranteed to be
> +            //   valid as long as `dev` is alive.
> +            // - `table` has static lifetime, hence it's valid for read.
> +            if unsafe { bindings::acpi_of_match_device(adev, table.as_ptr(), &raw mut raw_id) } {
> +                // SAFETY:
> +                // - the function returns true, therefore `raw_id` has been set to a pointer to a
> +                //   valid `of_device_id`.
> +                // - `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id`
> +                //   and does not add additional invariants, so it's safe to transmute.
> +                let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
> +
> +                return Some(table.info(
> +                    <of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id),
> +                ));
>              }
>          }
> +
> +        None
>      }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-04 21:33         ` Markus Probst
  2026-04-04 21:38           ` Danilo Krummrich
@ 2026-04-04 22:08           ` Gary Guo
  1 sibling, 0 replies; 12+ messages in thread
From: Gary Guo @ 2026-04-04 22:08 UTC (permalink / raw)
  To: Markus Probst, Gary Guo, Danilo Krummrich, Greg Kroah-Hartman
  Cc: Rafael J. Wysocki, Len Brown, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, linux-acpi, rust-for-linux,
	driver-core

On Sat Apr 4, 2026 at 10:33 PM BST, Markus Probst wrote:
> On Sat, 2026-04-04 at 22:23 +0100, Gary Guo wrote:
>> BTW, if most drivers use driver_data of ID as pointers, why is it defined as
>> kernel_ulong_t instead of just `void*`?
>
> The return type of `device_get_match_data` is `const void *`.
> `of_device_id->data` has type `const void *`.
> `acpi_device_id->driver_data` and `pci_device_id->driver_data` has type
> kernel_ulong_t.
>
> Kernel doc of pci_device_id:
> "
> Data private to the driver.
> Most drivers don't need to use driver_data field.
> Best practice is to use driver_data as an index
> into a static list of equivalent device types,
> instead of using it as a pointer.
> "
> suggests it was intended for use as index in the first place.
>
> Thanks
> - Markus Probst

I would have believed it if I haven't seen a huge number of drivers just using
pointers and do a cast :)
of_device_id/dmi_system_id is also just using void* for its driver_data.

I mean, blaming through git history it looks like this documentation predates
git, so I'm not really sure how relevant this still is.

Best,
Gary

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] rust: ACPI: fix missing match data for PRP0001
  2026-04-04 21:32         ` Danilo Krummrich
@ 2026-04-05  5:47           ` Greg Kroah-Hartman
  0 siblings, 0 replies; 12+ messages in thread
From: Greg Kroah-Hartman @ 2026-04-05  5:47 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Gary Guo, Markus Probst, Rafael J. Wysocki, Len Brown,
	Miguel Ojeda, Boqun Feng, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, linux-kernel,
	linux-acpi, rust-for-linux, driver-core

On Sat, Apr 04, 2026 at 11:32:44PM +0200, Danilo Krummrich wrote:
> On Sat Apr 4, 2026 at 11:23 PM CEST, Gary Guo wrote:
> > BTW, if most drivers use driver_data of ID as pointers, why is it defined as
> > kernel_ulong_t instead of just `void*`?
> 
> I think that's because the ID tables are exported to userspace via
> scripts/mod/file2alias.c. If it would be void *, then there could be a mismatch
> in size when cross compiling.
> 

That is correct, and is why it is defined this way.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-04-05  5:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-01 14:06 [PATCH] rust: ACPI: fix missing match data for PRP0001 Markus Probst
2026-04-01 18:32 ` Greg Kroah-Hartman
2026-04-01 18:46   ` Markus Probst
2026-04-01 22:15     ` Danilo Krummrich
2026-04-02 12:31       ` Greg Kroah-Hartman
2026-04-04 21:23       ` Gary Guo
2026-04-04 21:32         ` Danilo Krummrich
2026-04-05  5:47           ` Greg Kroah-Hartman
2026-04-04 21:33         ` Markus Probst
2026-04-04 21:38           ` Danilo Krummrich
2026-04-04 22:08           ` Gary Guo
2026-04-04 22:07 ` Danilo Krummrich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox