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
+            );
+        }
+        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 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.

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.

Best regards

Dirk

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.

Drivers can still decide to properly print the error if the returned Result
indicates one.

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

On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> 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:
> >>>> +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?

If the above fails it means that for a (specific) device a driver expects that
a specific property of some firmware node is present. So, yes, I think it does
matter.

> There is nothing wrong with the (unknown)
> device, no? What is wrong here is the firmware (node). It misses
> something.

How do we know the firmware node is wrong? Maybe the driver has wrong
expectations for this device?

> 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 think we can't always fix them, even if they're wrong. How do we fix ACPI
firmware nodes for instance?

(Software nodes provide a solution for that, see also commit 59abd83672f7
("drivers: base: Introducing software nodes to the firmware node framework").)

On 27/04/2025 14:23, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
>> 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:
>>>>>> +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?
> 
> If the above fails it means that for a (specific) device a driver expects that
> a specific property of some firmware node is present. So, yes, I think it does
> matter.
> 
>> There is nothing wrong with the (unknown)
>> device, no? What is wrong here is the firmware (node). It misses
>> something.
> 
> How do we know the firmware node is wrong? Maybe the driver has wrong
> expectations for this device?
> 
>> 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 think we can't always fix them, even if they're wrong. How do we fix ACPI
> firmware nodes for instance?

So the argument here is that the device (driver) is expecting something
to be "required" is wrong and might need to be fixed. Not the firmware.
Yes, ok, that is a valid argument. I have a device tree background and
there in 99% of the cases the device tree needs a fix ;)

But let me ask the other way around, then: What will it hurt or break if
we keep the pr_err() like Remo did? Even knowing that its not perfect?
But knowing that it will give at least a note that something is wrong
with at least a starting point for searching what needs to be fixed. I
mean even if we don't get the device, we will get the affected node we
can search for which device uses it as "required".

Could we somehow agree that in 90% of the cases this should be catched
at device (driver) development time, already? And therefore it should be
beneficial if we don't require each and every driver to be "bloated"
with checking this individually?

Dirk

On Mon, Apr 28, 2025 at 07:03:07AM +0200, Dirk Behme wrote:
> On 27/04/2025 14:23, Danilo Krummrich wrote:
> > On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> >> 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:
> >>>>>> +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?
> > 
> > If the above fails it means that for a (specific) device a driver expects that
> > a specific property of some firmware node is present. So, yes, I think it does
> > matter.
> > 
> >> There is nothing wrong with the (unknown)
> >> device, no? What is wrong here is the firmware (node). It misses
> >> something.
> > 
> > How do we know the firmware node is wrong? Maybe the driver has wrong
> > expectations for this device?
> > 
> >> 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 think we can't always fix them, even if they're wrong. How do we fix ACPI
> > firmware nodes for instance?
> 
> So the argument here is that the device (driver) is expecting something
> to be "required" is wrong and might need to be fixed. Not the firmware.
> Yes, ok, that is a valid argument. I have a device tree background and
> there in 99% of the cases the device tree needs a fix ;)
> 
> But let me ask the other way around, then: What will it hurt or break if
> we keep the pr_err() like Remo did? Even knowing that its not perfect?
> But knowing that it will give at least a note that something is wrong
> with at least a starting point for searching what needs to be fixed. I
> mean even if we don't get the device, we will get the affected node we
> can search for which device uses it as "required".
> 
> Could we somehow agree that in 90% of the cases this should be catched
> at device (driver) development time, already?

I don't see why *catching* such errors needs pr_err() in core code; without it
you still get a Result as return value that you need to handle in some way.

> And therefore it should be
> beneficial if we don't require each and every driver to be "bloated"
> with checking this individually?

I guess you mean "bloated with *printing* this individually", rather than
"checking".

This is where we disagree: I think it is "bloating" the core kernel instead if
we start adding error prints to core code, where a proper error code is
propagated up to the driver.

I did say that I would agree to a certain extend with this specific one if we
could print it properly, since it is designed to leave no doubt that returning
an error code from required() is fatal for the driver. But I'm not even sure
about this anymore.

I still haven't read a reason why this one is so crucial to print from core
code, while for other things that are always fatal (e.g. request_irq()) we
don't.

However, if you really think we need a common helper that prints something in
the error case, maybe we can add an *additional* helper

	pub fn required_by(self, dev: &Device) -> Result<T>

and document that it is the same as required(), with an additional error print
in case of failure for the given device.

- Danilo

On Mon, Apr 28, 2025 at 06:09:36PM +0200, Danilo Krummrich wrote:
> On Mon, Apr 28, 2025 at 07:03:07AM +0200, Dirk Behme wrote:
> > On 27/04/2025 14:23, Danilo Krummrich wrote:
> > > On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> > >> 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:
> > >>>>>> +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?
> > > 
> > > If the above fails it means that for a (specific) device a driver expects that
> > > a specific property of some firmware node is present. So, yes, I think it does
> > > matter.
> > > 
> > >> There is nothing wrong with the (unknown)
> > >> device, no? What is wrong here is the firmware (node). It misses
> > >> something.
> > > 
> > > How do we know the firmware node is wrong? Maybe the driver has wrong
> > > expectations for this device?
> > > 
> > >> 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 think we can't always fix them, even if they're wrong. How do we fix ACPI
> > > firmware nodes for instance?
> > 
> > So the argument here is that the device (driver) is expecting something
> > to be "required" is wrong and might need to be fixed. Not the firmware.
> > Yes, ok, that is a valid argument. I have a device tree background and
> > there in 99% of the cases the device tree needs a fix ;)
> > 
> > But let me ask the other way around, then: What will it hurt or break if
> > we keep the pr_err() like Remo did? Even knowing that its not perfect?
> > But knowing that it will give at least a note that something is wrong
> > with at least a starting point for searching what needs to be fixed. I
> > mean even if we don't get the device, we will get the affected node we
> > can search for which device uses it as "required".
> > 
> > Could we somehow agree that in 90% of the cases this should be catched
> > at device (driver) development time, already?
> 
> I don't see why *catching* such errors needs pr_err() in core code; without it
> you still get a Result as return value that you need to handle in some way.
> 
> > And therefore it should be
> > beneficial if we don't require each and every driver to be "bloated"
> > with checking this individually?
> 
> I guess you mean "bloated with *printing* this individually", rather than
> "checking".
> 
> This is where we disagree: I think it is "bloating" the core kernel instead if
> we start adding error prints to core code, where a proper error code is
> propagated up to the driver.

1 or more error strings in every single driver is what bloats the 
kernel, not 1 string. It's all kernel code and memory usage whether it's 
core or drivers.

> I did say that I would agree to a certain extend with this specific one if we
> could print it properly, since it is designed to leave no doubt that returning
> an error code from required() is fatal for the driver. But I'm not even sure
> about this anymore.
> 
> I still haven't read a reason why this one is so crucial to print from core
> code, while for other things that are always fatal (e.g. request_irq()) we
> don't.

request_irq() is not always fatal. Some drivers fallback to polling. In 
general, we've been adding _optional() variants of functions to return 
NULL rather than errors which is handled silently by subsequent API 
calls. Secondarily, those print errors in the non-optional case. It's 
not real consistent in this area, but something we should improve.

> However, if you really think we need a common helper that prints something in
> the error case, maybe we can add an *additional* helper
> 
> 	pub fn required_by(self, dev: &Device) -> Result<T>
> 
> and document that it is the same as required(), with an additional error print
> in case of failure for the given device.

One thing that's really hard to debug in C drivers is where an 
error came from. You can for example turn on initcall_debug and see that 
a driver probe returned an error. It's virtually impossible to tell 
where that originated from. The only way to tell is with prints. That is 
probably the root of why probe has so many error prints. I think we can 
do a lot better with rust given Result can hold more than just an int. 
We obviously can't get back to the origin if that was C code, but just 
if we know exactly which call from probe failed that would be a huge 
improvement.

Rob

On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
> 
> One thing that's really hard to debug in C drivers is where an 
> error came from. You can for example turn on initcall_debug and see that 
> a driver probe returned an error. It's virtually impossible to tell 
> where that originated from. The only way to tell is with prints. That is 
> probably the root of why probe has so many error prints. I think we can 
> do a lot better with rust given Result can hold more than just an int. 

This I fully agree with, not sure if the solution is to put more stuff into the
Result type though. However, there are things like #[track_caller] (also
recently mentioned by Benno), which might be a good candidate for improving this
situation.

As mentioned, for now let's go with

	pub fn required_by(self, dev: &Device) -> Result<T>

additional to required() for this purpose to get a proper dev_err() print.

On Mon Apr 28, 2025 at 11:21 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
>> 
>> One thing that's really hard to debug in C drivers is where an 
>> error came from. You can for example turn on initcall_debug and see that 
>> a driver probe returned an error. It's virtually impossible to tell 
>> where that originated from. The only way to tell is with prints. That is 
>> probably the root of why probe has so many error prints. I think we can 
>> do a lot better with rust given Result can hold more than just an int. 
>
> This I fully agree with, not sure if the solution is to put more stuff into the
> Result type though. However, there are things like #[track_caller] (also
> recently mentioned by Benno), which might be a good candidate for improving this
> situation.
>
> As mentioned, for now let's go with
>
> 	pub fn required_by(self, dev: &Device) -> Result<T>
>
> additional to required() for this purpose to get a proper dev_err() print.

Could it make sense to _replace_ `required` with `required_by` ?
Otherwise `required` sits a little awkwardly between `optional` and
`required_by`. I can't think of a situation where `required` would be
preferred.

On Mon, Apr 28, 2025 at 11:50:19PM +0200, Remo Senekowitsch wrote:
> On Mon Apr 28, 2025 at 11:21 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
> >> 
> >> One thing that's really hard to debug in C drivers is where an 
> >> error came from. You can for example turn on initcall_debug and see that 
> >> a driver probe returned an error. It's virtually impossible to tell 
> >> where that originated from. The only way to tell is with prints. That is 
> >> probably the root of why probe has so many error prints. I think we can 
> >> do a lot better with rust given Result can hold more than just an int. 
> >
> > This I fully agree with, not sure if the solution is to put more stuff into the
> > Result type though. However, there are things like #[track_caller] (also
> > recently mentioned by Benno), which might be a good candidate for improving this
> > situation.
> >
> > As mentioned, for now let's go with
> >
> > 	pub fn required_by(self, dev: &Device) -> Result<T>
> >
> > additional to required() for this purpose to get a proper dev_err() print.
> 
> Could it make sense to _replace_ `required` with `required_by` ?
> Otherwise `required` sits a little awkwardly between `optional` and
> `required_by`. I can't think of a situation where `required` would be
> preferred.

Fine with me; required() also seems not useful to implement required_by().

However, I think we should revisit those generic error prints once we tackled
the issue of ergonomics in finding the source of an error in general.

On Sat Apr 26, 2025 at 12:15 PM CEST, 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.
>
> Drivers can still decide to properly print the error if the returned Result
> indicates one.

One alternative would be to store a reference count to the device in
`FwNode`. At that point we'd be guaranteed to have a valid reference
whenever we want to log something.

On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> On Sat Apr 26, 2025 at 12:15 PM CEST, 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.
> >
> > Drivers can still decide to properly print the error if the returned Result
> > indicates one.
> 
> One alternative would be to store a reference count to the device in
> `FwNode`. At that point we'd be guaranteed to have a valid reference
> whenever we want to log something.

Yes, that would work. However, I'm not convinced that it's worth to store an
ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
be able to force an error print if a required device property isn't available.

Why do you think it is important to force this error print by having it in
PropertyGuard::required() and even take an additional device reference for this
purpose, rather than leaving it to the driver when to print a message for an
error condition that makes it fail to probe()?

On 26.04.25 16:19, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>> On Sat Apr 26, 2025 at 12:15 PM CEST, 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.
>>>
>>> Drivers can still decide to properly print the error if the returned Result
>>> indicates one.
>>
>> One alternative would be to store a reference count to the device in
>> `FwNode`. At that point we'd be guaranteed to have a valid reference
>> whenever we want to log something.
> 
> Yes, that would work. However, I'm not convinced that it's worth to store an
> ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
> be able to force an error print if a required device property isn't available.
> 
> Why do you think it is important to force this error print by having it in
> PropertyGuard::required() and even take an additional device reference for this
> purpose, rather than leaving it to the driver when to print a message for an
> error condition that makes it fail to probe()?


To my understanding doing the error print in "core" was proposed by
Rob [1]:

"If the property is missing and required, then we may want to print an
error msg (in the core, not every caller)"

Dirk

[1]
https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/496884813

On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> On 26.04.25 16:19, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> >> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> >>> 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.
> >>>
> >>> Drivers can still decide to properly print the error if the returned Result
> >>> indicates one.
> >>
> >> One alternative would be to store a reference count to the device in
> >> `FwNode`. At that point we'd be guaranteed to have a valid reference
> >> whenever we want to log something.
> > 
> > Yes, that would work. However, I'm not convinced that it's worth to store an
> > ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
> > be able to force an error print if a required device property isn't available.
> > 
> > Why do you think it is important to force this error print by having it in
> > PropertyGuard::required() and even take an additional device reference for this
> > purpose, rather than leaving it to the driver when to print a message for an
> > error condition that makes it fail to probe()?
> 
> To my understanding doing the error print in "core" was proposed by
> Rob [1]:

That is fine, though it doesn't answer my question above. :)

On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>> On 26.04.25 16:19, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>> >> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
>> >>> 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.
>> >>>
>> >>> Drivers can still decide to properly print the error if the returned Result
>> >>> indicates one.
>> >>
>> >> One alternative would be to store a reference count to the device in
>> >> `FwNode`. At that point we'd be guaranteed to have a valid reference
>> >> whenever we want to log something.
>> > 
>> > Yes, that would work. However, I'm not convinced that it's worth to store an
>> > ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
>> > be able to force an error print if a required device property isn't available.
>> > 
>> > Why do you think it is important to force this error print by having it in
>> > PropertyGuard::required() and even take an additional device reference for this
>> > purpose, rather than leaving it to the driver when to print a message for an
>> > error condition that makes it fail to probe()?
>> 
>> To my understanding doing the error print in "core" was proposed by
>> Rob [1]:
>
> That is fine, though it doesn't answer my question above. :)

If the question is addressed to me, I don't think it is important.
I don't have a particular preference either way. I'm just trying to
come up with a solution that is satisfactory to everyone. We should
hear from Rob if he's ok with removing the logging entirely given the
limitations.

On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>>> On 26.04.25 16:19, Danilo Krummrich wrote:
>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
...
>>>> Why do you think it is important to force this error print by having it in
>>>> PropertyGuard::required() and even take an additional device reference for this
>>>> purpose, rather than leaving it to the driver when to print a message for an
>>>> error condition that makes it fail to probe()?
>>>
>>> To my understanding doing the error print in "core" was proposed by
>>> Rob [1]:
>>
>> That is fine, though it doesn't answer my question above. :)
> 
> If the question is addressed to me, I don't think it is important.
> I don't have a particular preference either way. I'm just trying to

Generally, printing in libraries an lower level routines (in this case,
"core") is undesirable. We'll do it anyway, sometimes:

     a) Behind a *_DEBUG configuration, to debug the core itself, or

     b) Desperation: hard to recover from errors, that the upper layers
        for some reason lack context to provide an adequate error
        message for.

The idea is that the lower level you are in the software stack, the
more rare printing should be.


thanks,
-- 
John Hubbard

> come up with a solution that is satisfactory to everyone. We should
> hear from Rob if he's ok with removing the logging entirely given the
> limitations.
>

On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> > On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> > > On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> > > > On 26.04.25 16:19, Danilo Krummrich wrote:
> > > > > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> > > > > > On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> ...
> > > > > Why do you think it is important to force this error print by having it in
> > > > > PropertyGuard::required() and even take an additional device reference for this
> > > > > purpose, rather than leaving it to the driver when to print a message for an
> > > > > error condition that makes it fail to probe()?
> > > > 
> > > > To my understanding doing the error print in "core" was proposed by
> > > > Rob [1]:
> > > 
> > > That is fine, though it doesn't answer my question above. :)
> > 
> > If the question is addressed to me, I don't think it is important.
> > I don't have a particular preference either way. I'm just trying to
> 
> Generally, printing in libraries an lower level routines (in this case,
> "core") is undesirable. We'll do it anyway, sometimes:
> 
>     a) Behind a *_DEBUG configuration, to debug the core itself, or
> 
>     b) Desperation: hard to recover from errors, that the upper layers
>        for some reason lack context to provide an adequate error
>        message for.
> 
> The idea is that the lower level you are in the software stack, the
> more rare printing should be.

If that's a kernel style/requirement, I've never heard that. About the 
only coding style in this area I'm aware of don't print messages on 
kmalloc failure because the core does. It's the same concept here.

When practically every caller is printing a message, it should go in the 
called function. It's not really different than anything else we do. If 
we have 2 functions commonly called in sequence, we combine them into a 
single helper function. 

It's a pattern we have in the C API that I'd rather not repeat with 
Rust.

Rob

On 4/28/25 1:18 PM, Rob Herring wrote:
> On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
>> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
>>> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>>>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>>>>> On 26.04.25 16:19, Danilo Krummrich wrote:
>>>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>>>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
>> ...
>> The idea is that the lower level you are in the software stack, the
>> more rare printing should be.
> 
> If that's a kernel style/requirement, I've never heard that. About the 
> only coding style in this area I'm aware of don't print messages on 
> kmalloc failure because the core does. It's the same concept here.
> 
> When practically every caller is printing a message, it should go in the 

If *every* caller, without exception, today and tomorrow, including 
callers that expect failure--if all of those require printing a message,
then yes, it's time to print from the lower level routine.

Short of that, you end up with potentially excessive, and definitely
unrequested and/or undesirable printing.

> called function. It's not really different than anything else we do. If 
> we have 2 functions commonly called in sequence, we combine them into a 
> single helper function. 
> 

Right. And the rules are similar: the lower level has to be accurately
factored out. Not approximately.

thanks,
-- 
John Hubbard

On Mon, Apr 28, 2025 at 01:25:03PM -0700, John Hubbard wrote:
> On 4/28/25 1:18 PM, Rob Herring wrote:
> > On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
> >> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> >>> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> >>>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> >>>>> On 26.04.25 16:19, Danilo Krummrich wrote:
> >>>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> >>>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> >> ...
> >> The idea is that the lower level you are in the software stack, the
> >> more rare printing should be.
> > 
> > If that's a kernel style/requirement, I've never heard that. About the 
> > only coding style in this area I'm aware of don't print messages on 
> > kmalloc failure because the core does. It's the same concept here.
> > 
> > When practically every caller is printing a message, it should go in the 
> 
> If *every* caller, without exception, today and tomorrow, including 
> callers that expect failure--if all of those require printing a message,
> then yes, it's time to print from the lower level routine.

We do know for 2 reasons. The first is we document with schema whether a 
property is required or not. That is a contract between the firmware and 
the OS. Changing what's required breaks that contract. Second, the 
caller indicates whether the property is required or not. We already do 
this with subsystems that are indirectly accessing properties (e.g. 
clk_get() and clk_get_optional()).

But see my other reply. We are perhaps arguing about the symptoms rather 
than what is the root cause for having prints in the first place.

Rob