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
types of properties where appropriate.
The PropertyGuard is a way to force users to specify whether a property
is supposed to be required or not. This allows us to move error
logging of missing required properties into core, preventing a lot of
boilerplate in drivers.
Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 383 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index f6e6c980d..0d4ea3168 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,9 +4,17 @@
//!
//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
-use core::ptr;
+use core::{mem::MaybeUninit, ptr};
-use crate::{bindings, device::Device, str::CStr, types::Opaque};
+use crate::{
+ alloc::KVec,
+ bindings, c_str,
+ device::Device,
+ error::{to_result, Result},
+ prelude::*,
+ str::{BStr, CStr, CString},
+ types::Opaque,
+};
impl Device {
/// Obtain the fwnode corresponding to the device.
@@ -28,6 +36,38 @@ fn fwnode(&self) -> &FwNode {
pub fn property_present(&self, name: &CStr) -> bool {
self.fwnode().property_present(name)
}
+
+ /// Returns firmware property `name` boolean value
+ pub fn property_read_bool(&self, name: &CStr) -> bool {
+ self.fwnode().property_read_bool(name)
+ }
+
+ /// Returns the index of matching string `match_str` for firmware string property `name`
+ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+ self.fwnode().property_match_string(name, match_str)
+ }
+
+ /// Returns firmware property `name` integer array values in a KVec
+ pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
+ &'fwnode self,
+ name: &'name CStr,
+ len: usize,
+ ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
+ self.fwnode().property_read_array_vec(name, len)
+ }
+
+ /// Returns integer array length for firmware property `name`
+ pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
+ self.fwnode().property_count_elem::<T>(name)
+ }
+
+ /// Returns firmware property `name` integer scalar value
+ pub fn property_read<'fwnode, 'name, T: Property>(
+ &'fwnode self,
+ name: &'name CStr,
+ ) -> PropertyGuard<'fwnode, 'name, T> {
+ self.fwnode().property_read(name)
+ }
}
/// A reference-counted fwnode_handle.
@@ -59,6 +99,150 @@ pub fn property_present(&self, name: &CStr) -> bool {
// SAFETY: By the invariant of `CStr`, `name` is null-terminated.
unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
}
+
+ /// Returns firmware property `name` boolean value
+ pub fn property_read_bool(&self, name: &CStr) -> bool {
+ // SAFETY: `name` is non-null and null-terminated. `self.as_raw()` is valid
+ // because `self` is valid.
+ unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
+ }
+
+ /// Returns the index of matching string `match_str` for firmware string property `name`
+ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+ // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
+ // valid because `self` is valid.
+ let ret = unsafe {
+ bindings::fwnode_property_match_string(
+ self.as_raw(),
+ name.as_char_ptr(),
+ match_str.as_char_ptr(),
+ )
+ };
+ to_result(ret)?;
+ Ok(ret as usize)
+ }
+
+ /// Returns firmware property `name` integer array values in a KVec
+ pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
+ &'fwnode self,
+ name: &'name CStr,
+ len: usize,
+ ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
+ let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
+
+ // SAFETY: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
+ // didn't return an error and it has at least space for `len` number
+ // of elements.
+ let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
+ let res = if err < 0 {
+ Err(Error::from_errno(err))
+ } else {
+ // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
+ unsafe { val.set_len(len) }
+ Ok(val)
+ };
+ Ok(PropertyGuard {
+ inner: res,
+ fwnode: self,
+ name,
+ })
+ }
+
+ /// Returns integer array length for firmware property `name`
+ pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
+ // SAFETY: `out_param` is allowed to be null because `len` is zero.
+ let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
+ to_result(ret)?;
+ Ok(ret as usize)
+ }
+
+ /// Returns the value of firmware property `name`.
+ ///
+ /// This method is generic over the type of value to read. Informally,
+ /// the types that can be read are booleans, strings, unsigned integers and
+ /// arrays of unsigned integers.
+ ///
+ /// Reading a `KVec` of integers is done with the separate
+ /// method [`Self::property_read_array_vec`], because it takes an
+ /// additional `len` argument.
+ ///
+ /// When reading a boolean, this method never fails. A missing property
+ /// is interpreted as `false`, whereas a present property is interpreted
+ /// as `true`.
+ ///
+ /// For more precise documentation about what types can be read, see
+ /// the [implementors of Property][Property#implementors] and [its
+ /// implementations on foreign types][Property#foreign-impls].
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// # use crate::{device::Device, types::CString};
+ /// fn examples(dev: &Device) -> Result {
+ /// let fwnode = dev.fwnode();
+ /// let b: bool = fwnode.property_read("some-bool").required()?;
+ /// if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
+ /// // ...
+ /// }
+ /// }
+ /// ```
+ pub fn property_read<'fwnode, 'name, T: Property>(
+ &'fwnode self,
+ name: &'name CStr,
+ ) -> PropertyGuard<'fwnode, 'name, T> {
+ PropertyGuard {
+ inner: T::read(self, name),
+ fwnode: self,
+ name,
+ }
+ }
+
+ /// helper used to display name or path of a fwnode
+ ///
+ /// # Safety
+ ///
+ /// Callers must provide a valid format string for a fwnode.
+ unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
+ let mut buf = [0; 256];
+ // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
+ // valid because `self` is valid.
+ let written = unsafe {
+ bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
+ };
+ // SAFETY: `written` is smaller or equal to `buf.len()`.
+ let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
+ write!(f, "{}", BStr::from_bytes(b))
+ }
+
+ /// Returns an object that implements [`Display`](core::fmt::Display) for
+ /// printing the name of a node.
+ pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
+ struct FwNodeDisplayName<'a>(&'a FwNode);
+
+ impl core::fmt::Display for FwNodeDisplayName<'_> {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ // SAFETY: "%pfwP" is a valid format string for fwnode
+ unsafe { self.0.fmt(f, c_str!("%pfwP")) }
+ }
+ }
+
+ FwNodeDisplayName(self)
+ }
+
+ /// Returns an object that implements [`Display`](core::fmt::Display) for
+ /// printing the full path of a node.
+ pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
+ struct FwNodeDisplayPath<'a>(&'a FwNode);
+
+ impl core::fmt::Display for FwNodeDisplayPath<'_> {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ // SAFETY: "%pfwf" is a valid format string for fwnode
+ unsafe { self.0.fmt(f, c_str!("%pfwf")) }
+ }
+ }
+
+ FwNodeDisplayPath(self)
+ }
}
// SAFETY: Instances of `FwNode` are always reference-counted.
@@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
}
}
+
+/// Implemented for several types that can be read as properties.
+///
+/// Informally, this is implemented for strings, integers and arrays of
+/// integers. It's used to make [`FwNode::property_read`] generic over the
+/// type of property being read. There are also two dedicated methods to read
+/// other types, because they require more specialized function signatures:
+/// - [`property_read_bool`](Device::property_read_bool)
+/// - [`property_read_array_vec`](Device::property_read_array_vec)
+pub trait Property: Sized {
+ /// Used to make [`FwNode::property_read`] generic.
+ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for CString {
+ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+ let mut str: *mut u8 = ptr::null_mut();
+ let pstr: *mut _ = &mut str;
+
+ // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
+ // valid because `fwnode` is valid.
+ let ret = unsafe {
+ bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
+ };
+ to_result(ret)?;
+
+ // SAFETY: `pstr` contains a non-null ptr on success
+ let str = unsafe { CStr::from_char_ptr(*pstr) };
+ Ok(str.try_into()?)
+ }
+}
+/// Implemented for all integers that can be read as properties.
+///
+/// This helper trait is needed on top of the existing [`Property`]
+/// trait to associate the integer types of various sizes with their
+/// corresponding `fwnode_property_read_*_array` functions.
+pub trait PropertyInt: Copy {
+ /// # Safety
+ ///
+ /// Callers must uphold the same safety invariants as for the various
+ /// `fwnode_property_read_*_array` functions.
+ unsafe fn read_array(
+ fwnode: *const bindings::fwnode_handle,
+ propname: *const ffi::c_char,
+ val: *mut Self,
+ nval: usize,
+ ) -> ffi::c_int;
+}
+// This macro generates implementations of the traits `Property` and
+// `PropertyInt` for integers of various sizes. Its input is a list
+// of pairs separated by commas. The first element of the pair is the
+// type of the integer, the second one is the name of its corresponding
+// `fwnode_property_read_*_array` function.
+macro_rules! impl_property_for_int {
+ ($($int:ty: $f:ident),* $(,)?) => { $(
+ impl PropertyInt for $int {
+ unsafe fn read_array(
+ fwnode: *const bindings::fwnode_handle,
+ propname: *const ffi::c_char,
+ val: *mut Self,
+ nval: usize,
+ ) -> ffi::c_int {
+ // SAFETY: The safety invariants on the trait require
+ // callers to uphold the invariants of the functions
+ // this macro is called with.
+ unsafe {
+ bindings::$f(fwnode, propname, val.cast(), nval)
+ }
+ }
+ }
+ )* };
+}
+impl_property_for_int! {
+ u8: fwnode_property_read_u8_array,
+ u16: fwnode_property_read_u16_array,
+ u32: fwnode_property_read_u32_array,
+ u64: fwnode_property_read_u64_array,
+ i8: fwnode_property_read_u8_array,
+ i16: fwnode_property_read_u16_array,
+ i32: fwnode_property_read_u32_array,
+ i64: fwnode_property_read_u64_array,
+}
+/// # Safety
+///
+/// Callers must ensure that if `len` is non-zero, `out_param` must be
+/// valid and point to memory that has enough space to hold at least
+/// `len` number of elements.
+unsafe fn read_array_out_param<T: PropertyInt>(
+ fwnode: &FwNode,
+ name: &CStr,
+ out_param: *mut T,
+ len: usize,
+) -> ffi::c_int {
+ // SAFETY: `name` is non-null and null-terminated.
+ // `fwnode.as_raw` is valid because `fwnode` is valid.
+ // `out_param` is valid and has enough space for at least
+ // `len` number of elements as per the safety requirement.
+ unsafe { T::read_array(fwnode.as_raw(), name.as_char_ptr(), out_param, len) }
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
+ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+ let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+ // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
+ // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
+ // because `MaybeUninit<T>` has the same memory layout as `T`.
+ let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
+ to_result(ret)?;
+
+ // SAFETY: `val` is always initialized when
+ // fwnode_property_read_<T>_array is successful.
+ Ok(val.map(|v| unsafe { v.assume_init() }))
+ }
+}
+impl<T: PropertyInt> Property for T {
+ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+ let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
+ Ok(val[0])
+ }
+}
+
+/// A helper for reading device properties.
+///
+/// Use [`Self::required`] if a missing property is considered a bug and
+/// [`Self::optional`] otherwise.
+///
+/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
+pub struct PropertyGuard<'fwnode, 'name, T> {
+ /// The result of reading the property.
+ inner: Result<T>,
+ /// The fwnode of the property, used for logging in the "required" case.
+ fwnode: &'fwnode FwNode,
+ /// The name of the property, used for logging in the "required" case.
+ name: &'name CStr,
+}
+
+impl<T> PropertyGuard<'_, '_, T> {
+ /// Access the property, indicating it is required.
+ ///
+ /// If the property is not present, the error is automatically logged. If a
+ /// missing property is not an error, use [`Self::optional`] instead.
+ pub fn required(self) -> Result<T> {
+ if self.inner.is_err() {
+ // Get the device associated with the fwnode for device-associated
+ // logging.
+ // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
+ // has a somewhat vague comment, which could mean we're not
+ // supposed to access it:
+ // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
+ // SAFETY: According to the invariant of FwNode, it is valid.
+ let dev = unsafe { (*self.fwnode.as_raw()).dev };
+
+ if dev.is_null() {
+ pr_err!(
+ "{}: property '{}' is missing\n",
+ self.fwnode.display_path(),
+ self.name
+ );
+ } else {
+ // SAFETY: If dev is not null, it points to a valid device.
+ let dev: &Device = unsafe { &*dev.cast() };
+ dev_err!(
+ dev,
+ "{}: property '{}' is missing\n",
+ self.fwnode.display_path(),
+ self.name
+ );
+ };
+ }
+ self.inner
+ }
+
+ /// Access the property, indicating it is optional.
+ ///
+ /// In contrast to [`Self::required`], no error message is logged if
+ /// the property is not present.
+ pub fn optional(self) -> Option<T> {
+ self.inner.ok()
+ }
+
+ /// Access the property or the specified default value.
+ ///
+ /// Do not pass a sentinel value as default to detect a missing property.
+ /// Use [`Self::required`] or [`Self::optional`] instead.
+ pub fn or(self, default: T) -> T {
+ self.inner.unwrap_or(default)
+ }
+}
+
+impl<T: Default> PropertyGuard<'_, '_, T> {
+ /// Access the property or a default value.
+ ///
+ /// Use [`Self::or`] to specify a custom default value.
+ pub fn or_default(self) -> T {
+ self.inner.unwrap_or_default()
+ }
+}
--
2.49.0Hi Remo,
On 14/04/2025 17:26, Remo Senekowitsch 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
> types of properties where appropriate.
>
> The PropertyGuard is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
>
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
> rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
....
> + /// Returns the value of firmware property `name`.
> + ///
> + /// This method is generic over the type of value to read. Informally,
> + /// the types that can be read are booleans, strings, unsigned integers and
> + /// arrays of unsigned integers.
> + ///
> + /// Reading a `KVec` of integers is done with the separate
> + /// method [`Self::property_read_array_vec`], because it takes an
> + /// additional `len` argument.
> + ///
> + /// When reading a boolean, this method never fails. A missing property
> + /// is interpreted as `false`, whereas a present property is interpreted
> + /// as `true`.
> + ///
> + /// For more precise documentation about what types can be read, see
> + /// the [implementors of Property][Property#implementors] and [its
> + /// implementations on foreign types][Property#foreign-impls].
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use crate::{device::Device, types::CString};
> + /// fn examples(dev: &Device) -> Result {
> + /// let fwnode = dev.fwnode();
> + /// let b: bool = fwnode.property_read("some-bool").required()?;
> + /// if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> + /// // ...
> + /// }
> + /// }
> + /// ```
With CONFIG_RUST_KERNEL_DOCTESTS=y I had to change this example to [1]
to make it buildable.
Best regards
Dirk
[1]
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 17ad176378206..bd43c910786d6 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -214,11 +214,10 @@ pub fn property_count_elem<T: PropertyInt>(&self,
name: &CStr) -> Result<usize>
/// # Examples
///
/// ```
- /// # use crate::{device::Device, types::CString};
- /// fn examples(dev: &Device) -> Result {
- /// let fwnode = dev.fwnode();
- /// let b: bool = fwnode.property_read("some-bool").required()?;
- /// if let Some(s) =
fwnode.property_read::<CString>("some-str").optional() {
+ /// # use kernel::{c_str, property::FwNode, str::CString};
+ /// fn examples(fwnode: &FwNode) -> () {
+ /// let b: bool = fwnode.property_read_bool(c_str!("some-bool"));
+ /// if let Some(s) =
fwnode.property_read::<CString>(c_str!("some-str")).optional() {
/// // ...
/// }
/// }
Hi Remo,
On 14/04/2025 17:26, Remo Senekowitsch 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
> types of properties where appropriate.
>
> The PropertyGuard is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
>
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
> rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
...
> + /// helper used to display name or path of a fwnode
> + ///
> + /// # Safety
> + ///
> + /// Callers must provide a valid format string for a fwnode.
> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> + let mut buf = [0; 256];
> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> + // valid because `self` is valid.
> + let written = unsafe {
> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> + };
> + // SAFETY: `written` is smaller or equal to `buf.len()`.
> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> + write!(f, "{}", BStr::from_bytes(b))
> + }
> +
> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> + /// printing the name of a node.
> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
I don't know about the details but with rustc 1.81 [1] I'm getting [2].
Just doing what is proposed seems to "fix" it:
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 3b1af034e902e..eadf7501d499b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -26,6 +26,7 @@
#![feature(const_mut_refs)]
#![feature(const_ptr_write)]
#![feature(const_refs_to_cell)]
+#![feature(precise_capturing)]
// Ensure conditional compilation based on the kernel configuration works;
// otherwise we may silently break things like initcall handling.
Just want to mention it because I think the minimal rustc version we
have to support is 1.78.
Best regards
Dirk
P.S.: Many thanks for working on this! :)
[1]
$ rustc --version
rustc 1.81.0 (eeb90cda1 2024-09-04)
[2]
error[E0658]: precise captures on `impl Trait` are experimental
--> rust/kernel/property.rs:256:61
|
256 | pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
| ^^^
|
= note: see issue #123432
<https://github.com/rust-lang/rust/issues/123432> for more information
= help: add `#![feature(precise_capturing)]` to the crate attributes
to enable
= note: this compiler was built on 2024-09-04; consider upgrading it
if it is out of date
error[E0658]: precise captures on `impl Trait` are experimental
--> rust/kernel/property.rs:271:61
|
271 | pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
| ^^^
|
= note: see issue #123432
<https://github.com/rust-lang/rust/issues/123432> for more information
= help: add `#![feature(precise_capturing)]` to the crate attributes
to enable
= note: this compiler was built on 2024-09-04; consider upgrading it
if it is out of date
error: aborting due to 2 previous errors
Hi Dirk,
On Wed Apr 23, 2025 at 2:29 PM CEST, Dirk Behme wrote:
> Hi Remo,
>
> On 14/04/2025 17:26, Remo Senekowitsch 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
>> types of properties where appropriate.
>>
>> The PropertyGuard is a way to force users to specify whether a property
>> is supposed to be required or not. This allows us to move error
>> logging of missing required properties into core, preventing a lot of
>> boilerplate in drivers.
>>
>> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> ---
>> rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 383 insertions(+), 2 deletions(-)
>>
>> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
>> index f6e6c980d..0d4ea3168 100644
>> --- a/rust/kernel/property.rs
>> +++ b/rust/kernel/property.rs
> ...
>> + /// helper used to display name or path of a fwnode
>> + ///
>> + /// # Safety
>> + ///
>> + /// Callers must provide a valid format string for a fwnode.
>> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
>> + let mut buf = [0; 256];
>> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
>> + // valid because `self` is valid.
>> + let written = unsafe {
>> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
>> + };
>> + // SAFETY: `written` is smaller or equal to `buf.len()`.
>> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
>> + write!(f, "{}", BStr::from_bytes(b))
>> + }
>> +
>> + /// Returns an object that implements [`Display`](core::fmt::Display) for
>> + /// printing the name of a node.
>> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
>
>
> I don't know about the details but with rustc 1.81 [1] I'm getting [2].
> Just doing what is proposed seems to "fix" it:
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 3b1af034e902e..eadf7501d499b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -26,6 +26,7 @@
> #![feature(const_mut_refs)]
> #![feature(const_ptr_write)]
> #![feature(const_refs_to_cell)]
> +#![feature(precise_capturing)]
>
> // Ensure conditional compilation based on the kernel configuration works;
> // otherwise we may silently break things like initcall handling.
>
> Just want to mention it because I think the minimal rustc version we
> have to support is 1.78.
Thanks for catching this! I'll make sure to compile with 1.78 from now
on. Luckily we don't need to activate an unstable feature, there is an
"older" capturing syntax that works here. (`'_` instead of `use<'_>`)
> Best regards
>
> Dirk
>
> P.S.: Many thanks for working on this! :)
>
> [1]
>
> $ rustc --version
> rustc 1.81.0 (eeb90cda1 2024-09-04)
>
> [2]
>
> error[E0658]: precise captures on `impl Trait` are experimental
> --> rust/kernel/property.rs:256:61
> |
> 256 | pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
> | ^^^
> |
> = note: see issue #123432
> <https://github.com/rust-lang/rust/issues/123432> for more information
> = help: add `#![feature(precise_capturing)]` to the crate attributes
> to enable
> = note: this compiler was built on 2024-09-04; consider upgrading it
> if it is out of date
>
> error[E0658]: precise captures on `impl Trait` are experimental
> --> rust/kernel/property.rs:271:61
> |
> 271 | pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
> | ^^^
> |
> = note: see issue #123432
> <https://github.com/rust-lang/rust/issues/123432> for more information
> = help: add `#![feature(precise_capturing)]` to the crate attributes
> to enable
> = note: this compiler was built on 2024-09-04; consider upgrading it
> if it is out of date
>
> error: aborting due to 2 previous errors
--
Best regards,
Remo
On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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
> types of properties where appropriate.
>
> The PropertyGuard is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
The patch adds a lot of thing, i.e.
* implement PropertyInt
* implement PropertyGuard
* extend FwNode by a lot of functions
* extend Device by some property functions
I see that from v1 a lot of things have been squashed, likely because there are
a few circular dependencies. Is there really no reasonable way to break this
down a bit?
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
> rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
> //!
> //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>
> -use core::ptr;
> +use core::{mem::MaybeUninit, ptr};
>
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> + alloc::KVec,
> + bindings, c_str,
> + device::Device,
> + error::{to_result, Result},
> + prelude::*,
> + str::{BStr, CStr, CString},
> + types::Opaque,
> +};
>
> impl Device {
> /// Obtain the fwnode corresponding to the device.
> @@ -28,6 +36,38 @@ fn fwnode(&self) -> &FwNode {
> pub fn property_present(&self, name: &CStr) -> bool {
> self.fwnode().property_present(name)
> }
> +
> + /// Returns firmware property `name` boolean value
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + self.fwnode().property_read_bool(name)
> + }
> +
> + /// Returns the index of matching string `match_str` for firmware string property `name`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + self.fwnode().property_match_string(name, match_str)
> + }
> +
> + /// Returns firmware property `name` integer array values in a KVec
> + pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
> + &'fwnode self,
> + name: &'name CStr,
> + len: usize,
> + ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
> + self.fwnode().property_read_array_vec(name, len)
> + }
> +
> + /// Returns integer array length for firmware property `name`
> + pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> + self.fwnode().property_count_elem::<T>(name)
> + }
> +
> + /// Returns firmware property `name` integer scalar value
> + pub fn property_read<'fwnode, 'name, T: Property>(
> + &'fwnode self,
> + name: &'name CStr,
> + ) -> PropertyGuard<'fwnode, 'name, T> {
> + self.fwnode().property_read(name)
> + }
> }
Okay, I start to see why you have all those Device functions in a separate file.
:)
I think we should move device.rs into its own module, i.e.
rust/kernel/device/mod.rs and then create rust/kernel/device/property.rs.
>
> /// A reference-counted fwnode_handle.
> @@ -59,6 +99,150 @@ pub fn property_present(&self, name: &CStr) -> bool {
> // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> }
> +
> + /// Returns firmware property `name` boolean value
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + // SAFETY: `name` is non-null and null-terminated. `self.as_raw()` is valid
> + // because `self` is valid.
> + unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
> + }
> +
> + /// Returns the index of matching string `match_str` for firmware string property `name`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
> + // valid because `self` is valid.
> + let ret = unsafe {
> + bindings::fwnode_property_match_string(
> + self.as_raw(),
> + name.as_char_ptr(),
> + match_str.as_char_ptr(),
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` integer array values in a KVec
> + pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
> + &'fwnode self,
> + name: &'name CStr,
> + len: usize,
> + ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
> + // didn't return an error and it has at least space for `len` number
> + // of elements.
> + let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
> + let res = if err < 0 {
> + Err(Error::from_errno(err))
> + } else {
> + // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
> + unsafe { val.set_len(len) }
> + Ok(val)
> + };
> + Ok(PropertyGuard {
> + inner: res,
> + fwnode: self,
> + name,
> + })
> + }
> +
> + /// Returns integer array length for firmware property `name`
> + pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> + // SAFETY: `out_param` is allowed to be null because `len` is zero.
> + let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns the value of firmware property `name`.
> + ///
> + /// This method is generic over the type of value to read. Informally,
> + /// the types that can be read are booleans, strings, unsigned integers and
> + /// arrays of unsigned integers.
> + ///
> + /// Reading a `KVec` of integers is done with the separate
> + /// method [`Self::property_read_array_vec`], because it takes an
> + /// additional `len` argument.
> + ///
> + /// When reading a boolean, this method never fails. A missing property
> + /// is interpreted as `false`, whereas a present property is interpreted
> + /// as `true`.
> + ///
> + /// For more precise documentation about what types can be read, see
> + /// the [implementors of Property][Property#implementors] and [its
> + /// implementations on foreign types][Property#foreign-impls].
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use crate::{device::Device, types::CString};
> + /// fn examples(dev: &Device) -> Result {
> + /// let fwnode = dev.fwnode();
> + /// let b: bool = fwnode.property_read("some-bool").required()?;
> + /// if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> + /// // ...
> + /// }
> + /// }
> + /// ```
> + pub fn property_read<'fwnode, 'name, T: Property>(
> + &'fwnode self,
> + name: &'name CStr,
> + ) -> PropertyGuard<'fwnode, 'name, T> {
> + PropertyGuard {
> + inner: T::read(self, name),
> + fwnode: self,
> + name,
> + }
> + }
> +
> + /// helper used to display name or path of a fwnode
> + ///
> + /// # Safety
> + ///
> + /// Callers must provide a valid format string for a fwnode.
> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> + let mut buf = [0; 256];
> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> + // valid because `self` is valid.
> + let written = unsafe {
> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> + };
Why do we need this? Can't we use write! right away?
> + // SAFETY: `written` is smaller or equal to `buf.len()`.
> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> + write!(f, "{}", BStr::from_bytes(b))
> + }
> +
> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> + /// printing the name of a node.
> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
> + struct FwNodeDisplayName<'a>(&'a FwNode);
> +
> + impl core::fmt::Display for FwNodeDisplayName<'_> {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + // SAFETY: "%pfwP" is a valid format string for fwnode
> + unsafe { self.0.fmt(f, c_str!("%pfwP")) }
> + }
> + }
> +
> + FwNodeDisplayName(self)
> + }
> +
> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> + /// printing the full path of a node.
> + pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
> + struct FwNodeDisplayPath<'a>(&'a FwNode);
> +
> + impl core::fmt::Display for FwNodeDisplayPath<'_> {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + // SAFETY: "%pfwf" is a valid format string for fwnode
> + unsafe { self.0.fmt(f, c_str!("%pfwf")) }
> + }
> + }
> +
> + FwNodeDisplayPath(self)
> + }
> }
>
> // SAFETY: Instances of `FwNode` are always reference-counted.
> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> }
> }
> +
> +/// Implemented for several types that can be read as properties.
> +///
> +/// Informally, this is implemented for strings, integers and arrays of
> +/// integers. It's used to make [`FwNode::property_read`] generic over the
> +/// type of property being read. There are also two dedicated methods to read
> +/// other types, because they require more specialized function signatures:
> +/// - [`property_read_bool`](Device::property_read_bool)
> +/// - [`property_read_array_vec`](Device::property_read_array_vec)
> +pub trait Property: Sized {
> + /// Used to make [`FwNode::property_read`] generic.
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> +}
> +
> +impl Property for CString {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let mut str: *mut u8 = ptr::null_mut();
> + let pstr: *mut _ = &mut str;
> +
> + // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
> + // valid because `fwnode` is valid.
> + let ret = unsafe {
> + bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
> + };
> + to_result(ret)?;
> +
> + // SAFETY: `pstr` contains a non-null ptr on success
> + let str = unsafe { CStr::from_char_ptr(*pstr) };
> + Ok(str.try_into()?)
> + }
> +}
I think it would be pretty weird to have a function CString::read() that takes a
FwNode argument, no? Same for all the other types below.
I assume you do this for
pub fn property_read<'fwnode, 'name, T: Property>(
&'fwnode self,
name: &'name CStr,
)
but given that you have to do the separate impls anyways, is there so much value
having the generic variant? You could still generate all the
property_read_{int}() variants with a macro.
If you really want a generic property_read(), I think you should create new
types instead and implement the Property trait for them instead.
> +/// Implemented for all integers that can be read as properties.
> +///
> +/// This helper trait is needed on top of the existing [`Property`]
> +/// trait to associate the integer types of various sizes with their
> +/// corresponding `fwnode_property_read_*_array` functions.
> +pub trait PropertyInt: Copy {
> + /// # Safety
> + ///
> + /// Callers must uphold the same safety invariants as for the various
> + /// `fwnode_property_read_*_array` functions.
> + unsafe fn read_array(
> + fwnode: *const bindings::fwnode_handle,
> + propname: *const ffi::c_char,
> + val: *mut Self,
> + nval: usize,
> + ) -> ffi::c_int;
> +}
> +// This macro generates implementations of the traits `Property` and
> +// `PropertyInt` for integers of various sizes. Its input is a list
> +// of pairs separated by commas. The first element of the pair is the
> +// type of the integer, the second one is the name of its corresponding
> +// `fwnode_property_read_*_array` function.
> +macro_rules! impl_property_for_int {
> + ($($int:ty: $f:ident),* $(,)?) => { $(
> + impl PropertyInt for $int {
> + unsafe fn read_array(
> + fwnode: *const bindings::fwnode_handle,
> + propname: *const ffi::c_char,
> + val: *mut Self,
> + nval: usize,
> + ) -> ffi::c_int {
> + // SAFETY: The safety invariants on the trait require
> + // callers to uphold the invariants of the functions
> + // this macro is called with.
> + unsafe {
> + bindings::$f(fwnode, propname, val.cast(), nval)
> + }
> + }
> + }
> + )* };
> +}
> +impl_property_for_int! {
> + u8: fwnode_property_read_u8_array,
> + u16: fwnode_property_read_u16_array,
> + u32: fwnode_property_read_u32_array,
> + u64: fwnode_property_read_u64_array,
> + i8: fwnode_property_read_u8_array,
> + i16: fwnode_property_read_u16_array,
> + i32: fwnode_property_read_u32_array,
> + i64: fwnode_property_read_u64_array,
> +}
> +/// # Safety
> +///
> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
> +/// valid and point to memory that has enough space to hold at least
> +/// `len` number of elements.
> +unsafe fn read_array_out_param<T: PropertyInt>(
> + fwnode: &FwNode,
> + name: &CStr,
> + out_param: *mut T,
> + len: usize,
> +) -> ffi::c_int {
> + // SAFETY: `name` is non-null and null-terminated.
> + // `fwnode.as_raw` is valid because `fwnode` is valid.
> + // `out_param` is valid and has enough space for at least
> + // `len` number of elements as per the safety requirement.
> + unsafe { T::read_array(fwnode.as_raw(), name.as_char_ptr(), out_param, len) }
> +}
> +impl<T: PropertyInt, const N: usize> Property for [T; N] {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
> +
> + // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
> + // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
> + // because `MaybeUninit<T>` has the same memory layout as `T`.
> + let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
> + to_result(ret)?;
> +
> + // SAFETY: `val` is always initialized when
> + // fwnode_property_read_<T>_array is successful.
> + Ok(val.map(|v| unsafe { v.assume_init() }))
> + }
> +}
> +impl<T: PropertyInt> Property for T {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
> + Ok(val[0])
> + }
> +}
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> + /// The result of reading the property.
> + inner: Result<T>,
> + /// The fwnode of the property, used for logging in the "required" case.
> + fwnode: &'fwnode FwNode,
> + /// The name of the property, used for logging in the "required" case.
> + name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> + /// Access the property, indicating it is required.
> + ///
> + /// If the property is not present, the error is automatically logged. If a
> + /// missing property is not an error, use [`Self::optional`] instead.
> + pub fn required(self) -> Result<T> {
> + if self.inner.is_err() {
> + // Get the device associated with the fwnode for device-associated
> + // logging.
> + // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
> + // has a somewhat vague comment, which could mean we're not
> + // supposed to access it:
> + // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
> + // SAFETY: According to the invariant of FwNode, it is valid.
> + let dev = unsafe { (*self.fwnode.as_raw()).dev };
I don't think this is valid it do, AFAICS, a firmware node handle doesn't own a
reference to the device pointer.
> +
> + if dev.is_null() {
> + pr_err!(
> + "{}: property '{}' is missing\n",
> + self.fwnode.display_path(),
> + self.name
> + );
> + } else {
> + // SAFETY: If dev is not null, it points to a valid device.
> + let dev: &Device = unsafe { &*dev.cast() };
> + dev_err!(
> + dev,
> + "{}: property '{}' is missing\n",
> + self.fwnode.display_path(),
> + self.name
> + );
> + };
> + }
> + self.inner
> + }
> +
> + /// Access the property, indicating it is optional.
> + ///
> + /// In contrast to [`Self::required`], no error message is logged if
> + /// the property is not present.
> + pub fn optional(self) -> Option<T> {
> + self.inner.ok()
> + }
> +
> + /// Access the property or the specified default value.
> + ///
> + /// Do not pass a sentinel value as default to detect a missing property.
> + /// Use [`Self::required`] or [`Self::optional`] instead.
> + pub fn or(self, default: T) -> T {
> + self.inner.unwrap_or(default)
> + }
> +}
> +
> +impl<T: Default> PropertyGuard<'_, '_, T> {
> + /// Access the property or a default value.
> + ///
> + /// Use [`Self::or`] to specify a custom default value.
> + pub fn or_default(self) -> T {
> + self.inner.unwrap_or_default()
> + }
> +}
> --
> 2.49.0
>
On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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
>> types of properties where appropriate.
>>
>> The PropertyGuard is a way to force users to specify whether a property
>> is supposed to be required or not. This allows us to move error
>> logging of missing required properties into core, preventing a lot of
>> boilerplate in drivers.
>
> The patch adds a lot of thing, i.e.
> * implement PropertyInt
> * implement PropertyGuard
> * extend FwNode by a lot of functions
> * extend Device by some property functions
>
> I see that from v1 a lot of things have been squashed, likely because there are
> a few circular dependencies. Is there really no reasonable way to break this
> down a bit?
I was explicitly asked to do this in the previous thread[1]. I'm happy
to invest time into organizing files and commits exactly the way people
want, but squashing and splitting the same commits back and forth
between subsequent patch series is a waste of my time.
Do reviewers not typically read the review comments of others as well?
What can I do to avoid this situation and make progress instead of
running in circles?
Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
>> + /// helper used to display name or path of a fwnode
>> + ///
>> + /// # Safety
>> + ///
>> + /// Callers must provide a valid format string for a fwnode.
>> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
>> + let mut buf = [0; 256];
>> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
>> + // valid because `self` is valid.
>> + let written = unsafe {
>> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
>> + };
>
> Why do we need this? Can't we use write! right away?
I don't know how, can you be more specific? I'm not too familiar with
how these formatting specifiers work under the hood, but on the face of
it, Rust and C seem very different.
>> + // SAFETY: `written` is smaller or equal to `buf.len()`.
>> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
>> + write!(f, "{}", BStr::from_bytes(b))
>> + }
>> +
>> + /// Returns an object that implements [`Display`](core::fmt::Display) for
>> + /// printing the name of a node.
>> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
>> + struct FwNodeDisplayName<'a>(&'a FwNode);
>> +
>> + impl core::fmt::Display for FwNodeDisplayName<'_> {
>> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> + // SAFETY: "%pfwP" is a valid format string for fwnode
>> + unsafe { self.0.fmt(f, c_str!("%pfwP")) }
>> + }
>> + }
>> +
>> + FwNodeDisplayName(self)
>> + }
>> +
>> + /// Returns an object that implements [`Display`](core::fmt::Display) for
>> + /// printing the full path of a node.
>> + pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
>> + struct FwNodeDisplayPath<'a>(&'a FwNode);
>> +
>> + impl core::fmt::Display for FwNodeDisplayPath<'_> {
>> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> + // SAFETY: "%pfwf" is a valid format string for fwnode
>> + unsafe { self.0.fmt(f, c_str!("%pfwf")) }
>> + }
>> + }
>> +
>> + FwNodeDisplayPath(self)
>> + }
>> }
>>
>> // SAFETY: Instances of `FwNode` are always reference-counted.
>> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>> }
>> }
>> +
>> +/// Implemented for several types that can be read as properties.
>> +///
>> +/// Informally, this is implemented for strings, integers and arrays of
>> +/// integers. It's used to make [`FwNode::property_read`] generic over the
>> +/// type of property being read. There are also two dedicated methods to read
>> +/// other types, because they require more specialized function signatures:
>> +/// - [`property_read_bool`](Device::property_read_bool)
>> +/// - [`property_read_array_vec`](Device::property_read_array_vec)
>> +pub trait Property: Sized {
>> + /// Used to make [`FwNode::property_read`] generic.
>> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
>> +}
>> +
>> +impl Property for CString {
>> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
>> + let mut str: *mut u8 = ptr::null_mut();
>> + let pstr: *mut _ = &mut str;
>> +
>> + // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
>> + // valid because `fwnode` is valid.
>> + let ret = unsafe {
>> + bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
>> + };
>> + to_result(ret)?;
>> +
>> + // SAFETY: `pstr` contains a non-null ptr on success
>> + let str = unsafe { CStr::from_char_ptr(*pstr) };
>> + Ok(str.try_into()?)
>> + }
>> +}
>
> I think it would be pretty weird to have a function CString::read() that takes a
> FwNode argument, no? Same for all the other types below.
>
> I assume you do this for
>
> pub fn property_read<'fwnode, 'name, T: Property>(
> &'fwnode self,
> name: &'name CStr,
> )
>
> but given that you have to do the separate impls anyways, is there so much value
> having the generic variant? You could still generate all the
> property_read_{int}() variants with a macro.
>
> If you really want a generic property_read(), I think you should create new
> types instead and implement the Property trait for them instead.
Yeah, that would be workable. On the other hand, it's not unusual in
Rust to implement traits on foreign types, right? If the problem is
the non-descriptive name "read" then we can change it to something more
verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
meant to be used directly, a verbose name wouldn't cause any damage.
I think making the function generic can be nice to work with, especially
if type inference is working. But I'm fine with either.
On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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
> >> types of properties where appropriate.
> >>
> >> The PropertyGuard is a way to force users to specify whether a property
> >> is supposed to be required or not. This allows us to move error
> >> logging of missing required properties into core, preventing a lot of
> >> boilerplate in drivers.
> >
> > The patch adds a lot of thing, i.e.
> > * implement PropertyInt
> > * implement PropertyGuard
> > * extend FwNode by a lot of functions
> > * extend Device by some property functions
> >
> > I see that from v1 a lot of things have been squashed, likely because there are
> > a few circular dependencies. Is there really no reasonable way to break this
> > down a bit?
>
> I was explicitly asked to do this in the previous thread[1].
I'm well aware that you were asked to do so and that one reason was that
subsequent patches started deleting code that was added in previous ones
(hence my suspicion of circular dependencies and that splitting up things might
not be super trivial).
> I'm happy
> to invest time into organizing files and commits exactly the way people
> want, but squashing and splitting the same commits back and forth
> between subsequent patch series is a waste of my time.
I don't think you were asked to go back and forth, but whether you see a
reasonable way to break things down a bit, where "reasonable" means without
deleting code that was just added.
> Do reviewers not typically read the review comments of others as well?
I think mostly they do, but maintainers and reviewers are rather busy people.
So, I don't think you can expect everyone to follow every thread, especially
when they get lengthy.
> What can I do to avoid this situation and make progress instead of
> running in circles?
I suggest to investigate whether it can be split it up in a reasonable way and
subsequently answer the question.
With your contribution you attempt to add a rather large portion of pretty core
code. This isn't an easy task and quite some discussion is totally expected;
please don't get frustrated, the series goes pretty well. :)
>
> Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
>
> >> + /// helper used to display name or path of a fwnode
> >> + ///
> >> + /// # Safety
> >> + ///
> >> + /// Callers must provide a valid format string for a fwnode.
> >> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> >> + let mut buf = [0; 256];
> >> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> >> + // valid because `self` is valid.
> >> + let written = unsafe {
> >> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> >> + };
> >
> > Why do we need this? Can't we use write! right away?
>
> I don't know how, can you be more specific? I'm not too familiar with
> how these formatting specifiers work under the hood, but on the face of
> it, Rust and C seem very different.
See below.
>
> >> + // SAFETY: `written` is smaller or equal to `buf.len()`.
> >> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> >> + write!(f, "{}", BStr::from_bytes(b))
> >> + }
> >> +
> >> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> >> + /// printing the name of a node.
> >> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
> >> + struct FwNodeDisplayName<'a>(&'a FwNode);
> >> +
> >> + impl core::fmt::Display for FwNodeDisplayName<'_> {
> >> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> >> + // SAFETY: "%pfwP" is a valid format string for fwnode
> >> + unsafe { self.0.fmt(f, c_str!("%pfwP")) }
I think this could just use write!() and fwnode_get_name(), right?
> >> + }
> >> + }
> >> +
> >> + FwNodeDisplayName(self)
> >> + }
> >> +
> >> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> >> + /// printing the full path of a node.
> >> + pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
> >> + struct FwNodeDisplayPath<'a>(&'a FwNode);
> >> +
> >> + impl core::fmt::Display for FwNodeDisplayPath<'_> {
> >> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> >> + // SAFETY: "%pfwf" is a valid format string for fwnode
> >> + unsafe { self.0.fmt(f, c_str!("%pfwf")) }
This one is indeed a bit more tricky, because it comes from
fwnode_full_name_string() in lib/vsprintf.c.
Maybe it would be better to replicate the loop within fwnode_full_name_string()
and call write! from there.
> >> + }
> >> + }
> >> +
> >> + FwNodeDisplayPath(self)
> >> + }
> >> }
> >>
> >> // SAFETY: Instances of `FwNode` are always reference-counted.
> >> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >> unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >> }
> >> }
> >> +
> >> +/// Implemented for several types that can be read as properties.
> >> +///
> >> +/// Informally, this is implemented for strings, integers and arrays of
> >> +/// integers. It's used to make [`FwNode::property_read`] generic over the
> >> +/// type of property being read. There are also two dedicated methods to read
> >> +/// other types, because they require more specialized function signatures:
> >> +/// - [`property_read_bool`](Device::property_read_bool)
> >> +/// - [`property_read_array_vec`](Device::property_read_array_vec)
> >> +pub trait Property: Sized {
> >> + /// Used to make [`FwNode::property_read`] generic.
> >> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> >> +}
> >> +
> >> +impl Property for CString {
> >> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> >> + let mut str: *mut u8 = ptr::null_mut();
> >> + let pstr: *mut _ = &mut str;
> >> +
> >> + // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
> >> + // valid because `fwnode` is valid.
> >> + let ret = unsafe {
> >> + bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
> >> + };
> >> + to_result(ret)?;
> >> +
> >> + // SAFETY: `pstr` contains a non-null ptr on success
> >> + let str = unsafe { CStr::from_char_ptr(*pstr) };
> >> + Ok(str.try_into()?)
> >> + }
> >> +}
> >
> > I think it would be pretty weird to have a function CString::read() that takes a
> > FwNode argument, no? Same for all the other types below.
> >
> > I assume you do this for
> >
> > pub fn property_read<'fwnode, 'name, T: Property>(
> > &'fwnode self,
> > name: &'name CStr,
> > )
> >
> > but given that you have to do the separate impls anyways, is there so much value
> > having the generic variant? You could still generate all the
> > property_read_{int}() variants with a macro.
> >
> > If you really want a generic property_read(), I think you should create new
> > types instead and implement the Property trait for them instead.
>
> Yeah, that would be workable. On the other hand, it's not unusual in
> Rust to implement traits on foreign types, right? If the problem is
> the non-descriptive name "read" then we can change it to something more
> verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
> meant to be used directly, a verbose name wouldn't cause any damage.
Yeah, if we keep this approach, I'd prefer a more descriptive name.
However, I'd like to hear some more opinions from other members of the Rust team
on this one.
On Tue, 15 Apr 2025 11:48:07 +0200
Danilo Krummrich <dakr@kernel.org> wrote:
> On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> > On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> > > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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
> > >> types of properties where appropriate.
> > >>
> > >> The PropertyGuard is a way to force users to specify whether a property
> > >> is supposed to be required or not. This allows us to move error
> > >> logging of missing required properties into core, preventing a lot of
> > >> boilerplate in drivers.
> > >
> > > The patch adds a lot of thing, i.e.
> > > * implement PropertyInt
> > > * implement PropertyGuard
> > > * extend FwNode by a lot of functions
> > > * extend Device by some property functions
> > >
> > > I see that from v1 a lot of things have been squashed, likely because there are
> > > a few circular dependencies. Is there really no reasonable way to break this
> > > down a bit?
> >
> > I was explicitly asked to do this in the previous thread[1].
>
> I'm well aware that you were asked to do so and that one reason was that
> subsequent patches started deleting code that was added in previous ones
> (hence my suspicion of circular dependencies and that splitting up things might
> not be super trivial).
>
> > I'm happy
> > to invest time into organizing files and commits exactly the way people
> > want, but squashing and splitting the same commits back and forth
> > between subsequent patch series is a waste of my time.
>
> I don't think you were asked to go back and forth, but whether you see a
> reasonable way to break things down a bit, where "reasonable" means without
> deleting code that was just added.
>
> > Do reviewers not typically read the review comments of others as well?
>
> I think mostly they do, but maintainers and reviewers are rather busy people.
> So, I don't think you can expect everyone to follow every thread, especially
> when they get lengthy.
>
> > What can I do to avoid this situation and make progress instead of
> > running in circles?
>
> I suggest to investigate whether it can be split it up in a reasonable way and
> subsequently answer the question.
>
> With your contribution you attempt to add a rather large portion of pretty core
> code. This isn't an easy task and quite some discussion is totally expected;
> please don't get frustrated, the series goes pretty well. :)
>
> >
> > Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
> >
> > >> + /// helper used to display name or path of a fwnode
> > >> + ///
> > >> + /// # Safety
> > >> + ///
> > >> + /// Callers must provide a valid format string for a fwnode.
> > >> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> > >> + let mut buf = [0; 256];
> > >> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> > >> + // valid because `self` is valid.
> > >> + let written = unsafe {
> > >> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> > >> + };
> > >
> > > Why do we need this? Can't we use write! right away?
> >
> > I don't know how, can you be more specific? I'm not too familiar with
> > how these formatting specifiers work under the hood, but on the face of
> > it, Rust and C seem very different.
>
> See below.
>
> >
> > >> + // SAFETY: `written` is smaller or equal to `buf.len()`.
> > >> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> > >> + write!(f, "{}", BStr::from_bytes(b))
> > >> + }
> > >> +
> > >> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> > >> + /// printing the name of a node.
> > >> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
> > >> + struct FwNodeDisplayName<'a>(&'a FwNode);
> > >> +
> > >> + impl core::fmt::Display for FwNodeDisplayName<'_> {
> > >> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> > >> + // SAFETY: "%pfwP" is a valid format string for fwnode
> > >> + unsafe { self.0.fmt(f, c_str!("%pfwP")) }
>
> I think this could just use write!() and fwnode_get_name(), right?
>
> > >> + }
> > >> + }
> > >> +
> > >> + FwNodeDisplayName(self)
> > >> + }
> > >> +
> > >> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> > >> + /// printing the full path of a node.
> > >> + pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
> > >> + struct FwNodeDisplayPath<'a>(&'a FwNode);
> > >> +
> > >> + impl core::fmt::Display for FwNodeDisplayPath<'_> {
> > >> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> > >> + // SAFETY: "%pfwf" is a valid format string for fwnode
> > >> + unsafe { self.0.fmt(f, c_str!("%pfwf")) }
>
> This one is indeed a bit more tricky, because it comes from
> fwnode_full_name_string() in lib/vsprintf.c.
>
> Maybe it would be better to replicate the loop within fwnode_full_name_string()
> and call write! from there.
>
> > >> + }
> > >> + }
> > >> +
> > >> + FwNodeDisplayPath(self)
> > >> + }
> > >> }
> > >>
> > >> // SAFETY: Instances of `FwNode` are always reference-counted.
> > >> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> > >> unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> > >> }
> > >> }
> > >> +
> > >> +/// Implemented for several types that can be read as properties.
> > >> +///
> > >> +/// Informally, this is implemented for strings, integers and arrays of
> > >> +/// integers. It's used to make [`FwNode::property_read`] generic over the
> > >> +/// type of property being read. There are also two dedicated methods to read
> > >> +/// other types, because they require more specialized function signatures:
> > >> +/// - [`property_read_bool`](Device::property_read_bool)
> > >> +/// - [`property_read_array_vec`](Device::property_read_array_vec)
> > >> +pub trait Property: Sized {
> > >> + /// Used to make [`FwNode::property_read`] generic.
> > >> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> > >> +}
> > >> +
> > >> +impl Property for CString {
> > >> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> > >> + let mut str: *mut u8 = ptr::null_mut();
> > >> + let pstr: *mut _ = &mut str;
> > >> +
> > >> + // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
> > >> + // valid because `fwnode` is valid.
> > >> + let ret = unsafe {
> > >> + bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
> > >> + };
> > >> + to_result(ret)?;
> > >> +
> > >> + // SAFETY: `pstr` contains a non-null ptr on success
> > >> + let str = unsafe { CStr::from_char_ptr(*pstr) };
> > >> + Ok(str.try_into()?)
> > >> + }
> > >> +}
> > >
> > > I think it would be pretty weird to have a function CString::read() that takes a
> > > FwNode argument, no? Same for all the other types below.
> > >
> > > I assume you do this for
> > >
> > > pub fn property_read<'fwnode, 'name, T: Property>(
> > > &'fwnode self,
> > > name: &'name CStr,
> > > )
> > >
> > > but given that you have to do the separate impls anyways, is there so much value
> > > having the generic variant? You could still generate all the
> > > property_read_{int}() variants with a macro.
> > >
> > > If you really want a generic property_read(), I think you should create new
> > > types instead and implement the Property trait for them instead.
> >
> > Yeah, that would be workable. On the other hand, it's not unusual in
> > Rust to implement traits on foreign types, right? If the problem is
> > the non-descriptive name "read" then we can change it to something more
> > verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
> > meant to be used directly, a verbose name wouldn't cause any damage.
>
> Yeah, if we keep this approach, I'd prefer a more descriptive name.
>
> However, I'd like to hear some more opinions from other members of the Rust team
> on this one.
I think the trait approach is what people quite typically use. The
additional functions are usually not an issue, as they won't be
available if you don't import the trait. Using names like
`read_from_fwnode_property` would be remove any concern for me.
Best,
Gary
On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote: > On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote: >> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote: >> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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 >> >> types of properties where appropriate. >> >> >> >> The PropertyGuard is a way to force users to specify whether a property >> >> is supposed to be required or not. This allows us to move error >> >> logging of missing required properties into core, preventing a lot of >> >> boilerplate in drivers. >> > >> > The patch adds a lot of thing, i.e. >> > * implement PropertyInt >> > * implement PropertyGuard >> > * extend FwNode by a lot of functions >> > * extend Device by some property functions >> > >> > I see that from v1 a lot of things have been squashed, likely because there are >> > a few circular dependencies. Is there really no reasonable way to break this >> > down a bit? >> >> I was explicitly asked to do this in the previous thread[1]. > > I'm well aware that you were asked to do so and that one reason was that > subsequent patches started deleting code that was added in previous ones > (hence my suspicion of circular dependencies and that splitting up things might > not be super trivial). > >> I'm happy >> to invest time into organizing files and commits exactly the way people >> want, but squashing and splitting the same commits back and forth >> between subsequent patch series is a waste of my time. > > I don't think you were asked to go back and forth, but whether you see a > reasonable way to break things down a bit, where "reasonable" means without > deleting code that was just added. I was asked to squash two specific commits. The first was making the read method generic. That was the one that deleted much code. Totally reasonable, and the generic stuff might be discarded anyway, so I won't be moving stuff back and forth. However, the second commit was the one introducing PropertyGuard. That was a beautifully atomic commit, no circular dependencies in sight. If this commit is to be split up, one of the smaller ones would without doubt look exactly the same as the one before. I provided a link[1] to the exact email telling me to squash that exact patch to avoid any confusion. >> Do reviewers not typically read the review comments of others as well? > > I think mostly they do, but maintainers and reviewers are rather busy people. > So, I don't think you can expect everyone to follow every thread, especially > when they get lengthy. > >> What can I do to avoid this situation and make progress instead of >> running in circles? > > I suggest to investigate whether it can be split it up in a reasonable way and > subsequently answer the question. The point is that I agree with you that the PropertyGuard patch can be split out. And possibly more: I think a reasonable person could make a separate commit for every `property_read_<type>` method. And I'm happy to do that if it's the way to go. But if I do that, send a v3 and then someone else asks me to squash it again (because they didn't read this exchange between you and me...) then I would've wasted my time. > With your contribution you attempt to add a rather large portion of pretty core > code. This isn't an easy task and quite some discussion is totally expected; > please don't get frustrated, the series goes pretty well. :) I'm trying, but I can't say I have a good first impression of doing code review over a mailing list. Doing open-heart surgery with a pickfork and writing documents in Microsoft Word both seem like more productive and enjoyable activities to me. Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
On Tue, Apr 15, 2025 at 6:11 AM Remo Senekowitsch <remo@buenzli.dev> wrote: > > On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote: > > On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote: > >> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote: > >> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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 > >> >> types of properties where appropriate. > >> >> > >> >> The PropertyGuard is a way to force users to specify whether a property > >> >> is supposed to be required or not. This allows us to move error > >> >> logging of missing required properties into core, preventing a lot of > >> >> boilerplate in drivers. > >> > > >> > The patch adds a lot of thing, i.e. > >> > * implement PropertyInt > >> > * implement PropertyGuard > >> > * extend FwNode by a lot of functions > >> > * extend Device by some property functions > >> > > >> > I see that from v1 a lot of things have been squashed, likely because there are > >> > a few circular dependencies. Is there really no reasonable way to break this > >> > down a bit? > >> > >> I was explicitly asked to do this in the previous thread[1]. > > > > I'm well aware that you were asked to do so and that one reason was that > > subsequent patches started deleting code that was added in previous ones > > (hence my suspicion of circular dependencies and that splitting up things might > > not be super trivial). > > > >> I'm happy > >> to invest time into organizing files and commits exactly the way people > >> want, but squashing and splitting the same commits back and forth > >> between subsequent patch series is a waste of my time. > > > > I don't think you were asked to go back and forth, but whether you see a > > reasonable way to break things down a bit, where "reasonable" means without > > deleting code that was just added. > > I was asked to squash two specific commits. The first was making the > read method generic. That was the one that deleted much code. Totally > reasonable, and the generic stuff might be discarded anyway, so I won't > be moving stuff back and forth. > > However, the second commit was the one introducing PropertyGuard. That > was a beautifully atomic commit, no circular dependencies in sight. If > this commit is to be split up, one of the smaller ones would without > doubt look exactly the same as the one before. I provided a link[1] > to the exact email telling me to squash that exact patch to avoid any > confusion. Adding PropertyGuard changes the API. I don't think it makes sense to review the API without it and then again with it. It might be reasonable to split adding the fwnode bindings and then the Device version. But those are in separate files anyway, so it's not really making review easier. > >> Do reviewers not typically read the review comments of others as well? > > > > I think mostly they do, but maintainers and reviewers are rather busy people. > > So, I don't think you can expect everyone to follow every thread, especially > > when they get lengthy. Please understand maintainers are reviewing 10s to 100s of patches a week. I don't necessarily remember my own comments from last week. > >> What can I do to avoid this situation and make progress instead of > >> running in circles? > > > > I suggest to investigate whether it can be split it up in a reasonable way and > > subsequently answer the question. > > The point is that I agree with you that the PropertyGuard patch can be > split out. And possibly more: I think a reasonable person could make a > separate commit for every `property_read_<type>` method. And I'm happy > to do that if it's the way to go. But if I do that, send a v3 and then > someone else asks me to squash it again (because they didn't read this > exchange between you and me...) then I would've wasted my time. A commit per method really seems excessive to me. I prefer to look at the API as a whole, and to do that with separate patches only makes that harder. A reasonable split here is possibly splitting the fwnode and the Device versions of the API. In any case, I think we've discussed this enough and I don't care to discuss it more, so whatever reasonable split you come up with is fine with me. > > With your contribution you attempt to add a rather large portion of pretty core > > code. This isn't an easy task and quite some discussion is totally expected; > > please don't get frustrated, the series goes pretty well. :) > > I'm trying, but I can't say I have a good first impression of doing > code review over a mailing list. Doing open-heart surgery with a > pickfork and writing documents in Microsoft Word both seem like more > productive and enjoyable activities to me. Mailing lists are a good scapegoat, but you can have 2 reviewers with 2 different opinions anywhere. That's just the nature of a large, distributed project with lots of reviewers. Rob
On Tue Apr 15, 2025 at 2:46 PM CEST, Rob Herring wrote: > > Adding PropertyGuard changes the API. I don't think it makes sense to > review the API without it and then again with it. We could add the PropertyGuard first and introduce the users later. Then they are in separate patches and the API doesn't change.
On Tue, Apr 15, 2025 at 07:46:19AM -0500, Rob Herring wrote: > > A reasonable split here is possibly splitting the fwnode and the > Device versions of the API. In any case, I think we've discussed this > enough and I don't care to discuss it more, so whatever reasonable > split you come up with is fine with me. +1 Let's wait for some more feedback on the Property trait impl for primitives, CString, etc. before moving on.
On Mon, Apr 14, 2025 at 07:44:36PM +0200, Danilo Krummrich wrote: > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch 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 > > types of properties where appropriate. > > > > The PropertyGuard is a way to force users to specify whether a property > > is supposed to be required or not. This allows us to move error > > logging of missing required properties into core, preventing a lot of > > boilerplate in drivers. > > The patch adds a lot of thing, i.e. > * implement PropertyInt I meant the Property trait and all its impls of course. :) > * implement PropertyGuard > * extend FwNode by a lot of functions > * extend Device by some property functions
© 2016 - 2025 Red Hat, Inc.