Re: [PATCH 1/3] rust: char_dev: add character device abstraction

[Greg Kroah-Hartman]

On Fri, Oct 11, 2024 at 08:55:42PM +0200, Josef Zoller wrote:
> +unsigned int rust_helper_MAJOR(dev_t dev)
> +{
> +	return MAJOR(dev);
> +}

I don't think you use this function in the code anywhere.

> +unsigned int rust_helper_MINOR(dev_t dev)
> +{
> +	return MINOR(dev);
> +}

Is this really needed?  No driver should care about their minor number,
except to possibly set it, not read it.

> +dev_t rust_helper_MKDEV(unsigned int major, unsigned int minor)
> +{
> +	return MKDEV(major, minor);
> +}

If you are only doing dynamic creation, as your initial text said, I
don't think you need this either as the kernel should create it for you.


> diff --git a/rust/kernel/char_dev.rs b/rust/kernel/char_dev.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b81c0d55ab60f18dc82a99991318a5ae0a26e560
> --- /dev/null
> +++ b/rust/kernel/char_dev.rs
> @@ -0,0 +1,976 @@
> +// SPDX-License-Identifier: GPL-2.0

Minor nit, you forgot a copyright line :)

> +
> +//! Character device support.
> +//!
> +//! C headers: [`include/linux/cdev.h`](srctree/include/linux/cdev.h) and
> +//! [`include/linux/fs.h`](srctree/include/linux/fs.h)
> +
> +use crate::{
> +    bindings, container_of,
> +    error::{to_result, VTABLE_DEFAULT_ERROR},
> +    fs::{File, LocalFile},
> +    ioctl::IoctlCommand,
> +    prelude::*,
> +    types::{ForeignOwnable, Opaque},
> +    uaccess::{UserPtr, UserSlice, UserSliceReader, UserSliceWriter},
> +};
> +use core::{ffi, mem, ops::Deref};
> +
> +/// Character device ID.
> +///
> +/// This is a wrapper around the kernel's `dev_t` type and identifies a
> +/// character device by its major and minor numbers.
> +#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
> +#[repr(transparent)]
> +pub struct CharDeviceID(bindings::dev_t); // u32
> +
> +impl CharDeviceID {
> +    /// Creates a new device ID from the given major and minor numbers.
> +    ///
> +    /// This corresponds to the kernel's `MKDEV` macro.
> +    pub fn new(major: u32, minor: u32) -> Self {
> +        // SAFETY: The kernel's `MKDEV` macro is safe to call with any values.
> +        Self(unsafe { bindings::MKDEV(major, minor) })
> +    }
> +
> +    /// Returns the major number of the device ID.
> +    ///
> +    /// This corresponds to the kernel's `MAJOR` macro.
> +    pub fn major(&self) -> u32 {
> +        // SAFETY: The kernel's `MAJOR` macro is safe to call with any value.
> +        unsafe { bindings::MAJOR(self.0) }
> +    }
> +
> +    /// Returns the minor number of the device ID.
> +    ///
> +    /// This corresponds to the kernel's `MINOR` macro.
> +    pub fn minor(&self) -> u32 {
> +        // SAFETY: The kernel's `MINOR` macro is safe to call with any value.
> +        unsafe { bindings::MINOR(self.0) }
> +    }
> +}
> +
> +/// Seek mode for the `llseek` method.
> +///
> +/// This enum corresponds to the `SEEK_*` constants in the kernel.
> +#[repr(u32)]
> +pub enum Whence {
> +    /// Set the file position to `offset`.
> +    Set = bindings::SEEK_SET,
> +    /// Set the file position to the current position plus `offset`.
> +    Cur = bindings::SEEK_CUR,
> +    /// Set the file position to the end of the file plus `offset`.
> +    End = bindings::SEEK_END,
> +    /// Set the file position to the next location in the file greater than or
> +    /// equal to `offset` containing data.
> +    Data = bindings::SEEK_DATA,
> +    /// Set the file position to the next hole in the file greater than or
> +    /// equal to `offset`.
> +    Hole = bindings::SEEK_HOLE,
> +}
> +
> +// Make sure at compile time that the `Whence` enum can be safely converted
> +// from integers up to `SEEK_MAX`.
> +const _: () = assert!(Whence::Hole as u32 == bindings::SEEK_MAX);
> +
> +/// Trait implemented by a registered character device.
> +///
> +/// A registered character device just handles the `open` operation on the
> +/// device file and returns an open device type (which implements the
> +/// [`OpenCharDevice`] trait) that handles the actual I/O operations on the
> +/// device file. Optionally, the `release` operation can be implemented to
> +/// handle the final close of the device file, but simple cleanup can also be
> +/// done in the `Drop` implementation of the open device type.

release is traditionally where you handle cleaning up what was allocated
for this "open", and then drop can handle any "global" state for the
device associated with this specific instance.  So "simple cleanup"
might not be possible in both places, as they are different parts of the
lifecycle of a device.

thanks,

greg k-h