Message-Id: <D96RNFS3N8L2.33MSG7T019UQM@buenzli.dev>
Date: Tue, 15 Apr 2025 01:55:42 +0200
From: "Remo Senekowitsch" <remo@...nzli.dev>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Rob Herring" <robh@...nel.org>, "Saravana Kannan"
 <saravanak@...gle.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

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.

Powered by blists - more mailing lists