Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Dirk Behme <dirk.behme@gmail.com>]

On 26.04.25 12:15, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>> On 25.04.25 17:35, Danilo Krummrich wrote:
>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>>>> This abstraction 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.
>>>>
>>>> It will be used by upcoming methods for reading device properties.
>>>>
>>>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>>>> ---
>>>>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 57 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>>>> index 28850aa3b..de31a1f56 100644
>>>> --- a/rust/kernel/device/property.rs
>>>> +++ b/rust/kernel/device/property.rs
>>>> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>>>      }
>>>>  }
>>>> +
>>>> +/// 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() {
>>>> +            pr_err!(
>>>> +                "{}: property '{}' is missing\n",
>>>> +                self.fwnode.display_path(),
>>>> +                self.name
>>>> +            );
>>>
>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
>>> guaranteed to be valid, hence the pr_*() print...
>>>
>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
>>> that it is unhappy about a missing required property it can do so by itself, I
>>> think.
>>
>> Hmm, the driver said by using 'required' that it *is* required. So a
>> missing property is definitely an error here. Else it would have used
>> 'optional'. Which doesn't print in case the property is missing.
>>
>> If I remember correctly having 'required' and 'optional' is the result
>> of some discussion on Zulip. And one conclusion of that discussion was
>> to move checking & printing the error out of the individual drivers
>> into a central place to avoid this error checking & printing in each
>> and every driver. I think the idea is that the drivers just have to do
>> ...required()?; and that's it, then.
> 
> Yes, I get the idea.
> 
> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> case. But this code is used by drivers from probe(), hence printing the error
> without saying for which device it did occur is a bit pointless.

Thinking a little about this, yes, we don't know the device here. But:
Does the device matter here? There is nothing wrong with the (unknown)
device, no? What is wrong here is the firmware (node). It misses
something. And this is exactly what the message tells: "There is an
error due to the missing node 'name' in 'path', please fix it". That
should be sufficient to identify the firmware/device tree description
and fix it.

I can still follow Rob's proposal on doing the printing in 'core' :)

Thanks,

Dirk