Re: [PATCH v5 4/9] rust: device: Enable printing fwnode name and path

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, May 21, 2025 at 03:03:07PM +0200, Remo Senekowitsch wrote:
> On Wed May 21, 2025 at 2:02 PM CEST, Greg Kroah-Hartman wrote:
> > On Tue, May 20, 2025 at 10:00:19PM +0200, Remo Senekowitsch wrote:
> >> Add two new public methods `display_name` and `display_path` to
> >> `FwNode`. They can be used by driver authors for logging purposes. In
> >> addition, they will be used by core property abstractions for automatic
> >> logging, for example when a driver attempts to read a required but
> >> missing property.
> >> 
> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> ---
> >>  rust/kernel/device/property.rs | 72 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 72 insertions(+)
> >> 
> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> index 70593343bd811..6ccc7947f9c31 100644
> >> --- a/rust/kernel/device/property.rs
> >> +++ b/rust/kernel/device/property.rs
> >> @@ -32,6 +32,78 @@ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
> >>          self.0.get()
> >>      }
> >>  
> >> +    /// 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 + '_ {
> >> +        struct FwNodeDisplayName<'a>(&'a FwNode);
> >> +
> >> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
> >> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> >> +                // SAFETY: self is valid by its type invariant
> >> +                let name = unsafe { bindings::fwnode_get_name(self.0.as_raw()) };
> >> +                if name.is_null() {
> >> +                    return Ok(());
> >
> > So if there is no name, you are returning Ok()?  Are you sure that's ok
> > to do?  What will the result of the string look like then?
> 
> In that case we're not writing anything to the formatter, which is
> equivalent to an empty string. `Ok(())` means that writing succeeded.
> 
> I assumed that a valid node would always have a name. And we're
> guaranteed to have a valid node. So I assumed this case would never
> happen and didn't think too hard about it. But even if a valid node has
> not name, empty string is probably the correct thing, right?

I don't know what this "name" is used for.  An empty string might not be
what you want to use here, given that you could be naming something
based on it, right?  fwnode_get_name() is used for many things,
including the detection if a name is not present at all, and if not,
then the code needs to clean up and abort.

So what exactly are you going to be using this for?

thanks,

greg k-h