linux-kernel - Re: [PATCH v2 1/5] rust: Move property

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <D9765O6UTEX9.1BI7DOZ1YMMZ4@buenzli.dev>
Date: Tue, 15 Apr 2025 13:17:46 +0200
From: "Remo Senekowitsch" <remo@...nzli.dev>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "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>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] rust: Move property_present to separate file

On Mon Apr 14, 2025 at 8:00 PM CEST, Danilo Krummrich wrote:
> 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:
>> >> +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.

That makes sense. In that case, I think we shouldn't duplicate the
methods on `Device` and `FwNode`, because their signatures would be
different. E.g. `Device::property_present` would have to return
`Option<bool>` while `FwNode::property_present` would return `bool`.
I think it would be cleaner to force driver authors to do something
like `if let Some(fwnode) = dev.fwnode() { /* read properties */ }`.