Re: [PATCH 01/10] rust: Move property_present to property.rs

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 9:51 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
>>
>> +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");
>
> It's usually not a good idea to panic the kernel especially with 
> something a driver calls as that's probably recoverable.
>
> 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.

Just to be clear on this, should I keep this as is, or return a result?
In the latter case, all the duplicated methods on `Device` that just
call `self.fwnode().same_method()` would have a result in their function
signatur as well. That includes `property_present`, `read_property`
and `children`.

>> +        }
>> +        // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
>> +        // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
>> +        // doesn't increment the refcount.
>> +        unsafe { &*fwnode_handle.cast() }
>> +    }
>> +
>> +    /// Checks if property is present or not.
>> +    pub fn property_present(&self, name: &CStr) -> bool {
>> +        self.fwnode().property_present(name)
>> +    }
>> +}
>
> The C developer in me wants to put this after the FwNode stuff since 
> this uses it.

Is that just a comment or a call to action? :-)

Remo