Re: [PATCH RFC 2/3] rust: Add bindings for device properties

[Dirk Behme <dirk.behme@de.bosch.com>]

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!(