Message-ID: <CAL_JsqJzh7dWU9gAZV_cKfi_6kOm3stbr9DdFzdse4gUYPn9Vg@mail.gmail.com>
Date: Sun, 27 Oct 2024 17:07:44 -0500
From: Rob Herring <robh@...nel.org>
To: Alex Gaynor <alex.gaynor@...il.com>
Cc: Saravana Kannan <saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 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>, 
	Danilo Krummrich <dakr@...nel.org>, Dirk Behme <dirk.behme@...il.com>, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC 2/3] rust: Add bindings for device properties

On Fri, Oct 25, 2024 at 4:12 PM Alex Gaynor <alex.gaynor@...il.com> wrote:
>
> On Fri, Oct 25, 2024 at 5:06 PM Rob Herring (Arm) <robh@...nel.org> wrote:
> >
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> >
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > sizes of properties (e.g. u8, u16, u32).
> >
> > Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> > ---
> > Not sure if we need the KVec variant, but I kept it as that was my first
> > pass attempt. Most callers are filling in some value in a driver data
> > struct. Sometimes the number of elements is not known, so the caller
> > calls to get the array size, allocs the correct size buffer, and then
> > reads the property again to fill in the buffer.
> >
> > I have not implemented a wrapper for device_property_read_string(_array)
> > because that API is problematic for dynamic DT nodes. The API just
> > returns pointer(s) into the raw DT data. We probably need to return a
> > copy of the string(s) instead for rust.
> >
> > After property accessors, next up is child node accessors/iterators.
> > ---
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/kernel/device.rs           | 145 +++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 217c776615b9..65717cc20a23 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -19,6 +19,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/phy.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/refcount.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 0c28b1e6b004..bb66a28df890 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -5,10 +5,14 @@
> >  //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> >  use crate::{
> > +    alloc::KVec,
> >      bindings,
> > +    error::{to_result, Result},
> > +    prelude::*,
> > +    str::CStr,
> >      types::{ARef, Opaque},
> >  };
> > -use core::{fmt, ptr};
> > +use core::{fmt, mem::size_of, ptr};
> >
> >  #[cfg(CONFIG_PRINTK)]
> >  use crate::c_str;
> > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> >              )
> >          };
> >      }
> > +
> > +    /// Returns if a firmware property `name` is true or false
> > +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> > +        unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > +    }
> > +
> > +    /// Returns if a firmware string property `name` has match for `match_str`
> > +    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > +        let ret = unsafe {
> > +            bindings::device_property_match_string(
> > +                self.as_raw(),
> > +                name.as_ptr() as *const i8,
> > +                match_str.as_ptr() as *const i8,
> > +            )
> > +        };
> > +        to_result(ret)?;
> > +        Ok(ret as usize)
> > +    }
> > +
> > +    /// Returns firmware property `name` scalar value
> > +    ///
> > +    /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > +    pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > +        let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > +
> > +        Self::_property_read_array(&self, name, &mut val)?;
> > +        Ok(val[0])
> > +    }
> > +
>
> This, and several of the other methods are unsound, because they can
> be used to construct arbitrary types for which may not allow all bit
> patterns. You can use:
> https://rust.docs.kernel.org/kernel/types/trait.FromBytes.html as the
> bound to ensure only valid types are used.

I meant to ask a question on this. FromBytes is probably still too
permissive for what is supported in DT. I think floating point types
would be allowed which is not valid in DT (though asahi linux has some
downstream patches adding FP data support). What I really need is the
Integer trait from the num crate or some way to define the exact types
supported. I tried Into<u32>, etc., but that didn't work.

>
> Also, instead of using mem::zeroed(), you should use MaybeUnininit
> (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html)
> which allows you to avoid needing to zero initialize.

Okay, thanks. Being new to rust, I find this a bit complicated
compared to C (perhaps it has to be to avoid this class of bugs).
Maybe that's because this stuff seems to have been evolving some with
the kernel rust support. In any case, some examples of how (and
perhaps) to do uninitialized and zero-initialized allocations would be
helpful as both are common in the kernel.

Rob

Powered by blists - more mailing lists