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.2On 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
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
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
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
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
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
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
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
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
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!(
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
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
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
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
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
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
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())
}
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.
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)
}
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.
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
© 2016 - 2024 Red Hat, Inc.