* [PATCH RFC 0/3] Initial rust bindings for device property reads
@ 2024-10-25 21:05 Rob Herring (Arm)
2024-10-25 21:05 ` [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample Rob Herring (Arm)
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-10-25 21:05 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
I got asked about upstreaming DT rust bindings, so I decided to take a
stab at it. This series adds rust bindings for the device property API
which is a firmware agnostic interface for reading firmware properties.
There are "DT rust bindings" patches floating around, but for many
drivers they don't need to know any DT specifics nor deal with struct
device_node (and its refcounts). And reading firmware properties are
simple enough for my feeble rust abilities.
This series is based on top of Danilo's PCI and platform device
series[1], though that's really only for the sample driver.
Please tell me how my rust code sucks before I spend more time beating
my head against the wall^W^W^W^W^W^W learning rust.
Rob
[1] https://lore.kernel.org/all/20241022213221.2383-1-dakr@kernel.org/
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Rob Herring (Arm) (3):
of: unittest: Add a platform device node for rust platform driver sample
rust: Add bindings for device properties
samples: rust: platform: Add property read examples
drivers/of/unittest-data/tests-platform.dtsi | 8 ++
rust/bindings/bindings_helper.h | 1 +
rust/kernel/device.rs | 145 ++++++++++++++++++++++++++-
samples/rust/rust_driver_platform.rs | 24 ++++-
4 files changed, 176 insertions(+), 2 deletions(-)
---
base-commit: eeb31b3e7e9e1b485763ecc66ece8afba1416b2a
change-id: 20241025-rust-platform-dev-0e89debcbba5
Best regards,
--
Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample
2024-10-25 21:05 [PATCH RFC 0/3] Initial rust bindings for device property reads Rob Herring (Arm)
@ 2024-10-25 21:05 ` Rob Herring (Arm)
2024-10-28 7:11 ` Dirk Behme
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-10-25 21:05 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
drivers/of/unittest-data/tests-platform.dtsi | 5 +++++
samples/rust/rust_driver_platform.rs | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index fa39611071b3..2caaf1c10ee6 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -33,6 +33,11 @@ dev@100 {
reg = <0x100>;
};
};
+
+ test-device@2 {
+ compatible = "test,rust-device";
+ reg = <0x2>;
+ };
};
};
};
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 55caaaa4f216..5cf4a8f86c13 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -15,7 +15,7 @@ struct SampleDriver {
MODULE_OF_TABLE,
<SampleDriver as platform::Driver>::IdInfo,
[(
- of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")),
+ of::DeviceId::new(c_str!("test,rust-device")),
Info(42)
)]
);
--
2.45.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:05 [PATCH RFC 0/3] Initial rust bindings for device property reads Rob Herring (Arm)
2024-10-25 21:05 ` [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample Rob Herring (Arm)
@ 2024-10-25 21:05 ` Rob Herring (Arm)
2024-10-25 21:12 ` Alex Gaynor
` (2 more replies)
2024-10-25 21:05 ` [PATCH RFC 3/3] samples: rust: platform: Add property read examples Rob Herring (Arm)
2024-10-28 7:11 ` [PATCH RFC 0/3] Initial rust bindings for device property reads Dirk Behme
3 siblings, 3 replies; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-10-25 21:05 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
The device property API is a firmware agnostic API for reading
properties from firmware (DT/ACPI) devices nodes and swnodes.
While the C API takes a pointer to a caller allocated variable/buffer,
the rust API is designed to return a value and can be used in struct
initialization. Rust generics are also utilized to support different
sizes of properties (e.g. u8, u16, u32).
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Not sure if we need the KVec variant, but I kept it as that was my first
pass attempt. Most callers are filling in some value in a driver data
struct. Sometimes the number of elements is not known, so the caller
calls to get the array size, allocs the correct size buffer, and then
reads the property again to fill in the buffer.
I have not implemented a wrapper for device_property_read_string(_array)
because that API is problematic for dynamic DT nodes. The API just
returns pointer(s) into the raw DT data. We probably need to return a
copy of the string(s) instead for rust.
After property accessors, next up is child node accessors/iterators.
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 145 insertions(+), 1 deletion(-)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 217c776615b9..65717cc20a23 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -19,6 +19,7 @@
#include <linux/pci.h>
#include <linux/phy.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/refcount.h>
#include <linux/sched.h>
#include <linux/slab.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 0c28b1e6b004..bb66a28df890 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -5,10 +5,14 @@
//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
use crate::{
+ alloc::KVec,
bindings,
+ error::{to_result, Result},
+ prelude::*,
+ str::CStr,
types::{ARef, Opaque},
};
-use core::{fmt, ptr};
+use core::{fmt, mem::size_of, ptr};
#[cfg(CONFIG_PRINTK)]
use crate::c_str;
@@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
)
};
}
+
+ /// Returns if a firmware property `name` is true or false
+ pub fn property_read_bool(&self, name: &CStr) -> bool {
+ unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
+ }
+
+ /// Returns if a firmware string property `name` has match for `match_str`
+ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+ let ret = unsafe {
+ bindings::device_property_match_string(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ match_str.as_ptr() as *const i8,
+ )
+ };
+ to_result(ret)?;
+ Ok(ret as usize)
+ }
+
+ /// Returns firmware property `name` scalar value
+ ///
+ /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
+ pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
+ let mut val: [T; 1] = unsafe { core::mem::zeroed() };
+
+ Self::_property_read_array(&self, name, &mut val)?;
+ Ok(val[0])
+ }
+
+ /// Returns firmware property `name` array values
+ ///
+ /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
+ pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
+ let mut val: [T; N] = unsafe { core::mem::zeroed() };
+
+ Self::_property_read_array(self, name, &mut val)?;
+ Ok(val)
+ }
+
+ fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
+ match size_of::<T>() {
+ 1 => to_result(unsafe {
+ bindings::device_property_read_u8_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ val.as_ptr() as *mut u8,
+ val.len(),
+ )
+ })?,
+ 2 => to_result(unsafe {
+ bindings::device_property_read_u16_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ val.as_ptr() as *mut u16,
+ val.len(),
+ )
+ })?,
+ 4 => to_result(unsafe {
+ bindings::device_property_read_u32_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ val.as_ptr() as *mut u32,
+ val.len(),
+ )
+ })?,
+ 8 => to_result(unsafe {
+ bindings::device_property_read_u64_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ val.as_ptr() as *mut u64,
+ val.len(),
+ )
+ })?,
+ _ => return Err(EINVAL),
+ }
+ Ok(())
+ }
+
+ pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+ let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
+
+ // SAFETY: len always matches capacity
+ unsafe { val.set_len(len) }
+ Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
+ Ok(val)
+ }
+
+ /// Returns array length for firmware property `name`
+ ///
+ /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
+ pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
+ let ret;
+
+ match size_of::<T>() {
+ 1 => {
+ ret = unsafe {
+ bindings::device_property_read_u8_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ ptr::null_mut(),
+ 0,
+ )
+ }
+ }
+ 2 => {
+ ret = unsafe {
+ bindings::device_property_read_u16_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ ptr::null_mut(),
+ 0,
+ )
+ }
+ }
+ 4 => {
+ ret = unsafe {
+ bindings::device_property_read_u32_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ ptr::null_mut(),
+ 0,
+ )
+ }
+ }
+ 8 => {
+ ret = unsafe {
+ bindings::device_property_read_u64_array(
+ self.as_raw(),
+ name.as_ptr() as *const i8,
+ ptr::null_mut(),
+ 0,
+ )
+ }
+ }
+ _ => return Err(EINVAL),
+ }
+ to_result(ret)?;
+ Ok(ret.try_into().unwrap())
+ }
}
// SAFETY: Instances of `Device` are always reference-counted.
--
2.45.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH RFC 3/3] samples: rust: platform: Add property read examples
2024-10-25 21:05 [PATCH RFC 0/3] Initial rust bindings for device property reads Rob Herring (Arm)
2024-10-25 21:05 ` [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample Rob Herring (Arm)
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
@ 2024-10-25 21:05 ` Rob Herring (Arm)
2024-10-28 7:13 ` Dirk Behme
2024-10-28 7:11 ` [PATCH RFC 0/3] Initial rust bindings for device property reads Dirk Behme
3 siblings, 1 reply; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-10-25 21:05 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
Add some example usage of the device property read methods for
DT/ACPI/swnode properties.
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
drivers/of/unittest-data/tests-platform.dtsi | 3 +++
samples/rust/rust_driver_platform.rs | 22 ++++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index 2caaf1c10ee6..a5369b9343b8 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -37,6 +37,9 @@ dev@100 {
test-device@2 {
compatible = "test,rust-device";
reg = <0x2>;
+
+ test,u32-prop = <0xdeadbeef>;
+ test,i16-array = /bits/ 16 <1 2 (-3) (-4)>;
};
};
};
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 5cf4a8f86c13..95c290806862 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -41,6 +41,28 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
}
};
+ let dev = pdev.as_ref();
+ if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+ {
+ dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
+ }
+
+ let prop = dev.property_read_bool(c_str!("test,bool-prop"));
+ dev_info!(dev, "bool prop is {}\n", prop);
+
+ let _prop = dev.property_read::<u32>(c_str!("test,u32-prop"))?;
+ let prop: u32 = dev.property_read(c_str!("test,u32-prop"))?;
+ dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+
+ let prop: [i16; 4] = dev.property_read_array(c_str!("test,i16-array"))?;
+ dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
+ dev_info!(
+ dev,
+ "'test,i16-array' length is {}\n",
+ dev.property_count_elem::<u16>(c_str!("test,i16-array"))
+ .unwrap()
+ );
+
let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
Ok(drvdata.into())
--
2.45.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
@ 2024-10-25 21:12 ` Alex Gaynor
2024-10-27 22:07 ` Rob Herring
2024-10-28 22:24 ` Rob Herring
2024-10-28 7:12 ` Dirk Behme
2024-10-29 14:16 ` Alice Ryhl
2 siblings, 2 replies; 28+ messages in thread
From: Alex Gaynor @ 2024-10-25 21:12 UTC (permalink / raw)
To: Rob Herring (Arm)
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dirk Behme, devicetree, linux-kernel,
rust-for-linux
On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Not sure if we need the KVec variant, but I kept it as that was my first
> pass attempt. Most callers are filling in some value in a driver data
> struct. Sometimes the number of elements is not known, so the caller
> calls to get the array size, allocs the correct size buffer, and then
> reads the property again to fill in the buffer.
>
> I have not implemented a wrapper for device_property_read_string(_array)
> because that API is problematic for dynamic DT nodes. The API just
> returns pointer(s) into the raw DT data. We probably need to return a
> copy of the string(s) instead for rust.
>
> After property accessors, next up is child node accessors/iterators.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 145 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9..65717cc20a23 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -19,6 +19,7 @@
> #include <linux/pci.h>
> #include <linux/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 0c28b1e6b004..bb66a28df890 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,10 +5,14 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> + alloc::KVec,
> bindings,
> + error::{to_result, Result},
> + prelude::*,
> + str::CStr,
> types::{ARef, Opaque},
> };
> -use core::{fmt, ptr};
> +use core::{fmt, mem::size_of, ptr};
>
> #[cfg(CONFIG_PRINTK)]
> use crate::c_str;
> @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> )
> };
> }
> +
> + /// Returns if a firmware property `name` is true or false
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> + }
> +
> + /// Returns if a firmware string property `name` has match for `match_str`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + let ret = unsafe {
> + bindings::device_property_match_string(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + match_str.as_ptr() as *const i8,
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` scalar value
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(&self, name, &mut val)?;
> + Ok(val[0])
> + }
> +
This, and several of the other methods are unsound, because they can
be used to construct arbitrary types for which may not allow all bit
patterns. You can use:
https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
bound to ensure only valid types are used.
Also, instead of using mem::zeroed(), you should use MaybeUnininit
(https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
which allows you to avoid needing to zero initialize.
> + /// Returns firmware property `name` array values
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(self, name, &mut val)?;
> + Ok(val)
> + }
> +
> + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
> + match size_of::<T>() {
> + 1 => to_result(unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u8,
> + val.len(),
> + )
> + })?,
> + 2 => to_result(unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u16,
> + val.len(),
> + )
> + })?,
> + 4 => to_result(unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u32,
> + val.len(),
> + )
> + })?,
> + 8 => to_result(unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u64,
> + val.len(),
> + )
> + })?,
> + _ => return Err(EINVAL),
> + }
> + Ok(())
> + }
> +
> + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: len always matches capacity
> + unsafe { val.set_len(len) }
> + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> + Ok(val)
> + }
> +
> + /// Returns array length for firmware property `name`
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> + let ret;
> +
> + match size_of::<T>() {
> + 1 => {
> + ret = unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 2 => {
> + ret = unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 4 => {
> + ret = unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 8 => {
> + ret = unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + _ => return Err(EINVAL),
> + }
> + to_result(ret)?;
> + Ok(ret.try_into().unwrap())
> + }
> }
>
> // SAFETY: Instances of `Device` are always reference-counted.
>
> --
> 2.45.2
>
--
All that is necessary for evil to succeed is for good people to do nothing.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:12 ` Alex Gaynor
@ 2024-10-27 22:07 ` Rob Herring
2024-10-28 22:24 ` Rob Herring
1 sibling, 0 replies; 28+ messages in thread
From: Rob Herring @ 2024-10-27 22:07 UTC (permalink / raw)
To: Alex Gaynor
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dirk Behme, devicetree, linux-kernel,
rust-for-linux
On Fri, Oct 25, 2024 at 4:12 PM Alex Gaynor <alex.gaynor@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> >
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > sizes of properties (e.g. u8, u16, u32).
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Not sure if we need the KVec variant, but I kept it as that was my first
> > pass attempt. Most callers are filling in some value in a driver data
> > struct. Sometimes the number of elements is not known, so the caller
> > calls to get the array size, allocs the correct size buffer, and then
> > reads the property again to fill in the buffer.
> >
> > I have not implemented a wrapper for device_property_read_string(_array)
> > because that API is problematic for dynamic DT nodes. The API just
> > returns pointer(s) into the raw DT data. We probably need to return a
> > copy of the string(s) instead for rust.
> >
> > After property accessors, next up is child node accessors/iterators.
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 217c776615b9..65717cc20a23 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -19,6 +19,7 @@
> > #include <linux/pci.h>
> > #include <linux/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/refcount.h>
> > #include <linux/sched.h>
> > #include <linux/slab.h>
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 0c28b1e6b004..bb66a28df890 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -5,10 +5,14 @@
> > //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> > use crate::{
> > + alloc::KVec,
> > bindings,
> > + error::{to_result, Result},
> > + prelude::*,
> > + str::CStr,
> > types::{ARef, Opaque},
> > };
> > -use core::{fmt, ptr};
> > +use core::{fmt, mem::size_of, ptr};
> >
> > #[cfg(CONFIG_PRINTK)]
> > use crate::c_str;
> > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> > )
> > };
> > }
> > +
> > + /// Returns if a firmware property `name` is true or false
> > + pub fn property_read_bool(&self, name: &CStr) -> bool {
> > + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > + }
> > +
> > + /// Returns if a firmware string property `name` has match for `match_str`
> > + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > + let ret = unsafe {
> > + bindings::device_property_match_string(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + match_str.as_ptr() as *const i8,
> > + )
> > + };
> > + to_result(ret)?;
> > + Ok(ret as usize)
> > + }
> > +
> > + /// Returns firmware property `name` scalar value
> > + ///
> > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > +
> > + Self::_property_read_array(&self, name, &mut val)?;
> > + Ok(val[0])
> > + }
> > +
>
> This, and several of the other methods are unsound, because they can
> be used to construct arbitrary types for which may not allow all bit
> patterns. You can use:
> https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
> bound to ensure only valid types are used.
I meant to ask a question on this. FromBytes is probably still too
permissive for what is supported in DT. I think floating point types
would be allowed which is not valid in DT (though asahi linux has some
downstream patches adding FP data support). What I really need is the
Integer trait from the num crate or some way to define the exact types
supported. I tried Into<u32>, etc., but that didn't work.
>
> Also, instead of using mem::zeroed(), you should use MaybeUnininit
> (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
> which allows you to avoid needing to zero initialize.
Okay, thanks. Being new to rust, I find this a bit complicated
compared to C (perhaps it has to be to avoid this class of bugs).
Maybe that's because this stuff seems to have been evolving some with
the kernel rust support. In any case, some examples of how (and
perhaps) to do uninitialized and zero-initialized allocations would be
helpful as both are common in the kernel.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 0/3] Initial rust bindings for device property reads
2024-10-25 21:05 [PATCH RFC 0/3] Initial rust bindings for device property reads Rob Herring (Arm)
` (2 preceding siblings ...)
2024-10-25 21:05 ` [PATCH RFC 3/3] samples: rust: platform: Add property read examples Rob Herring (Arm)
@ 2024-10-28 7:11 ` Dirk Behme
3 siblings, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-10-28 7:11 UTC (permalink / raw)
To: Rob Herring (Arm), Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
On 25.10.2024 23:05, Rob Herring (Arm) wrote:
> I got asked about upstreaming DT rust bindings, so I decided to take a
> stab at it.
Many thanks for this!
> This series adds rust bindings for the device property API
> which is a firmware agnostic interface for reading firmware properties.
> There are "DT rust bindings" patches floating around, but for many
> drivers they don't need to know any DT specifics nor deal with struct
> device_node (and its refcounts). And reading firmware properties are
> simple enough for my feeble rust abilities.
>
> This series is based on top of Danilo's PCI and platform device
> series[1], though that's really only for the sample driver.
On x86 having CONFIG_OF_UNITTESTS enabled I get
rust_driver_platform testcase-data:platform-tests:test-device@2: Probed
by OF compatible match: 'test,rust-device' with info: '42'.
rust_driver_platform testcase-data:platform-tests:test-device@2: matched
compatible string idx = 0
rust_driver_platform testcase-data:platform-tests:test-device@2: bool
prop is false
rust_driver_platform testcase-data:platform-tests:test-device@2:
'test,u32-prop' is 0xdeadbeef
rust_driver_platform testcase-data:platform-tests:test-device@2:
'test,i16-array' is [1, 2, -3, -4]
rust_driver_platform testcase-data:platform-tests:test-device@2:
'test,i16-array' length is 4
with this. Looks good :)
I'm not sure if it makes sense for an RFC patch series but in case it helps:
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Some comments will follow in the individual patches.
Best regards
Dirk
> Please tell me how my rust code sucks before I spend more time beating
> my head against the wall^W^W^W^W^W^W learning rust.
>
> Rob
>
> [1] https://lore.kernel.org/all/20241022213221.2383-1-dakr@kernel.org/
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Rob Herring (Arm) (3):
> of: unittest: Add a platform device node for rust platform driver sample
> rust: Add bindings for device properties
> samples: rust: platform: Add property read examples
>
> drivers/of/unittest-data/tests-platform.dtsi | 8 ++
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/device.rs | 145 ++++++++++++++++++++++++++-
> samples/rust/rust_driver_platform.rs | 24 ++++-
> 4 files changed, 176 insertions(+), 2 deletions(-)
> ---
> base-commit: eeb31b3e7e9e1b485763ecc66ece8afba1416b2a
> change-id: 20241025-rust-platform-dev-0e89debcbba5
>
> Best regards,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample
2024-10-25 21:05 ` [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample Rob Herring (Arm)
@ 2024-10-28 7:11 ` Dirk Behme
0 siblings, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-10-28 7:11 UTC (permalink / raw)
To: Rob Herring (Arm), Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
On 25.10.2024 23:05, Rob Herring (Arm) wrote:
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> drivers/of/unittest-data/tests-platform.dtsi | 5 +++++
> samples/rust/rust_driver_platform.rs | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index fa39611071b3..2caaf1c10ee6 100644
> --- a/drivers/of/unittest-data/tests-platform.dtsi
> +++ b/drivers/of/unittest-data/tests-platform.dtsi
> @@ -33,6 +33,11 @@ dev@100 {
> reg = <0x100>;
> };
> };
> +
> + test-device@2 {
> + compatible = "test,rust-device";
> + reg = <0x2>;
> + };
> };
> };
> };
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index 55caaaa4f216..5cf4a8f86c13 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -15,7 +15,7 @@ struct SampleDriver {
> MODULE_OF_TABLE,
> <SampleDriver as platform::Driver>::IdInfo,
> [(
> - of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")),
> + of::DeviceId::new(c_str!("test,rust-device")),
> Info(42)
> )]
> );
>
I'm not sure if parts or all of this should be squashed into Danilo's
patches. Anyway:
Reviewed-by: Dirk Behme <dirk.behme@de.bosch.com>
Best regards
Dirk
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
2024-10-25 21:12 ` Alex Gaynor
@ 2024-10-28 7:12 ` Dirk Behme
2024-10-29 14:16 ` Alice Ryhl
2 siblings, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-10-28 7:12 UTC (permalink / raw)
To: Rob Herring (Arm), Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
On 25.10.2024 23:05, Rob Herring (Arm) wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Building this on rust-next with CLIPPY=1 there are some improvement
requests:
* Function documentation for property_read_array_vec
* SAFETY comments for all unsafe{} blocks
* Different ret usage for match size_of::<T>()
* Use self instead of &self
See [1] below (without the SAFETY comments).
Best regards
Dirk
> ---
> Not sure if we need the KVec variant, but I kept it as that was my first
> pass attempt. Most callers are filling in some value in a driver data
> struct. Sometimes the number of elements is not known, so the caller
> calls to get the array size, allocs the correct size buffer, and then
> reads the property again to fill in the buffer.
>
> I have not implemented a wrapper for device_property_read_string(_array)
> because that API is problematic for dynamic DT nodes. The API just
> returns pointer(s) into the raw DT data. We probably need to return a
> copy of the string(s) instead for rust.
>
> After property accessors, next up is child node accessors/iterators.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 145 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9..65717cc20a23 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -19,6 +19,7 @@
> #include <linux/pci.h>
> #include <linux/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 0c28b1e6b004..bb66a28df890 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,10 +5,14 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> + alloc::KVec,
> bindings,
> + error::{to_result, Result},
> + prelude::*,
> + str::CStr,
> types::{ARef, Opaque},
> };
> -use core::{fmt, ptr};
> +use core::{fmt, mem::size_of, ptr};
>
> #[cfg(CONFIG_PRINTK)]
> use crate::c_str;
> @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> )
> };
> }
> +
> + /// Returns if a firmware property `name` is true or false
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> + }
> +
> + /// Returns if a firmware string property `name` has match for `match_str`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + let ret = unsafe {
> + bindings::device_property_match_string(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + match_str.as_ptr() as *const i8,
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` scalar value
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(&self, name, &mut val)?;
> + Ok(val[0])
> + }
> +
> + /// Returns firmware property `name` array values
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(self, name, &mut val)?;
> + Ok(val)
> + }
> +
> + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result > + match size_of::<T>() {
> + 1 => to_result(unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u8,
> + val.len(),
> + )
> + })?,
> + 2 => to_result(unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u16,
> + val.len(),
> + )
> + })?,
> + 4 => to_result(unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u32,
> + val.len(),
> + )
> + })?,
> + 8 => to_result(unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u64,
> + val.len(),
> + )
> + })?,
> + _ => return Err(EINVAL),
> + }
> + Ok(())
> + }
> +
> + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: len always matches capacity
> + unsafe { val.set_len(len) }
> + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> + Ok(val)
> + }
> +
> + /// Returns array length for firmware property `name`
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> + let ret;
> +
> + match size_of::<T>() {
> + 1 => {
> + ret = unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 2 => {
> + ret = unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 4 => {
> + ret = unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 8 => {
> + ret = unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + _ => return Err(EINVAL),
> + }
> + to_result(ret)?;
> + Ok(ret.try_into().unwrap())
> + }
> }
>
> // SAFETY: Instances of `Device` are always reference-counted.
[1]
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index bb66a28df890..3c461b1602d5 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -218,7 +218,7 @@ pub fn property_match_string(&self, name: &CStr,
match_str: &CStr) -> Result<usi
pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
let mut val: [T; 1] = unsafe { core::mem::zeroed() };
- Self::_property_read_array(&self, name, &mut val)?;
+ Self::_property_read_array(self, name, &mut val)?;
Ok(val[0])
}
@@ -271,12 +271,13 @@ fn _property_read_array<T>(&self, name: &CStr,
val: &mut [T]) -> Result {
Ok(())
}
+ /// ToDo
pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize)
-> Result<KVec<T>> {
let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
// SAFETY: len always matches capacity
unsafe { val.set_len(len) }
- Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
+ Self::_property_read_array::<T>(self, name, val.as_mut_slice())?;
Ok(val)
}
@@ -284,11 +285,10 @@ pub fn property_read_array_vec<T>(&self, name:
&CStr, len: usize) -> Result<KVec
///
/// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
- let ret;
- match size_of::<T>() {
+ let ret = match size_of::<T>() {
1 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u8_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -298,7 +298,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
2 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u16_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -308,7 +308,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
4 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u32_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -318,7 +318,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
8 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u64_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -328,7 +328,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
_ => return Err(EINVAL),
- }
+ };
to_result(ret)?;
Ok(ret.try_into().unwrap())
}
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 3/3] samples: rust: platform: Add property read examples
2024-10-25 21:05 ` [PATCH RFC 3/3] samples: rust: platform: Add property read examples Rob Herring (Arm)
@ 2024-10-28 7:13 ` Dirk Behme
0 siblings, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-10-28 7:13 UTC (permalink / raw)
To: Rob Herring (Arm), Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Dirk Behme
Cc: devicetree, linux-kernel, rust-for-linux
On 25.10.2024 23:05, Rob Herring (Arm) wrote:
> Add some example usage of the device property read methods for
> DT/ACPI/swnode properties.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
I think that we should mention that this works only with
CONFIG_OF_UNITTEST enabled? And/or maybe wrap the whole somehow with
#[cfg(CONFIG_OF_UNITTEST)]
?
> ---
> drivers/of/unittest-data/tests-platform.dtsi | 3 +++
> samples/rust/rust_driver_platform.rs | 22 ++++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index 2caaf1c10ee6..a5369b9343b8 100644
> --- a/drivers/of/unittest-data/tests-platform.dtsi
> +++ b/drivers/of/unittest-data/tests-platform.dtsi
> @@ -37,6 +37,9 @@ dev@100 {
> test-device@2 {
> compatible = "test,rust-device";
> reg = <0x2>;
> +
> + test,u32-prop = <0xdeadbeef>;
> + test,i16-array = /bits/ 16 <1 2 (-3) (-4)>;
> };
> };
> };
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index 5cf4a8f86c13..95c290806862 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -41,6 +41,28 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
> }
> };
>
> + let dev = pdev.as_ref();
Maybe move this up and use it in Danilo's part, as well? To stay consistent?
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -26,25 +26,25 @@ impl platform::Driver for SampleDriver {
fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>)
-> Result<Pin<KBox<Self>>> {
dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
+ let dev = pdev.as_ref();
match (Self::of_match_device(pdev), info) {
(Some(id), Some(info)) => {
dev_info!(
- pdev.as_ref(),
+ dev,
"Probed by OF compatible match: '{}' with info:
'{}'.\n",
id.compatible(),
info.0
);
}
_ => {
- dev_info!(pdev.as_ref(), "Probed by name.\n");
+ dev_info!(dev, "Probed by name.\n");
}
};
- let dev = pdev.as_ref();
if let Ok(idx) =
dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
{
- dev_info!(pdev.as_ref(), "matched compatible string idx =
{}\n", idx);
+ dev_info!(dev, "matched compatible string idx = {}\n", idx);
}
let prop = dev.property_read_bool(c_str!("test,bool-prop"));
@@ -56,12 +56,10 @@ fn probe(pdev: &mut platform::Device, info:
Option<&Self::IdInfo>) -> Result<Pin
> + if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> + {
> + dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
> + }
> +
> + let prop = dev.property_read_bool(c_str!("test,bool-prop"));
I stopped reading here with "hey, "test,bool-prop" isn't in the
tests-platform.dtsi above, no?". Until I realized that this is
intentional to get back false. So whats about adding a comment like
// Intentionally check for an non-existent property to get back false
?
> + dev_info!(dev, "bool prop is {}\n", prop);
> +
> + let _prop = dev.property_read::<u32>(c_str!("test,u32-prop"))?;
> + let prop: u32 = dev.property_read(c_str!("test,u32-prop"))?;
> + dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> + let prop: [i16; 4] = dev.property_read_array(c_str!("test,i16-array"))?;
> + dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
> + dev_info!(
> + dev,
> + "'test,i16-array' length is {}\n",
> + dev.property_count_elem::<u16>(c_str!("test,i16-array"))
> + .unwrap()
In the error case unwrap() (or expect()) will result in panic(). Besides
some very rare cases I don't see a reason why device drivers should
panic. Esp. not if reading some array length from the device tree fails
;) So I don't think we should encourage using unwrap() or expect() in
drivers by using them even in example code. Which most probably will be
copied quite often, then ;)
What's about anything like this instead?
let prop: [i16; 4] =
dev.property_read_array(c_str!("test,i16-array"))?;
dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
- dev_info!(
- dev,
- "'test,i16-array' length is {}\n",
- dev.property_count_elem::<u16>(c_str!("test,i16-array"))
- .unwrap()
- );
+ let len = dev.property_count_elem::<u16>(c_str!("test,i16-array"));
+ if let Ok(length) = len {
+ dev_info!(dev, "'test,i16-array' length is {}\n", length);
+ }
let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
Or if we want a reasonable error message in the error case, as well, we
could switch to a match, instead.
Best regards
Dirk
> + );
> +
> let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
>
> Ok(drvdata.into())
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:12 ` Alex Gaynor
2024-10-27 22:07 ` Rob Herring
@ 2024-10-28 22:24 ` Rob Herring
2024-10-29 2:08 ` Alex Gaynor
1 sibling, 1 reply; 28+ messages in thread
From: Rob Herring @ 2024-10-28 22:24 UTC (permalink / raw)
To: Alex Gaynor
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dirk Behme, devicetree, linux-kernel,
rust-for-linux
On Fri, Oct 25, 2024 at 4:12 PM Alex Gaynor <alex.gaynor@gmail.com> wrote:
>
> On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> >
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > sizes of properties (e.g. u8, u16, u32).
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Not sure if we need the KVec variant, but I kept it as that was my first
> > pass attempt. Most callers are filling in some value in a driver data
> > struct. Sometimes the number of elements is not known, so the caller
> > calls to get the array size, allocs the correct size buffer, and then
> > reads the property again to fill in the buffer.
> >
> > I have not implemented a wrapper for device_property_read_string(_array)
> > because that API is problematic for dynamic DT nodes. The API just
> > returns pointer(s) into the raw DT data. We probably need to return a
> > copy of the string(s) instead for rust.
> >
> > After property accessors, next up is child node accessors/iterators.
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 217c776615b9..65717cc20a23 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -19,6 +19,7 @@
> > #include <linux/pci.h>
> > #include <linux/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/refcount.h>
> > #include <linux/sched.h>
> > #include <linux/slab.h>
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 0c28b1e6b004..bb66a28df890 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -5,10 +5,14 @@
> > //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> > use crate::{
> > + alloc::KVec,
> > bindings,
> > + error::{to_result, Result},
> > + prelude::*,
> > + str::CStr,
> > types::{ARef, Opaque},
> > };
> > -use core::{fmt, ptr};
> > +use core::{fmt, mem::size_of, ptr};
> >
> > #[cfg(CONFIG_PRINTK)]
> > use crate::c_str;
> > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> > )
> > };
> > }
> > +
> > + /// Returns if a firmware property `name` is true or false
> > + pub fn property_read_bool(&self, name: &CStr) -> bool {
> > + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > + }
> > +
> > + /// Returns if a firmware string property `name` has match for `match_str`
> > + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > + let ret = unsafe {
> > + bindings::device_property_match_string(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + match_str.as_ptr() as *const i8,
> > + )
> > + };
> > + to_result(ret)?;
> > + Ok(ret as usize)
> > + }
> > +
> > + /// Returns firmware property `name` scalar value
> > + ///
> > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > +
> > + Self::_property_read_array(&self, name, &mut val)?;
> > + Ok(val[0])
> > + }
> > +
>
> This, and several of the other methods are unsound, because they can
> be used to construct arbitrary types for which may not allow all bit
> patterns. You can use:
> https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
> bound to ensure only valid types are used.
>
> Also, instead of using mem::zeroed(), you should use MaybeUnininit
> (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
> which allows you to avoid needing to zero initialize.
Something like this what you had in mind?:
pub fn property_read_array<T, const N: usize>(&self, name: &CStr) ->
Result<[T; N]> {
let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
Self::_property_read_array(self, name, &mut val)?;
// SAFETY: On success, _property_read_array has filled in the array
let val = unsafe { mem::transmute_copy(&val) };
Ok(val)
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-28 22:24 ` Rob Herring
@ 2024-10-29 2:08 ` Alex Gaynor
0 siblings, 0 replies; 28+ messages in thread
From: Alex Gaynor @ 2024-10-29 2:08 UTC (permalink / raw)
To: Rob Herring
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Dirk Behme, devicetree, linux-kernel,
rust-for-linux
On Mon, Oct 28, 2024 at 6:24 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 4:12 PM Alex Gaynor <alex.gaynor@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > >
> > > The device property API is a firmware agnostic API for reading
> > > properties from firmware (DT/ACPI) devices nodes and swnodes.
> > >
> > > While the C API takes a pointer to a caller allocated variable/buffer,
> > > the rust API is designed to return a value and can be used in struct
> > > initialization. Rust generics are also utilized to support different
> > > sizes of properties (e.g. u8, u16, u32).
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > > Not sure if we need the KVec variant, but I kept it as that was my first
> > > pass attempt. Most callers are filling in some value in a driver data
> > > struct. Sometimes the number of elements is not known, so the caller
> > > calls to get the array size, allocs the correct size buffer, and then
> > > reads the property again to fill in the buffer.
> > >
> > > I have not implemented a wrapper for device_property_read_string(_array)
> > > because that API is problematic for dynamic DT nodes. The API just
> > > returns pointer(s) into the raw DT data. We probably need to return a
> > > copy of the string(s) instead for rust.
> > >
> > > After property accessors, next up is child node accessors/iterators.
> > > ---
> > > rust/bindings/bindings_helper.h | 1 +
> > > rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 145 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > > index 217c776615b9..65717cc20a23 100644
> > > --- a/rust/bindings/bindings_helper.h
> > > +++ b/rust/bindings/bindings_helper.h
> > > @@ -19,6 +19,7 @@
> > > #include <linux/pci.h>
> > > #include <linux/phy.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > #include <linux/refcount.h>
> > > #include <linux/sched.h>
> > > #include <linux/slab.h>
> > > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > > index 0c28b1e6b004..bb66a28df890 100644
> > > --- a/rust/kernel/device.rs
> > > +++ b/rust/kernel/device.rs
> > > @@ -5,10 +5,14 @@
> > > //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> > >
> > > use crate::{
> > > + alloc::KVec,
> > > bindings,
> > > + error::{to_result, Result},
> > > + prelude::*,
> > > + str::CStr,
> > > types::{ARef, Opaque},
> > > };
> > > -use core::{fmt, ptr};
> > > +use core::{fmt, mem::size_of, ptr};
> > >
> > > #[cfg(CONFIG_PRINTK)]
> > > use crate::c_str;
> > > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> > > )
> > > };
> > > }
> > > +
> > > + /// Returns if a firmware property `name` is true or false
> > > + pub fn property_read_bool(&self, name: &CStr) -> bool {
> > > + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > > + }
> > > +
> > > + /// Returns if a firmware string property `name` has match for `match_str`
> > > + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > > + let ret = unsafe {
> > > + bindings::device_property_match_string(
> > > + self.as_raw(),
> > > + name.as_ptr() as *const i8,
> > > + match_str.as_ptr() as *const i8,
> > > + )
> > > + };
> > > + to_result(ret)?;
> > > + Ok(ret as usize)
> > > + }
> > > +
> > > + /// Returns firmware property `name` scalar value
> > > + ///
> > > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > > + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > > + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > > +
> > > + Self::_property_read_array(&self, name, &mut val)?;
> > > + Ok(val[0])
> > > + }
> > > +
> >
> > This, and several of the other methods are unsound, because they can
> > be used to construct arbitrary types for which may not allow all bit
> > patterns. You can use:
> > https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
> > bound to ensure only valid types are used.
> >
> > Also, instead of using mem::zeroed(), you should use MaybeUnininit
> > (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
> > which allows you to avoid needing to zero initialize.
>
> Something like this what you had in mind?:
>
> pub fn property_read_array<T, const N: usize>(&self, name: &CStr) ->
> Result<[T; N]> {
> let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
>
> Self::_property_read_array(self, name, &mut val)?;
>
> // SAFETY: On success, _property_read_array has filled in the array
> let val = unsafe { mem::transmute_copy(&val) };
> Ok(val)
> }
Yes, that's right. Ideally you could use
https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.array_assume_init
instead of transmute, but it's not yet stable.
Alex
--
All that is necessary for evil to succeed is for good people to do nothing.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
2024-10-25 21:12 ` Alex Gaynor
2024-10-28 7:12 ` Dirk Behme
@ 2024-10-29 14:16 ` Alice Ryhl
2024-10-29 17:57 ` Rob Herring
2 siblings, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2024-10-29 14:16 UTC (permalink / raw)
To: Rob Herring (Arm)
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Fri, Oct 25, 2024 at 11:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
>
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> Not sure if we need the KVec variant, but I kept it as that was my first
> pass attempt. Most callers are filling in some value in a driver data
> struct. Sometimes the number of elements is not known, so the caller
> calls to get the array size, allocs the correct size buffer, and then
> reads the property again to fill in the buffer.
>
> I have not implemented a wrapper for device_property_read_string(_array)
> because that API is problematic for dynamic DT nodes. The API just
> returns pointer(s) into the raw DT data. We probably need to return a
> copy of the string(s) instead for rust.
>
> After property accessors, next up is child node accessors/iterators.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 145 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9..65717cc20a23 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -19,6 +19,7 @@
> #include <linux/pci.h>
> #include <linux/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 0c28b1e6b004..bb66a28df890 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -5,10 +5,14 @@
> //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>
> use crate::{
> + alloc::KVec,
> bindings,
> + error::{to_result, Result},
> + prelude::*,
> + str::CStr,
> types::{ARef, Opaque},
> };
> -use core::{fmt, ptr};
> +use core::{fmt, mem::size_of, ptr};
>
> #[cfg(CONFIG_PRINTK)]
> use crate::c_str;
> @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> )
> };
> }
> +
> + /// Returns if a firmware property `name` is true or false
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> + }
> +
> + /// Returns if a firmware string property `name` has match for `match_str`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + let ret = unsafe {
> + bindings::device_property_match_string(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + match_str.as_ptr() as *const i8,
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` scalar value
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(&self, name, &mut val)?;
> + Ok(val[0])
> + }
> +
> + /// Returns firmware property `name` array values
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(self, name, &mut val)?;
> + Ok(val)
> + }
> +
> + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
> + match size_of::<T>() {
> + 1 => to_result(unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u8,
> + val.len(),
> + )
> + })?,
> + 2 => to_result(unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u16,
> + val.len(),
> + )
> + })?,
> + 4 => to_result(unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u32,
> + val.len(),
> + )
> + })?,
> + 8 => to_result(unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u64,
> + val.len(),
> + )
> + })?,
> + _ => return Err(EINVAL),
> + }
> + Ok(())
> + }
> +
> + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: len always matches capacity
> + unsafe { val.set_len(len) }
> + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> + Ok(val)
> + }
> +
> + /// Returns array length for firmware property `name`
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
This always returns usize? I'm a bit confused ...
> + match size_of::<T>() {
> + 1 => {
> + ret = unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 2 => {
> + ret = unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 4 => {
> + ret = unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 8 => {
> + ret = unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + _ => return Err(EINVAL),
You can use `kernel::build_error!` here to trigger a build failure if
the size is wrong.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 14:16 ` Alice Ryhl
@ 2024-10-29 17:57 ` Rob Herring
2024-10-29 18:29 ` Miguel Ojeda
2024-10-29 18:48 ` Alice Ryhl
0 siblings, 2 replies; 28+ messages in thread
From: Rob Herring @ 2024-10-29 17:57 UTC (permalink / raw)
To: Alice Ryhl
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 9:16 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Oct 25, 2024 at 11:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> >
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > sizes of properties (e.g. u8, u16, u32).
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Not sure if we need the KVec variant, but I kept it as that was my first
> > pass attempt. Most callers are filling in some value in a driver data
> > struct. Sometimes the number of elements is not known, so the caller
> > calls to get the array size, allocs the correct size buffer, and then
> > reads the property again to fill in the buffer.
> >
> > I have not implemented a wrapper for device_property_read_string(_array)
> > because that API is problematic for dynamic DT nodes. The API just
> > returns pointer(s) into the raw DT data. We probably need to return a
> > copy of the string(s) instead for rust.
> >
> > After property accessors, next up is child node accessors/iterators.
> > ---
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/device.rs | 145 +++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 217c776615b9..65717cc20a23 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -19,6 +19,7 @@
> > #include <linux/pci.h>
> > #include <linux/phy.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/refcount.h>
> > #include <linux/sched.h>
> > #include <linux/slab.h>
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 0c28b1e6b004..bb66a28df890 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -5,10 +5,14 @@
> > //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> > use crate::{
> > + alloc::KVec,
> > bindings,
> > + error::{to_result, Result},
> > + prelude::*,
> > + str::CStr,
> > types::{ARef, Opaque},
> > };
> > -use core::{fmt, ptr};
> > +use core::{fmt, mem::size_of, ptr};
> >
> > #[cfg(CONFIG_PRINTK)]
> > use crate::c_str;
> > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> > )
> > };
> > }
> > +
> > + /// Returns if a firmware property `name` is true or false
> > + pub fn property_read_bool(&self, name: &CStr) -> bool {
> > + unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > + }
> > +
> > + /// Returns if a firmware string property `name` has match for `match_str`
> > + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > + let ret = unsafe {
> > + bindings::device_property_match_string(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + match_str.as_ptr() as *const i8,
> > + )
> > + };
> > + to_result(ret)?;
> > + Ok(ret as usize)
> > + }
> > +
> > + /// Returns firmware property `name` scalar value
> > + ///
> > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > + pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > + let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > +
> > + Self::_property_read_array(&self, name, &mut val)?;
> > + Ok(val[0])
> > + }
> > +
> > + /// Returns firmware property `name` array values
> > + ///
> > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> > + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> > +
> > + Self::_property_read_array(self, name, &mut val)?;
> > + Ok(val)
> > + }
> > +
> > + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
> > + match size_of::<T>() {
> > + 1 => to_result(unsafe {
> > + bindings::device_property_read_u8_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + val.as_ptr() as *mut u8,
> > + val.len(),
> > + )
> > + })?,
> > + 2 => to_result(unsafe {
> > + bindings::device_property_read_u16_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + val.as_ptr() as *mut u16,
> > + val.len(),
> > + )
> > + })?,
> > + 4 => to_result(unsafe {
> > + bindings::device_property_read_u32_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + val.as_ptr() as *mut u32,
> > + val.len(),
> > + )
> > + })?,
> > + 8 => to_result(unsafe {
> > + bindings::device_property_read_u64_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + val.as_ptr() as *mut u64,
> > + val.len(),
> > + )
> > + })?,
> > + _ => return Err(EINVAL),
> > + }
> > + Ok(())
> > + }
> > +
> > + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> > + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> > +
> > + // SAFETY: len always matches capacity
> > + unsafe { val.set_len(len) }
> > + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> > + Ok(val)
> > + }
> > +
> > + /// Returns array length for firmware property `name`
> > + ///
> > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
>
> This always returns usize? I'm a bit confused ...
The C version returned an int so we could return an errno or positive
count. With Result, we don't need negative values and isn't usize
generally used for counts of things like size_t in C?
> > + match size_of::<T>() {
> > + 1 => {
> > + ret = unsafe {
> > + bindings::device_property_read_u8_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + ptr::null_mut(),
> > + 0,
> > + )
> > + }
> > + }
> > + 2 => {
> > + ret = unsafe {
> > + bindings::device_property_read_u16_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + ptr::null_mut(),
> > + 0,
> > + )
> > + }
> > + }
> > + 4 => {
> > + ret = unsafe {
> > + bindings::device_property_read_u32_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + ptr::null_mut(),
> > + 0,
> > + )
> > + }
> > + }
> > + 8 => {
> > + ret = unsafe {
> > + bindings::device_property_read_u64_array(
> > + self.as_raw(),
> > + name.as_ptr() as *const i8,
> > + ptr::null_mut(),
> > + 0,
> > + )
> > + }
> > + }
> > + _ => return Err(EINVAL),
>
> You can use `kernel::build_error!` here to trigger a build failure if
> the size is wrong.
I really want a build error if the type is wrong, then the _ case
would be unreachable. No way to do that?
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 17:57 ` Rob Herring
@ 2024-10-29 18:29 ` Miguel Ojeda
2024-10-29 18:30 ` Miguel Ojeda
2024-10-29 18:48 ` Alice Ryhl
1 sibling, 1 reply; 28+ messages in thread
From: Miguel Ojeda @ 2024-10-29 18:29 UTC (permalink / raw)
To: Rob Herring
Cc: Alice Ryhl, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 6:58 PM Rob Herring <robh@kernel.org> wrote:
>
> I really want a build error if the type is wrong, then the _ case
> would be unreachable. No way to do that?
When you need to work with a set of types, you would normally define a
trait and implement it only for those traits, e.g. see
rust/kernel/transmute.rs. Then it is a build error if you don't
satisfy bounds etc.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 18:29 ` Miguel Ojeda
@ 2024-10-29 18:30 ` Miguel Ojeda
0 siblings, 0 replies; 28+ messages in thread
From: Miguel Ojeda @ 2024-10-29 18:30 UTC (permalink / raw)
To: Rob Herring
Cc: Alice Ryhl, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 7:29 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> trait and implement it only for those traits, e.g. see
"for those types"
Cheers,
Miguel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 17:57 ` Rob Herring
2024-10-29 18:29 ` Miguel Ojeda
@ 2024-10-29 18:48 ` Alice Ryhl
2024-10-29 18:57 ` Miguel Ojeda
1 sibling, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2024-10-29 18:48 UTC (permalink / raw)
To: Rob Herring
Cc: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 6:58 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 9:16 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Oct 25, 2024 at 11:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> > > +
> > > + /// Returns array length for firmware property `name`
> > > + ///
> > > + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > > + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> >
> > This always returns usize? I'm a bit confused ...
>
> The C version returned an int so we could return an errno or positive
> count. With Result, we don't need negative values and isn't usize
> generally used for counts of things like size_t in C?
Ok, I think I misunderstood what this does. usize is fine.
> > > + match size_of::<T>() {
> > > + 1 => {
> > > + ret = unsafe {
> > > + bindings::device_property_read_u8_array(
> > > + self.as_raw(),
> > > + name.as_ptr() as *const i8,
> > > + ptr::null_mut(),
> > > + 0,
> > > + )
> > > + }
> > > + }
> > > + 2 => {
> > > + ret = unsafe {
> > > + bindings::device_property_read_u16_array(
> > > + self.as_raw(),
> > > + name.as_ptr() as *const i8,
> > > + ptr::null_mut(),
> > > + 0,
> > > + )
> > > + }
> > > + }
> > > + 4 => {
> > > + ret = unsafe {
> > > + bindings::device_property_read_u32_array(
> > > + self.as_raw(),
> > > + name.as_ptr() as *const i8,
> > > + ptr::null_mut(),
> > > + 0,
> > > + )
> > > + }
> > > + }
> > > + 8 => {
> > > + ret = unsafe {
> > > + bindings::device_property_read_u64_array(
> > > + self.as_raw(),
> > > + name.as_ptr() as *const i8,
> > > + ptr::null_mut(),
> > > + 0,
> > > + )
> > > + }
> > > + }
> > > + _ => return Err(EINVAL),
> >
> > You can use `kernel::build_error!` here to trigger a build failure if
> > the size is wrong.
>
> I really want a build error if the type is wrong, then the _ case
> would be unreachable. No way to do that?
One option is to define a trait for integers:
trait Integer: FromBytes + AsBytes + Copy {
const SIZE: IntSize;
}
enum IntSize {
S8,
S16,
S32,
S64,
}
macro_rules! impl_int {
($($typ:ty),* $(,)?) => {$(
impl Integer for $typ {
const SIZE: IntSize = match size_of::<Self>() {
1 => IntSize::S8,
2 => IntSize::S16,
4 => IntSize::S32,
8 => IntSize::S64,
_ => panic!("invalid size"),
};
}
)*};
}
impl_int! {
u8, u16, u32, u64, usize,
i8, i16, i32, i64, isize,
}
Using the above trait, you can match on the IntSize.
pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
match T::SIZE {
IntSize::S8 => ...,
IntSize::S16 => ...,
IntSize::S32 => ...,
IntSize::S64 => ...,
}
this leaves no catch-all case and calling it with non-integer types
will not compile.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 18:48 ` Alice Ryhl
@ 2024-10-29 18:57 ` Miguel Ojeda
2024-10-29 19:35 ` Rob Herring
0 siblings, 1 reply; 28+ messages in thread
From: Miguel Ojeda @ 2024-10-29 18:57 UTC (permalink / raw)
To: Alice Ryhl
Cc: Rob Herring, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> One option is to define a trait for integers:
+1, one more thing to consider is whether it makes sense to define a
DT-only trait that holds all the types that can be a device property
(like `bool` too, not just the `Integer`s).
Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 18:57 ` Miguel Ojeda
@ 2024-10-29 19:35 ` Rob Herring
2024-10-29 22:05 ` Rob Herring
2024-10-30 8:15 ` Alice Ryhl
0 siblings, 2 replies; 28+ messages in thread
From: Rob Herring @ 2024-10-29 19:35 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Alice Ryhl, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > One option is to define a trait for integers:
Yeah, but that doesn't feel like something I should do here. I imagine
other things might need the same thing. Perhaps the bindings for
readb/readw/readl for example. And essentially the crate:num already
has the trait I need. Shouldn't the kernel mirror that? I recall
seeing some topic of including crates in the kernel?
> +1, one more thing to consider is whether it makes sense to define a
> DT-only trait that holds all the types that can be a device property
> (like `bool` too, not just the `Integer`s).
>
> Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
Is there no way to say must have traitA or traitB?
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 19:35 ` Rob Herring
@ 2024-10-29 22:05 ` Rob Herring
2024-10-30 8:09 ` Alice Ryhl
2024-10-30 8:15 ` Alice Ryhl
1 sibling, 1 reply; 28+ messages in thread
From: Rob Herring @ 2024-10-29 22:05 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Alice Ryhl, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 2:35 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > One option is to define a trait for integers:
>
> Yeah, but that doesn't feel like something I should do here. I imagine
> other things might need the same thing. Perhaps the bindings for
> readb/readw/readl for example. And essentially the crate:num already
> has the trait I need. Shouldn't the kernel mirror that? I recall
> seeing some topic of including crates in the kernel?
Looking a bit closer at FromBytes, that's almost what I need. I'm not
worried if it also includes usize and isize. However, since the trait
also is provided for arrays/slices, the following happens to work:
let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array"))?;
Which is a typo meant to be:
let prop: [i16; 4] = dev.property_read_array(c_str!("test,i16-array"))?;
And it doesn't really work. It reads all the data, but reads it as a
u64 (DT data is BE), so the order is wrong.
And now my dreams of "if it compiles, it works" are shattered. ;)
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 22:05 ` Rob Herring
@ 2024-10-30 8:09 ` Alice Ryhl
0 siblings, 0 replies; 28+ messages in thread
From: Alice Ryhl @ 2024-10-30 8:09 UTC (permalink / raw)
To: Rob Herring
Cc: Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 11:05 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 2:35 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > One option is to define a trait for integers:
> >
> > Yeah, but that doesn't feel like something I should do here. I imagine
> > other things might need the same thing. Perhaps the bindings for
> > readb/readw/readl for example. And essentially the crate:num already
> > has the trait I need. Shouldn't the kernel mirror that? I recall
> > seeing some topic of including crates in the kernel?
>
> Looking a bit closer at FromBytes, that's almost what I need. I'm not
> worried if it also includes usize and isize. However, since the trait
> also is provided for arrays/slices, the following happens to work:
>
> let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array"))?;
>
> Which is a typo meant to be:
>
> let prop: [i16; 4] = dev.property_read_array(c_str!("test,i16-array"))?;
>
> And it doesn't really work. It reads all the data, but reads it as a
> u64 (DT data is BE), so the order is wrong.
>
> And now my dreams of "if it compiles, it works" are shattered. ;)
It sounds like FromBytes isn't the right way to go, then.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-29 19:35 ` Rob Herring
2024-10-29 22:05 ` Rob Herring
@ 2024-10-30 8:15 ` Alice Ryhl
2024-10-30 14:05 ` Rob Herring
1 sibling, 1 reply; 28+ messages in thread
From: Alice Ryhl @ 2024-10-30 8:15 UTC (permalink / raw)
To: Rob Herring
Cc: Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > >
> > > One option is to define a trait for integers:
>
> Yeah, but that doesn't feel like something I should do here. I imagine
> other things might need the same thing. Perhaps the bindings for
> readb/readw/readl for example. And essentially the crate:num already
> has the trait I need. Shouldn't the kernel mirror that? I recall
> seeing some topic of including crates in the kernel?
You can design the trait to look similar to traits in external crates.
We did that for FromBytes/AsBytes.
I assume you're referring to the PrimInt trait [1]? That trait doesn't
really let you get rid of the catch-all case, and it's not even
unreachable due to the u128 type.
[1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
> > +1, one more thing to consider is whether it makes sense to define a
> > DT-only trait that holds all the types that can be a device property
> > (like `bool` too, not just the `Integer`s).
> >
> > Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
>
> Is there no way to say must have traitA or traitB?
No. What should it do if you pass it something that implements both traits?
If you want a single function name, you'll need one trait.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 8:15 ` Alice Ryhl
@ 2024-10-30 14:05 ` Rob Herring
2024-10-30 15:43 ` Alice Ryhl
2024-10-30 16:03 ` Dirk Behme
0 siblings, 2 replies; 28+ messages in thread
From: Rob Herring @ 2024-10-30 14:05 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > >
> > > On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > > >
> > > > One option is to define a trait for integers:
> >
> > Yeah, but that doesn't feel like something I should do here. I imagine
> > other things might need the same thing. Perhaps the bindings for
> > readb/readw/readl for example. And essentially the crate:num already
> > has the trait I need. Shouldn't the kernel mirror that? I recall
> > seeing some topic of including crates in the kernel?
>
> You can design the trait to look similar to traits in external crates.
> We did that for FromBytes/AsBytes.
>
> I assume you're referring to the PrimInt trait [1]? That trait doesn't
> really let you get rid of the catch-all case, and it's not even
> unreachable due to the u128 type.
It was num::Integer which seems to be similar.
>
> [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
>
> > > +1, one more thing to consider is whether it makes sense to define a
> > > DT-only trait that holds all the types that can be a device property
> > > (like `bool` too, not just the `Integer`s).
> > >
> > > Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
> >
> > Is there no way to say must have traitA or traitB?
>
> No. What should it do if you pass it something that implements both traits?
>
> If you want a single function name, you'll need one trait.
I'm not sure I want that actually.
DT boolean is a bit special. A property not present is false.
Everything else is true. For example, 'prop = <0>' or 'prop =
"string"' are both true. I'm moving things in the kernel to be
stricter so that those cases are errors. I recently introduced
(of|device)_property_present() for that reason. There's no type
information stored in DT. At the DT level, it's all just byte arrays.
However, we now have all the type information for properties within
the schema. So eventually, I want to use that to warn on accessing
properties with the wrong type.
For example, I think I don't want this to work:
if dev.property_read(c_str!("test,i16-array"))? {
// do something
}
But instead have:
if dev.property_present(c_str!("test,i16-array")) {
// do something
}
To actually warn on property_read_bool, I'm going to have to rework
the underlying C implementation to separate device_property_present
and device_property_read_bool implementations.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 14:05 ` Rob Herring
@ 2024-10-30 15:43 ` Alice Ryhl
2024-10-30 16:03 ` Dirk Behme
1 sibling, 0 replies; 28+ messages in thread
From: Alice Ryhl @ 2024-10-30 15:43 UTC (permalink / raw)
To: Rob Herring
Cc: Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Dirk Behme, devicetree,
linux-kernel, rust-for-linux
On Wed, Oct 30, 2024 at 3:06 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> > > <miguel.ojeda.sandonis@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> > > > >
> > > > > One option is to define a trait for integers:
> > >
> > > Yeah, but that doesn't feel like something I should do here. I imagine
> > > other things might need the same thing. Perhaps the bindings for
> > > readb/readw/readl for example. And essentially the crate:num already
> > > has the trait I need. Shouldn't the kernel mirror that? I recall
> > > seeing some topic of including crates in the kernel?
> >
> > You can design the trait to look similar to traits in external crates.
> > We did that for FromBytes/AsBytes.
> >
> > I assume you're referring to the PrimInt trait [1]? That trait doesn't
> > really let you get rid of the catch-all case, and it's not even
> > unreachable due to the u128 type.
>
> It was num::Integer which seems to be similar.
Abstracting over a set of C functions that matches on the different
integer types seems like it'll be pretty common in the kernel. I think
it's perfectly fine for you to add a trait for that purpose.
> > [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
> >
> > > > +1, one more thing to consider is whether it makes sense to define a
> > > > DT-only trait that holds all the types that can be a device property
> > > > (like `bool` too, not just the `Integer`s).
> > > >
> > > > Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
> > >
> > > Is there no way to say must have traitA or traitB?
> >
> > No. What should it do if you pass it something that implements both traits?
> >
> > If you want a single function name, you'll need one trait.
>
> I'm not sure I want that actually.
>
> DT boolean is a bit special. A property not present is false.
> Everything else is true. For example, 'prop = <0>' or 'prop =
> "string"' are both true. I'm moving things in the kernel to be
> stricter so that those cases are errors. I recently introduced
> (of|device)_property_present() for that reason. There's no type
> information stored in DT. At the DT level, it's all just byte arrays.
> However, we now have all the type information for properties within
> the schema. So eventually, I want to use that to warn on accessing
> properties with the wrong type.
>
> For example, I think I don't want this to work:
>
> if dev.property_read(c_str!("test,i16-array"))? {
> // do something
> }
>
> But instead have:
>
> if dev.property_present(c_str!("test,i16-array")) {
> // do something
> }
>
> To actually warn on property_read_bool, I'm going to have to rework
> the underlying C implementation to separate device_property_present
> and device_property_read_bool implementations.
Having bool separate seems fine to me.
Alice
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 14:05 ` Rob Herring
2024-10-30 15:43 ` Alice Ryhl
@ 2024-10-30 16:03 ` Dirk Behme
2024-10-30 16:47 ` Rob Herring
1 sibling, 1 reply; 28+ messages in thread
From: Dirk Behme @ 2024-10-30 16:03 UTC (permalink / raw)
To: Rob Herring, Alice Ryhl
Cc: Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, devicetree, linux-kernel,
rust-for-linux
On 30.10.24 15:05, Rob Herring wrote:
> On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
>>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>>
>>>> On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>
>>>>> One option is to define a trait for integers:
>>>
>>> Yeah, but that doesn't feel like something I should do here. I imagine
>>> other things might need the same thing. Perhaps the bindings for
>>> readb/readw/readl for example. And essentially the crate:num already
>>> has the trait I need. Shouldn't the kernel mirror that? I recall
>>> seeing some topic of including crates in the kernel?
>>
>> You can design the trait to look similar to traits in external crates.
>> We did that for FromBytes/AsBytes.
>>
>> I assume you're referring to the PrimInt trait [1]? That trait doesn't
>> really let you get rid of the catch-all case, and it's not even
>> unreachable due to the u128 type.
>
> It was num::Integer which seems to be similar.
>
>>
>> [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
>>
>>>> +1, one more thing to consider is whether it makes sense to define a
>>>> DT-only trait that holds all the types that can be a device property
>>>> (like `bool` too, not just the `Integer`s).
>>>>
>>>> Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
>>>
>>> Is there no way to say must have traitA or traitB?
>>
>> No. What should it do if you pass it something that implements both traits?
>>
>> If you want a single function name, you'll need one trait.
>
> I'm not sure I want that actually.
>
> DT boolean is a bit special. A property not present is false.
> Everything else is true. For example, 'prop = <0>' or 'prop =
> "string"' are both true. I'm moving things in the kernel to be
> stricter so that those cases are errors. I recently introduced
> (of|device)_property_present() for that reason. There's no type
> information stored in DT. At the DT level, it's all just byte arrays.
> However, we now have all the type information for properties within
> the schema. So eventually, I want to use that to warn on accessing
> properties with the wrong type.
>
> For example, I think I don't want this to work:
>
> if dev.property_read(c_str!("test,i16-array"))? {
> // do something
> }
>
> But instead have:
>
> if dev.property_present(c_str!("test,i16-array")) {
> // do something
> }
I think we have "optional" properties which can be there (== true) or
not (== false). Let's assume for this example "test,i16-array" is such
kind of "optional" property. With what you gave above we need two
device tree accesses, then? One to check if it is there and one to
read the data:
let mut array = <empty_marker>;
if dev.property_present(c_str!("test,i16-array")) {
array = dev.property_read(c_str!("test,i16-array"))?;
}
?
Instead of these two accesses, I was thinking to use the error
property_read() will return if the optional property is not there to
just do one access:
let mut array = <empty_marker>;
if let Ok(val) = dev.property_read(c_str!("test,i16-array")) {
array = val;
}
(and ignore the error case as its irrelvant in the optional case)
Have I missed anything?
Best regards
Dirk
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 16:03 ` Dirk Behme
@ 2024-10-30 16:47 ` Rob Herring
2024-10-31 7:19 ` Dirk Behme
2024-11-15 6:39 ` Dirk Behme
0 siblings, 2 replies; 28+ messages in thread
From: Rob Herring @ 2024-10-30 16:47 UTC (permalink / raw)
To: Dirk Behme
Cc: Alice Ryhl, Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, devicetree, linux-kernel,
rust-for-linux
On Wed, Oct 30, 2024 at 11:03 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>
> On 30.10.24 15:05, Rob Herring wrote:
> > On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
> >>>
> >>> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
> >>> <miguel.ojeda.sandonis@gmail.com> wrote:
> >>>>
> >>>> On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >>>>>
> >>>>> One option is to define a trait for integers:
> >>>
> >>> Yeah, but that doesn't feel like something I should do here. I imagine
> >>> other things might need the same thing. Perhaps the bindings for
> >>> readb/readw/readl for example. And essentially the crate:num already
> >>> has the trait I need. Shouldn't the kernel mirror that? I recall
> >>> seeing some topic of including crates in the kernel?
> >>
> >> You can design the trait to look similar to traits in external crates.
> >> We did that for FromBytes/AsBytes.
> >>
> >> I assume you're referring to the PrimInt trait [1]? That trait doesn't
> >> really let you get rid of the catch-all case, and it's not even
> >> unreachable due to the u128 type.
> >
> > It was num::Integer which seems to be similar.
> >
> >>
> >> [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
> >>
> >>>> +1, one more thing to consider is whether it makes sense to define a
> >>>> DT-only trait that holds all the types that can be a device property
> >>>> (like `bool` too, not just the `Integer`s).
> >>>>
> >>>> Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
> >>>
> >>> Is there no way to say must have traitA or traitB?
> >>
> >> No. What should it do if you pass it something that implements both traits?
> >>
> >> If you want a single function name, you'll need one trait.
> >
> > I'm not sure I want that actually.
> >
> > DT boolean is a bit special. A property not present is false.
> > Everything else is true. For example, 'prop = <0>' or 'prop =
> > "string"' are both true. I'm moving things in the kernel to be
> > stricter so that those cases are errors. I recently introduced
> > (of|device)_property_present() for that reason. There's no type
> > information stored in DT. At the DT level, it's all just byte arrays.
> > However, we now have all the type information for properties within
> > the schema. So eventually, I want to use that to warn on accessing
> > properties with the wrong type.
> >
> > For example, I think I don't want this to work:
> >
> > if dev.property_read(c_str!("test,i16-array"))? {
> > // do something
> > }
> >
> > But instead have:
> >
> > if dev.property_present(c_str!("test,i16-array")) {
> > // do something
> > }
>
> I think we have "optional" properties which can be there (== true) or
> not (== false). Let's assume for this example "test,i16-array" is such
> kind of "optional" property. With what you gave above we need two
> device tree accesses, then? One to check if it is there and one to
> read the data:
Yes, lots of properties are optional especially since any new property
added has to be because the DT is an ABI.
> let mut array = <empty_marker>;
> if dev.property_present(c_str!("test,i16-array")) {
> array = dev.property_read(c_str!("test,i16-array"))?;
> }
>
> ?
>
> Instead of these two accesses, I was thinking to use the error
> property_read() will return if the optional property is not there to
> just do one access:
>
> let mut array = <empty_marker>;
> if let Ok(val) = dev.property_read(c_str!("test,i16-array")) {
> array = val;
> }
>
> (and ignore the error case as its irrelvant in the optional case)
>
> Have I missed anything?
If you grep "_property_present", most if not all calls never need the
data. When you need the data, you read it and test for EINVAL if you
want to handle "not present". The overhead of parsing the data is not
nothing, so I think it is better to provide both.
The typical pattern in the C code is:
u32 val = DEFAULT_VALUE;
of_property_read_u32(node, "a-property", &val);
// val is now either the read property or the default. If the property
is required, then the error code needs to be checked.
Maybe we should have:
let val: u32 = dev.property_read_optional(c_str!("test,i16-array"),
DEFAULT_VALUE);
Or looks like Option<> could be used here?:
let val: u32 = dev.property_read(c_str!("test,i16-array"),
Option<DEFAULT_VALUE>);
One thing I'd like to improve is having fewer driver error messages
and a printk for a missing required property is a common one. We have
APIs like clk_get and clk_get_optional (which parse firmware
properties). The difference is the former prints an error message on
error case and the latter is silent.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 16:47 ` Rob Herring
@ 2024-10-31 7:19 ` Dirk Behme
2024-11-15 6:39 ` Dirk Behme
1 sibling, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-10-31 7:19 UTC (permalink / raw)
To: Rob Herring
Cc: Alice Ryhl, Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, devicetree, linux-kernel,
rust-for-linux
On 30.10.24 17:47, Rob Herring wrote:
> On Wed, Oct 30, 2024 at 11:03 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>>
>> On 30.10.24 15:05, Rob Herring wrote:
>>> On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>
>>>> On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
>>>>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>>>
>>>>>>> One option is to define a trait for integers:
>>>>>
>>>>> Yeah, but that doesn't feel like something I should do here. I imagine
>>>>> other things might need the same thing. Perhaps the bindings for
>>>>> readb/readw/readl for example. And essentially the crate:num already
>>>>> has the trait I need. Shouldn't the kernel mirror that? I recall
>>>>> seeing some topic of including crates in the kernel?
>>>>
>>>> You can design the trait to look similar to traits in external crates.
>>>> We did that for FromBytes/AsBytes.
>>>>
>>>> I assume you're referring to the PrimInt trait [1]? That trait doesn't
>>>> really let you get rid of the catch-all case, and it's not even
>>>> unreachable due to the u128 type.
>>>
>>> It was num::Integer which seems to be similar.
>>>
>>>>
>>>> [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
>>>>
>>>>>> +1, one more thing to consider is whether it makes sense to define a
>>>>>> DT-only trait that holds all the types that can be a device property
>>>>>> (like `bool` too, not just the `Integer`s).
>>>>>>
>>>>>> Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
>>>>>
>>>>> Is there no way to say must have traitA or traitB?
>>>>
>>>> No. What should it do if you pass it something that implements both traits?
>>>>
>>>> If you want a single function name, you'll need one trait.
>>>
>>> I'm not sure I want that actually.
>>>
>>> DT boolean is a bit special. A property not present is false.
>>> Everything else is true. For example, 'prop = <0>' or 'prop =
>>> "string"' are both true. I'm moving things in the kernel to be
>>> stricter so that those cases are errors. I recently introduced
>>> (of|device)_property_present() for that reason. There's no type
>>> information stored in DT. At the DT level, it's all just byte arrays.
>>> However, we now have all the type information for properties within
>>> the schema. So eventually, I want to use that to warn on accessing
>>> properties with the wrong type.
>>>
>>> For example, I think I don't want this to work:
>>>
>>> if dev.property_read(c_str!("test,i16-array"))? {
>>> // do something
>>> }
>>>
>>> But instead have:
>>>
>>> if dev.property_present(c_str!("test,i16-array")) {
>>> // do something
>>> }
>>
>> I think we have "optional" properties which can be there (== true) or
>> not (== false). Let's assume for this example "test,i16-array" is such
>> kind of "optional" property. With what you gave above we need two
>> device tree accesses, then? One to check if it is there and one to
>> read the data:
>
> Yes, lots of properties are optional especially since any new property
> added has to be because the DT is an ABI.
>
>> let mut array = <empty_marker>;
>> if dev.property_present(c_str!("test,i16-array")) {
>> array = dev.property_read(c_str!("test,i16-array"))?;
>> }
>>
>> ?
>>
>> Instead of these two accesses, I was thinking to use the error
>> property_read() will return if the optional property is not there to
>> just do one access:
>>
>> let mut array = <empty_marker>;
>> if let Ok(val) = dev.property_read(c_str!("test,i16-array")) {
>> array = val;
>> }
>>
>> (and ignore the error case as its irrelvant in the optional case)
>>
>> Have I missed anything?
>
> If you grep "_property_present", most if not all calls never need the
> data. When you need the data, you read it and test for EINVAL if you
> want to handle "not present". The overhead of parsing the data is not
> nothing, so I think it is better to provide both.
>
> The typical pattern in the C code is:
>
> u32 val = DEFAULT_VALUE;
> of_property_read_u32(node, "a-property", &val);
>
> // val is now either the read property or the default. If the property
> is required, then the error code needs to be checked.
Yes :)
> Maybe we should have:
>
> let val: u32 = dev.property_read_optional(c_str!("test,i16-array"),
> DEFAULT_VALUE);
>
> Or looks like Option<> could be used here?:
>
> let val: u32 = dev.property_read(c_str!("test,i16-array"),
> Option<DEFAULT_VALUE>);
In the success case we will get back Some(val)? In the error case
'val' will get Some(DEFAULT_VALUE)? But where would we get the error
value itself (e.g. EINVAL)? Or is the idea to not care about that any
more then? When would we use None?
Best regards
Dirk
> One thing I'd like to improve is having fewer driver error messages
> and a printk for a missing required property is a common one. We have
> APIs like clk_get and clk_get_optional (which parse firmware
> properties). The difference is the former prints an error message on
> error case and the latter is silent.
>
> Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 2/3] rust: Add bindings for device properties
2024-10-30 16:47 ` Rob Herring
2024-10-31 7:19 ` Dirk Behme
@ 2024-11-15 6:39 ` Dirk Behme
1 sibling, 0 replies; 28+ messages in thread
From: Dirk Behme @ 2024-11-15 6:39 UTC (permalink / raw)
To: Rob Herring, Dirk Behme
Cc: Alice Ryhl, Miguel Ojeda, Saravana Kannan, Greg Kroah-Hartman,
Rafael J. Wysocki, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, devicetree, linux-kernel,
rust-for-linux
Hi Rob,
On 30/10/2024 17:47, Rob Herring wrote:
> On Wed, Oct 30, 2024 at 11:03 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>>
>> On 30.10.24 15:05, Rob Herring wrote:
>>> On Wed, Oct 30, 2024 at 3:15 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>
>>>> On Tue, Oct 29, 2024 at 8:35 PM Rob Herring <robh@kernel.org> wrote:
>>>>>
>>>>> On Tue, Oct 29, 2024 at 1:57 PM Miguel Ojeda
>>>>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Oct 29, 2024 at 7:48 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>>>>>>
>>>>>>> One option is to define a trait for integers:
>>>>>
>>>>> Yeah, but that doesn't feel like something I should do here. I imagine
>>>>> other things might need the same thing. Perhaps the bindings for
>>>>> readb/readw/readl for example. And essentially the crate:num already
>>>>> has the trait I need. Shouldn't the kernel mirror that? I recall
>>>>> seeing some topic of including crates in the kernel?
>>>>
>>>> You can design the trait to look similar to traits in external crates.
>>>> We did that for FromBytes/AsBytes.
>>>>
>>>> I assume you're referring to the PrimInt trait [1]? That trait doesn't
>>>> really let you get rid of the catch-all case, and it's not even
>>>> unreachable due to the u128 type.
>>>
>>> It was num::Integer which seems to be similar.
>>>
>>>>
>>>> [1]: https://docs.rs/num-traits/0.2.19/num_traits/int/trait.PrimInt.html
>>>>
>>>>>> +1, one more thing to consider is whether it makes sense to define a
>>>>>> DT-only trait that holds all the types that can be a device property
>>>>>> (like `bool` too, not just the `Integer`s).
>>>>>>
>>>>>> Then we can avoid e.g. `property_read_bool` and simply do it in `property_read`.
>>>>>
>>>>> Is there no way to say must have traitA or traitB?
>>>>
>>>> No. What should it do if you pass it something that implements both traits?
>>>>
>>>> If you want a single function name, you'll need one trait.
>>>
>>> I'm not sure I want that actually.
>>>
>>> DT boolean is a bit special. A property not present is false.
>>> Everything else is true. For example, 'prop = <0>' or 'prop =
>>> "string"' are both true. I'm moving things in the kernel to be
>>> stricter so that those cases are errors. I recently introduced
>>> (of|device)_property_present() for that reason. There's no type
>>> information stored in DT. At the DT level, it's all just byte arrays.
>>> However, we now have all the type information for properties within
>>> the schema. So eventually, I want to use that to warn on accessing
>>> properties with the wrong type.
>>>
>>> For example, I think I don't want this to work:
>>>
>>> if dev.property_read(c_str!("test,i16-array"))? {
>>> // do something
>>> }
>>>
>>> But instead have:
>>>
>>> if dev.property_present(c_str!("test,i16-array")) {
>>> // do something
>>> }
>>
>> I think we have "optional" properties which can be there (== true) or
>> not (== false). Let's assume for this example "test,i16-array" is such
>> kind of "optional" property. With what you gave above we need two
>> device tree accesses, then? One to check if it is there and one to
>> read the data:
>
> Yes, lots of properties are optional especially since any new property
> added has to be because the DT is an ABI.
>
>> let mut array = <empty_marker>;
>> if dev.property_present(c_str!("test,i16-array")) {
>> array = dev.property_read(c_str!("test,i16-array"))?;
>> }
>>
>> ?
>>
>> Instead of these two accesses, I was thinking to use the error
>> property_read() will return if the optional property is not there to
>> just do one access:
>>
>> let mut array = <empty_marker>;
>> if let Ok(val) = dev.property_read(c_str!("test,i16-array")) {
>> array = val;
>> }
>>
>> (and ignore the error case as its irrelvant in the optional case)
>>
>> Have I missed anything?
>
> If you grep "_property_present", most if not all calls never need the
> data. When you need the data, you read it and test for EINVAL if you
> want to handle "not present". The overhead of parsing the data is not
> nothing, so I think it is better to provide both.
>
> The typical pattern in the C code is:
>
> u32 val = DEFAULT_VALUE;
> of_property_read_u32(node, "a-property", &val);
>
> // val is now either the read property or the default. If the property
> is required, then the error code needs to be checked.
>
> Maybe we should have:
>
> let val: u32 = dev.property_read_optional(c_str!("test,i16-array"),
> DEFAULT_VALUE);
>
> Or looks like Option<> could be used here?:
>
> let val: u32 = dev.property_read(c_str!("test,i16-array"),
> Option<DEFAULT_VALUE>);
>
> One thing I'd like to improve is having fewer driver error messages
> and a printk for a missing required property is a common one. We have
> APIs like clk_get and clk_get_optional (which parse firmware
> properties). The difference is the former prints an error message on
> error case and the latter is silent.
Maybe something like [1]?
It uses 'None' for mandatory properties and 'Some(<default>)' for
optional properties. And gives an error only in case of a missing
manadatory property.
Best regards
Dirk
[1]
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 4161e7534018a..d97ec2d13a0ba 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -214,12 +214,26 @@ pub fn property_match_string(&self, name: &CStr,
match_str: &CStr) -> Result<usi
/// Returns firmware property `name` scalar value
///
+ /// Option is None: The property is assumed to be mandatory and an
error
+ /// is returned if it is not found.
+ /// Option is Some(default): The property is optional and the
passed default
+ /// value is returned if it is not found.
+ ///
/// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
- pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
+ pub fn property_read<T: Copy>(&self, name: &CStr, default:
Option<T>) -> Result<T> {
let mut val: [T; 1] = unsafe { core::mem::zeroed() };
- Self::_property_read_array(&self, name, &mut val)?;
- Ok(val[0])
+ match Self::_property_read_array(&self, name, &mut val) {
+ Ok(()) => return Ok(val[0]),
+ Err(e) => match default {
+ Some(default) => return Ok(default),
+ None => {
+ dev_err!(self, "Failed to read mandatory property
'{}' with error {}\n",
+ name, e.to_errno());
+ Err(e)
+ }
+ },
+ }
}
/// Returns firmware property `name` array values
diff --git a/samples/rust/rust_driver_platform.rs
b/samples/rust/rust_driver_platform.rs
index 95c2908068623..f8fe0f554183d 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -50,10 +50,21 @@ fn probe(pdev: &mut platform::Device, info:
Option<&Self::IdInfo>) -> Result<Pin
let prop = dev.property_read_bool(c_str!("test,bool-prop"));
dev_info!(dev, "bool prop is {}\n", prop);
- let _prop = dev.property_read::<u32>(c_str!("test,u32-prop"))?;
- let prop: u32 = dev.property_read(c_str!("test,u32-prop"))?;
+ let _prop = dev.property_read::<u32>(c_str!("test,u32-prop"),
None)?;
+ let prop: u32 = dev.property_read(c_str!("test,u32-prop"), None)?;
dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+ // Assume 'test,u32-optional' is an optional property which
does not exist.
+ let prop: u32 = dev.property_read(c_str!("test,u32-optional"),
Some(0xdb))?;
+ // Should print the default 0xdb and give no error.
+ dev_info!(dev, "'test,u32-optional' default is {:#x}\n", prop);
+
+ // Assume 'test,u32-mandatory' is a mandatory property which
does not exist.
+ // Should print an error (but ignore it in this example).
+ match dev.property_read::<u32>(c_str!("test,u32-mandatory"),
None) {
+ _ => (),
+ }
+
let prop: [i16; 4] =
dev.property_read_array(c_str!("test,i16-array"))?;
dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
dev_info!(
^ permalink raw reply related [flat|nested] 28+ messages in thread
end of thread, other threads:[~2024-11-15 6:40 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-25 21:05 [PATCH RFC 0/3] Initial rust bindings for device property reads Rob Herring (Arm)
2024-10-25 21:05 ` [PATCH RFC 1/3] of: unittest: Add a platform device node for rust platform driver sample Rob Herring (Arm)
2024-10-28 7:11 ` Dirk Behme
2024-10-25 21:05 ` [PATCH RFC 2/3] rust: Add bindings for device properties Rob Herring (Arm)
2024-10-25 21:12 ` Alex Gaynor
2024-10-27 22:07 ` Rob Herring
2024-10-28 22:24 ` Rob Herring
2024-10-29 2:08 ` Alex Gaynor
2024-10-28 7:12 ` Dirk Behme
2024-10-29 14:16 ` Alice Ryhl
2024-10-29 17:57 ` Rob Herring
2024-10-29 18:29 ` Miguel Ojeda
2024-10-29 18:30 ` Miguel Ojeda
2024-10-29 18:48 ` Alice Ryhl
2024-10-29 18:57 ` Miguel Ojeda
2024-10-29 19:35 ` Rob Herring
2024-10-29 22:05 ` Rob Herring
2024-10-30 8:09 ` Alice Ryhl
2024-10-30 8:15 ` Alice Ryhl
2024-10-30 14:05 ` Rob Herring
2024-10-30 15:43 ` Alice Ryhl
2024-10-30 16:03 ` Dirk Behme
2024-10-30 16:47 ` Rob Herring
2024-10-31 7:19 ` Dirk Behme
2024-11-15 6:39 ` Dirk Behme
2024-10-25 21:05 ` [PATCH RFC 3/3] samples: rust: platform: Add property read examples Rob Herring (Arm)
2024-10-28 7:13 ` Dirk Behme
2024-10-28 7:11 ` [PATCH RFC 0/3] Initial rust bindings for device property reads Dirk Behme
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).