Message-Id: <DCCM3G61QPMD.2R4QDAG1U7NCQ@kernel.org>
Date: Tue, 26 Aug 2025 21:38:25 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Matthew Maurer" <mmaurer@...gle.com>
Cc: "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>, "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>, "Sami Tolvanen" <samitolvanen@...gle.com>, "Timur
 Tabi" <ttabi@...dia.com>, "Benno Lossin" <lossin@...nel.org>, "Dirk Beheme"
 <dirk.behme@...bosch.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v10 3/7] rust: debugfs: Add support for writable files

On Wed Aug 20, 2025 at 12:53 AM CEST, Matthew Maurer wrote:
> +    /// Creates a read-write file in this directory.
> +    ///
> +    /// Reading the file uses the [`Render`] implementation.
> +    /// Writing to the file uses the [`UpdateFromSlice`] implementation.
> +    pub fn read_write_file<
> +        'a,
> +        T: Render + UpdateFromSlice + Send + Sync + 'static,
> +        E: 'a,
> +        TI: PinInit<T, E> + 'a,

Same comments as in the previous patch, I think using a where clause is a bit
cleaner, even though with this formatting it'd be fine too, but this is not
guaranteed.

> +    >(
> +        &'a self,
> +        name: &'a CStr,
> +        data: TI,

impl PinInit<T, E> + 'a

> +    ) -> impl PinInit<File<T>, E> + 'a {
> +        let file_ops = &<T as ReadWriteFile<_>>::FILE_OPS;
> +        self.create_file(name, data, file_ops)
> +    }

> +fn update<T: UpdateFromSlice + Sync>(data: &T, buf: *const c_char, count: usize) -> isize {
> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf as *mut c_void), count).reader();

This naming is pretty close to what I was about to propose for the UpdateFromSlice
trait. :)

Given that I proposed debugfs::Writer instead of debugfs::Render, I think we
should just rename debugfs::UpdateFromSlice to debugfs::Reader.

> +
> +    if let Err(e) = data.update_from_slice(&mut reader) {
> +        return e.to_errno() as isize;
> +    }
> +
> +    count as isize
> +}

<snip>

> +/// # Safety
> +///
> +/// `inode` must be a valid pointer to an `inode` struct.
> +/// `file` must be a valid pointer to a `file` struct.
> +unsafe extern "C" fn write_only_open(
> +    inode: *mut bindings::inode,
> +    file: *mut bindings::file,
> +) -> c_int {
> +    // SAFETY: The caller ensures that `inode` and `file` are valid pointers.
> +    unsafe {
> +        (*file).private_data = (*inode).i_private;
> +    }

NIT: If you move the semicolon at the end of the unsafe block it goes into a
single line.

> +    0
> +}

<snip>

> +impl<T: UpdateFromSlice + Sync> WriteFile<T> for T {
> +    const FILE_OPS: FileOps<T> = {
> +        let operations = bindings::file_operations {
> +            open: Some(write_only_open),
> +            write: Some(write_only_write::<T>),
> +            llseek: Some(bindings::noop_llseek),
> +            // SAFETY: `file_operations` supports zeroes in all fields.
> +            ..unsafe { core::mem::zeroed() }
> +        };
> +        // SAFETY:
> +        // * `write_only_open` populates the file private data with the inode private data
> +        // * `write_only_write`'s only requirement is that the private data of the file point to
> +        //   a `T` and be legal to convert to a shared reference, which `write_only_open`
> +        //   satisfies.
> +        unsafe { FileOps::new(operations, 0o200) }

I think it'd be nice to have an abstraction for file modes, but this can be done
separately.

> +    };
> +}
> diff --git a/rust/kernel/debugfs/traits.rs b/rust/kernel/debugfs/traits.rs
> index 2939e18e3dda39571cd7255505e5f605f0e3d154..d64638898faaa1a6a9898c374b8c1114993376c9 100644
> --- a/rust/kernel/debugfs/traits.rs
> +++ b/rust/kernel/debugfs/traits.rs
> @@ -3,8 +3,15 @@
>  
>  //! Traits for rendering or updating values exported to DebugFS.
>  
> +use crate::prelude::*;
>  use crate::sync::Mutex;
> +use crate::uaccess::UserSliceReader;
>  use core::fmt::{self, Debug, Formatter};
> +use core::str::FromStr;
> +use core::sync::atomic::{
> +    AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, AtomicU16, AtomicU32, AtomicU64,
> +    AtomicU8, AtomicUsize, Ordering,
> +};
>  
>  /// A trait for types that can be rendered into a string.
>  ///
> @@ -26,3 +33,65 @@ fn render(&self, f: &mut Formatter<'_>) -> fmt::Result {
>          writeln!(f, "{self:?}")
>      }
>  }
> +
> +/// A trait for types that can be updated from a user slice.
> +///
> +/// This works similarly to `FromStr`, but operates on a `UserSliceReader` rather than a &str.
> +///
> +/// It is automatically implemented for all atomic integers, or any type that implements `FromStr`
> +/// wrapped in a `Mutex`.
> +pub trait UpdateFromSlice {

As mentioned above, I think we should name this Reader and the Render thing
Writer.

> +    /// Updates the value from the given user slice.
> +    fn update_from_slice(&self, reader: &mut UserSliceReader) -> Result<()>;

read_from_slice()?

> +}
> +
> +impl<T: FromStr> UpdateFromSlice for Mutex<T> {
> +    fn update_from_slice(&self, reader: &mut UserSliceReader) -> Result<()> {
> +        let mut buf = [0u8; 128];
> +        if reader.len() > buf.len() {
> +            return Err(EINVAL);
> +        }
> +        let n = reader.len();
> +        reader.read_slice(&mut buf[..n])?;
> +
> +        let s = core::str::from_utf8(&buf[..n]).map_err(|_| EINVAL)?;
> +        let val = s.trim().parse::<T>().map_err(|_| EINVAL)?;
> +        *self.lock() = val;
> +        Ok(())
> +    }
> +}
> +
> +macro_rules! impl_update_from_slice_for_atomic {
> +    ($(($atomic_type:ty, $int_type:ty)),*) => {
> +        $(
> +            impl UpdateFromSlice for $atomic_type {
> +                fn update_from_slice(&self, reader: &mut UserSliceReader) -> Result<()> {
> +                    let mut buf = [0u8; 21]; // Enough for a 64-bit number.
> +                    if reader.len() > buf.len() {
> +                        return Err(EINVAL);
> +                    }
> +                    let n = reader.len();
> +                    reader.read_slice(&mut buf[..n])?;
> +
> +                    let s = core::str::from_utf8(&buf[..n]).map_err(|_| EINVAL)?;
> +                    let val = s.trim().parse::<$int_type>().map_err(|_| EINVAL)?;
> +                    self.store(val, Ordering::Relaxed);
> +                    Ok(())
> +                }
> +            }
> +        )*
> +    };
> +}
> +
> +impl_update_from_slice_for_atomic!(
> +    (AtomicI16, i16),
> +    (AtomicI32, i32),
> +    (AtomicI64, i64),
> +    (AtomicI8, i8),
> +    (AtomicIsize, isize),
> +    (AtomicU16, u16),
> +    (AtomicU32, u32),
> +    (AtomicU64, u64),
> +    (AtomicU8, u8),
> +    (AtomicUsize, usize)
> +);
>
> -- 
> 2.51.0.rc1.167.g924127e9c0-goog

Powered by blists - more mailing lists