Message-ID: <984f6b1b-b396-45e1-8325-90a01e4305c3@walterzollerpiano.com>
Date: Sat, 12 Oct 2024 23:48:59 +0200
From: Josef Zoller <josef@...terzollerpiano.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Arnd Bergmann <arnd@...db.de>, 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>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/3] rust: char_dev: add character device abstraction



On 12.10.24 09:36, Greg Kroah-Hartman wrote:
> 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.

Yes, I do. I use it at rust/kernel/char_dev.rs:41.

>> +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.

It's not really needed, but I use it in my scull example to just print
the major and minor numbers of the devices, which makes it easy to
quickly create a device file for testing using mknod.

>> +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.

Yeah, you're right. I forgot to remove this one after I changed to only
allowing dynamic device number creation.

>> 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 :)

Oh, I just looked at some other rust files in rust/kernel and saw that
most of them don't have a copyright line. I'll add one to this file.

>> +
>> +//! 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.

I guess, I haven't explained this clearly enough in the doc comment.
We have two different types here that correspond to two different parts
of the lifecycle of a device file:

We have a type implementing the `CharDevice` trait. An instance of this
type is created on device registration and lives until the device is
deregistered. And we have a type implementing the `OpenCharDevice`
trait. An instance of this type is created by the CharDevice type in the
open implementation and lives until the corresponding release is called.
That means that the OpenCharDevice type instance is dropped in
f_ops->release, even when the CharDevice type doesn't implement release,
so simple cleanup can happen in the drop implementation of the type
implementing the OpenCharDevice trait and will correctly be called on
release.

Cheers,
Josef

Powered by blists - more mailing lists