Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Apr 14, 2025 at 06:40:54PM +0200, Remo Senekowitsch wrote:
> On Mon Apr 14, 2025 at 6:00 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 14, 2025 at 05:26:26PM +0200, Remo Senekowitsch wrote:
> >> Not all property-related APIs can be exposed directly on a device.
> >> For example, iterating over child nodes of a device will yield
> >> fwnode_handle. Thus, in order to access properties on these child nodes,
> >> duplicate the APIs on a fwnode as they are in C.
> >
> > What do you mean with "duplicate the APIs"?
> 
> The property related methods on `Device` and `FwNode` are mostly
> the same. Same names, same function signatures. The methods on
> `Device` simply call `self.fwnode()` and then the same method on
> `FwNode`. I was asked to do this so driver authors don't have to write
> `dev.fwnode().read_propert()` etc. every time they want to do something
> on the device itself. This is the case here for `property_present` as
> well as the methods added in the following patches.

Yeah, I noticed when I continued with patch 2 of this series.

> > Also, this patch does three separate things.
> >
> >   (1) move property_present() to separate file
> >   (2) implement FwNode abstraction
> >   (3) implement Device::fwnode()
> >
> > I'd rather keep property_present() in device.rs and also move fwnode() into
> > device.rs, even though in C it's in property.c. impl Device blocks should be in
> > device.rs.
> 
> I was asked to move it by Rob Herring on Zulip[1]. Dirk Behme also
> agreed with it[2]. I have no strong opinion on this, hopefully you can
> come to an agreement. Whatever it is, I'll happily implement it.

Please also see my comment in patch 2. If we think device.rs becomes too messy
with that, we should convert device.rs to its own module, i.e.
rust/kernel/device/mod.rs and then have rust/kernel/device/property.rs instead.

> >> +++ b/rust/kernel/property.rs
> >> @@ -0,0 +1,75 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Unified device property interface.
> >> +//!
> >> +//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
> >> +
> >> +use core::ptr;
> >> +
> >> +use crate::{bindings, device::Device, str::CStr, types::Opaque};
> >> +
> >> +impl Device {
> >> +    /// Obtain the fwnode corresponding to the device.
> >> +    fn fwnode(&self) -> &FwNode {
> >> +        // SAFETY: `self` is valid.
> >> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
> >> +        if fwnode_handle.is_null() {
> >> +            panic!("fwnode_handle cannot be null");
> >
> > Please don't use panic!() unless you hit an error condition that is impossible
> > to handle.
> >
> > If __dev_fwnode() can ever return NULL, make this function return
> > Option<&FwNode> instead.
> 
> Rob Herring mentioned this in the previous thread already:
> 
> > Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
> > Though often that's a legacy code path, so maybe not allowing NULL is
> > fine for now.
> 
> I asked a follow-up question about how to proceed but got no reply[3].

I NULL is a possible return value of __dev_fwnode() (which seems to be the
case), there's no other option than makeing fwnode() fallible.

> 
> Best regards,
> Remo
> 
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495807346 [1]
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495945200 [2]
> Link: https://lore.kernel.org/rust-for-linux/D96HP1UV49SY.1U1BFA14P8XHE@buenzli.dev/T/#mc7967352b7bd1be812806939150511c1a5018bb1 [3]