| 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
| ||
|
Message-ID: <7974c521-975c-4df6-bf3c-f5f3e123019e@de.bosch.com>
Date: Mon, 28 Oct 2024 08:12:31 +0100
From: Dirk Behme <dirk.behme@...bosch.com>
To: "Rob Herring (Arm)" <robh@...nel.org>, Saravana Kannan
<saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, 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>, Danilo Krummrich <dakr@...nel.org>, Dirk Behme
<dirk.behme@...il.com>
CC: <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 25.10.2024 23:05, Rob Herring (Arm) 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>
Building this on rust-next with CLIPPY=1 there are some improvement
requests:
* Function documentation for property_read_array_vec
* SAFETY comments for all unsafe{} blocks
* Different ret usage for match size_of::<T>()
* Use self instead of &self
See [1] below (without the SAFETY comments).
Best regards
Dirk
> ---
> 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])
> + }
> +
> + /// Returns firmware property `name` array values
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> + let mut val: [T; N] = unsafe { core::mem::zeroed() };
> +
> + Self::_property_read_array(self, name, &mut val)?;
> + Ok(val)
> + }
> +
> + fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result > + match size_of::<T>() {
> + 1 => to_result(unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u8,
> + val.len(),
> + )
> + })?,
> + 2 => to_result(unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u16,
> + val.len(),
> + )
> + })?,
> + 4 => to_result(unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u32,
> + val.len(),
> + )
> + })?,
> + 8 => to_result(unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + val.as_ptr() as *mut u64,
> + val.len(),
> + )
> + })?,
> + _ => return Err(EINVAL),
> + }
> + Ok(())
> + }
> +
> + pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: len always matches capacity
> + unsafe { val.set_len(len) }
> + Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> + Ok(val)
> + }
> +
> + /// Returns array length for firmware property `name`
> + ///
> + /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> + pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> + let ret;
> +
> + match size_of::<T>() {
> + 1 => {
> + ret = unsafe {
> + bindings::device_property_read_u8_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 2 => {
> + ret = unsafe {
> + bindings::device_property_read_u16_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 4 => {
> + ret = unsafe {
> + bindings::device_property_read_u32_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + 8 => {
> + ret = unsafe {
> + bindings::device_property_read_u64_array(
> + self.as_raw(),
> + name.as_ptr() as *const i8,
> + ptr::null_mut(),
> + 0,
> + )
> + }
> + }
> + _ => return Err(EINVAL),
> + }
> + to_result(ret)?;
> + Ok(ret.try_into().unwrap())
> + }
> }
>
> // SAFETY: Instances of `Device` are always reference-counted.
[1]
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index bb66a28df890..3c461b1602d5 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -218,7 +218,7 @@ pub fn property_match_string(&self, name: &CStr,
match_str: &CStr) -> Result<usi
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)?;
+ Self::_property_read_array(self, name, &mut val)?;
Ok(val[0])
}
@@ -271,12 +271,13 @@ fn _property_read_array<T>(&self, name: &CStr,
val: &mut [T]) -> Result {
Ok(())
}
+ /// ToDo
pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize)
-> Result<KVec<T>> {
let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
// SAFETY: len always matches capacity
unsafe { val.set_len(len) }
- Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
+ Self::_property_read_array::<T>(self, name, val.as_mut_slice())?;
Ok(val)
}
@@ -284,11 +285,10 @@ pub fn property_read_array_vec<T>(&self, name:
&CStr, len: usize) -> Result<KVec
///
/// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
- let ret;
- match size_of::<T>() {
+ let ret = match size_of::<T>() {
1 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u8_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -298,7 +298,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
2 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u16_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -308,7 +308,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
4 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u32_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -318,7 +318,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
8 => {
- ret = unsafe {
+ unsafe {
bindings::device_property_read_u64_array(
self.as_raw(),
name.as_ptr() as *const i8,
@@ -328,7 +328,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) ->
Result<usize> {
}
}
_ => return Err(EINVAL),
- }
+ };
to_result(ret)?;
Ok(ret.try_into().unwrap())
}
Powered by blists - more mailing lists