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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0756503c-02e7-477a-9e89-e7d4881c8ce6@gmail.com>
Date: Sun, 27 Apr 2025 08:11:58 +0200
From: Dirk Behme <dirk.behme@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Remo Senekowitsch <remo@...nzli.dev>, 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>, Dirk Behme
 <dirk.behme@...bosch.com>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

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@...nzli.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