[PATCH v3 00/16] rnull: add configfs, remote completion to rnull

[PATCH v3 00/16] rnull: add configfs, remote completion to rnull

[Andreas Hindborg]

This series adds configuration via configfs and remote completion to
the rnull driver. The series also includes a set of changes to the
rust block device driver API: a few cleanup patches, and a few features
supporting the rnull changes.

The series removes the raw buffer formatting logic from `kernel::block`
and improves the logic available in `kernel::string` to support the same
use as the removed logic.

This series is a smaller subset of the patches available in the
downstream rnull tree. I hope to minimize the delta between upstream
and downstream over the next few kernel releases.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v3:
- Rename `NullBorrowFormatter` as `NullTerminatedFormatter`.
- Remove `pos` from `NullBorrowFormatter`.
- Call into `Self::new` in `NullBorrowFormatter::from_array`.
- Use `Option` return type in `NullBorrowFormatter::from_array`
- Use `Option` return type in `NullBorrowFormatter::new`.
- Remove `BorrowFormatter` and update `Formatter` with a generic lifetime.
- Split visibility change of `str::Formatter` into separate patch.
- Link to v2: https://lore.kernel.org/r/20250708-rnull-up-v6-16-v2-0-ab93c0ff429b@kernel.org

Changes in v2:
- Rework formatter logic. Add two new formatters that write to slices,
  one of which adds a trailing null byte.
- Reorder and split patches so that changes are more clear.
- Fix a typo in soft-irq patch summary.
- Link to v1: https://lore.kernel.org/r/20250616-rnull-up-v6-16-v1-0-a4168b8e76b2@kernel.org

---
Andreas Hindborg (16):
      rust: str: normalize imports in `str.rs`
      rust: str: allow `str::Formatter` to format into `&mut [u8]`.
      rust: str: expose `str::Formatter::new` publicly.
      rust: str: make `RawFormatter::bytes_written` public.
      rust: str: introduce `NullTerminatedFormatter`
      rust: block: normalize imports for `gen_disk.rs`
      rust: block: use `NullTerminatedFormatter`
      rust: block: remove `RawWriter`
      rust: block: remove trait bound from `mq::Request` definition
      rust: block: add block related constants
      rnull: move driver to separate directory
      rnull: enable configuration via `configfs`
      rust: block: add `GenDisk` private data support
      rust: block: mq: fix spelling in a safety comment
      rust: block: add remote completion to `Request`
      rnull: add soft-irq completion support

 MAINTAINERS                        |   2 +-
 drivers/block/Kconfig              |  10 +-
 drivers/block/Makefile             |   4 +-
 drivers/block/rnull.rs             |  80 -----------
 drivers/block/rnull/Kconfig        |  13 ++
 drivers/block/rnull/Makefile       |   3 +
 drivers/block/rnull/configfs.rs    | 277 +++++++++++++++++++++++++++++++++++++
 drivers/block/rnull/rnull.rs       | 105 ++++++++++++++
 rust/kernel/block.rs               |  12 ++
 rust/kernel/block/mq.rs            |  14 +-
 rust/kernel/block/mq/gen_disk.rs   |  53 +++++--
 rust/kernel/block/mq/operations.rs |  65 +++++++--
 rust/kernel/block/mq/raw_writer.rs |  55 --------
 rust/kernel/block/mq/request.rs    |  21 ++-
 rust/kernel/str.rs                 |  78 +++++++++--
 15 files changed, 603 insertions(+), 189 deletions(-)
---
base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
change-id: 20250616-rnull-up-v6-16-b4571e28feee

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v3 01/16] rust: str: normalize imports in `str.rs`

[Andreas Hindborg]

Clean up imports in `str.rs`. This makes future code manipulation more
manageable.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index a927db8e079c..488b0e97004e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -3,10 +3,11 @@
 //! String representations.
 
 use crate::alloc::{flags::*, AllocError, KVec};
-use core::fmt::{self, Write};
-use core::ops::{self, Deref, DerefMut, Index};
-
 use crate::prelude::*;
+use core::{
+    fmt::{self, Write},
+    ops::{self, Deref, DerefMut, Index},
+};
 
 /// Byte string without UTF-8 validity guarantee.
 #[repr(transparent)]

-- 
2.47.2

[PATCH v3 02/16] rust: str: allow `str::Formatter` to format into `&mut [u8]`.

[Andreas Hindborg]

Improve `Formatter` so that it can write to an array or slice buffer.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 488b0e97004e..41af456a46c8 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -6,6 +6,7 @@
 use crate::prelude::*;
 use core::{
     fmt::{self, Write},
+    marker::PhantomData,
     ops::{self, Deref, DerefMut, Index},
 };
 
@@ -794,9 +795,9 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
 /// Allows formatting of [`fmt::Arguments`] into a raw buffer.
 ///
 /// Fails if callers attempt to write more than will fit in the buffer.
-pub(crate) struct Formatter(RawFormatter);
+pub(crate) struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
 
-impl Formatter {
+impl Formatter<'_> {
     /// Creates a new instance of [`Formatter`] with the given buffer.
     ///
     /// # Safety
@@ -805,11 +806,19 @@ impl Formatter {
     /// for the lifetime of the returned [`Formatter`].
     pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
         // SAFETY: The safety requirements of this function satisfy those of the callee.
-        Self(unsafe { RawFormatter::from_buffer(buf, len) })
+        Self(unsafe { RawFormatter::from_buffer(buf, len) }, PhantomData)
+    }
+
+    /// Create a new [`Self`] instance.
+    #[expect(dead_code)]
+    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
+        // SAFETY: `buffer` is valid for writes for the entire length for
+        // the lifetime of `Self`.
+        unsafe { Formatter::from_buffer(buffer.as_mut_ptr(), buffer.len()) }
     }
 }
 
-impl Deref for Formatter {
+impl Deref for Formatter<'_> {
     type Target = RawFormatter;
 
     fn deref(&self) -> &Self::Target {
@@ -817,7 +826,7 @@ fn deref(&self) -> &Self::Target {
     }
 }
 
-impl fmt::Write for Formatter {
+impl fmt::Write for Formatter<'_> {
     fn write_str(&mut self, s: &str) -> fmt::Result {
         self.0.write_str(s)?;
 

-- 
2.47.2

[PATCH v3 03/16] rust: str: expose `str::Formatter::new` publicly.

[Andreas Hindborg]

rnull is going to make use of `str::Formatter`, so expose it with public
visibility.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 41af456a46c8..28a6179385fc 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -703,7 +703,7 @@ fn test_bstr_debug() -> Result {
 ///
 /// The memory region between `pos` (inclusive) and `end` (exclusive) is valid for writes if `pos`
 /// is less than `end`.
-pub(crate) struct RawFormatter {
+pub struct RawFormatter {
     // Use `usize` to use `saturating_*` functions.
     beg: usize,
     pos: usize,
@@ -795,7 +795,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
 /// Allows formatting of [`fmt::Arguments`] into a raw buffer.
 ///
 /// Fails if callers attempt to write more than will fit in the buffer.
-pub(crate) struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
+pub struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
 
 impl Formatter<'_> {
     /// Creates a new instance of [`Formatter`] with the given buffer.
@@ -810,8 +810,7 @@ pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
     }
 
     /// Create a new [`Self`] instance.
-    #[expect(dead_code)]
-    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
+    pub fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
         // SAFETY: `buffer` is valid for writes for the entire length for
         // the lifetime of `Self`.
         unsafe { Formatter::from_buffer(buffer.as_mut_ptr(), buffer.len()) }

-- 
2.47.2

[PATCH v3 04/16] rust: str: make `RawFormatter::bytes_written` public.

[Andreas Hindborg]

rnull is going to make use of `kernel::str::RawFormatter::bytes_written`,
so make the visibility public.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 28a6179385fc..b1bc584803b0 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -761,7 +761,7 @@ pub(crate) fn pos(&self) -> *mut u8 {
     }
 
     /// Returns the number of bytes written to the formatter.
-    pub(crate) fn bytes_written(&self) -> usize {
+    pub fn bytes_written(&self) -> usize {
         self.pos - self.beg
     }
 }

-- 
2.47.2

[PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

[Andreas Hindborg]

Add `NullTerminatedFormatter`, a formatter that writes a null terminated
string to an array or slice buffer. Because this type needs to manage the
trailing null marker, the existing formatters cannot be used to implement
this type.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index b1bc584803b0..c58925438c6e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
     }
 }
 
+/// A mutable reference to a byte buffer where a string can be written into.
+///
+/// The buffer will be automatically null terminated after the last written character.
+///
+/// # Invariants
+///
+/// `buffer` is always null terminated.
+pub(crate) struct NullTerminatedFormatter<'a> {
+    buffer: &'a mut [u8],
+}
+
+impl<'a> NullTerminatedFormatter<'a> {
+    /// Create a new [`Self`] instance.
+    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
+        *(buffer.first_mut()?) = 0;
+
+        // INVARIANT: We null terminated the buffer above.
+        Some(Self { buffer })
+    }
+
+    #[expect(dead_code)]
+    pub(crate) fn from_array<const N: usize>(
+        buffer: &'a mut [crate::ffi::c_char; N],
+    ) -> Option<NullTerminatedFormatter<'a>> {
+        Self::new(buffer)
+    }
+}
+
+impl Write for NullTerminatedFormatter<'_> {
+    fn write_str(&mut self, s: &str) -> fmt::Result {
+        let bytes = s.as_bytes();
+        let len = bytes.len();
+
+        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.
+        if len > self.buffer.len() - 1 {
+            return Err(fmt::Error);
+        }
+
+        let buffer = core::mem::take(&mut self.buffer);
+        // We break the null termination invariant for a short while.
+        buffer[..len].copy_from_slice(bytes);
+        self.buffer = &mut buffer[len..];
+
+        // INVARIANT: We null terminate the buffer.
+        self.buffer[0] = 0;
+
+        Ok(())
+    }
+}
+
 /// An owned string that is guaranteed to have exactly one `NUL` byte, which is at the end.
 ///
 /// Used for interoperability with kernel APIs that take C strings.

-- 
2.47.2

[PATCH v3 06/16] rust: block: normalize imports for `gen_disk.rs`

[Andreas Hindborg]

Clean up the import statements in `gen_disk.rs` to make the code easier to
maintain.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block/mq/gen_disk.rs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index cd54cd64ea88..679ee1bb2195 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -5,9 +5,13 @@
 //! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
 //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
 
-use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
-use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
-use crate::{error, static_lock_class};
+use crate::{
+    bindings,
+    block::mq::{raw_writer::RawWriter, Operations, TagSet},
+    error::{self, from_err_ptr, Result},
+    static_lock_class,
+    sync::Arc,
+};
 use core::fmt::{self, Write};
 
 /// A builder for [`GenDisk`].

-- 
2.47.2

[PATCH v3 07/16] rust: block: use `NullTerminatedFormatter`

[Andreas Hindborg]

Use the new `NullTerminatedFormatter` to write the name of a `GenDisk` to
the name buffer. This new formatter automatically adds a trailing null
marker after the written characters, so we don't need to append that at the
call site any longer.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block/mq/gen_disk.rs   | 11 ++++++-----
 rust/kernel/block/mq/raw_writer.rs |  1 +
 rust/kernel/str.rs                 |  1 -
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 679ee1bb2195..39be2a31337f 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -7,9 +7,10 @@
 
 use crate::{
     bindings,
-    block::mq::{raw_writer::RawWriter, Operations, TagSet},
+    block::mq::{Operations, TagSet},
     error::{self, from_err_ptr, Result},
     static_lock_class,
+    str::NullTerminatedFormatter,
     sync::Arc,
 };
 use core::fmt::{self, Write};
@@ -143,14 +144,14 @@ pub fn build<T: Operations>(
         // SAFETY: `gendisk` is a valid pointer as we initialized it above
         unsafe { (*gendisk).fops = &TABLE };
 
-        let mut raw_writer = RawWriter::from_array(
+        let mut writer = NullTerminatedFormatter::from_array(
             // SAFETY: `gendisk` points to a valid and initialized instance. We
             // have exclusive access, since the disk is not added to the VFS
             // yet.
             unsafe { &mut (*gendisk).disk_name },
-        )?;
-        raw_writer.write_fmt(name)?;
-        raw_writer.write_char('\0')?;
+        )
+        .ok_or(error::code::EINVAL)?;
+        writer.write_fmt(name)?;
 
         // SAFETY: `gendisk` points to a valid and initialized instance of
         // `struct gendisk`. `set_capacity` takes a lock to synchronize this
diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
index 7e2159e4f6a6..0aef55703e71 100644
--- a/rust/kernel/block/mq/raw_writer.rs
+++ b/rust/kernel/block/mq/raw_writer.rs
@@ -24,6 +24,7 @@ fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
         Ok(Self { buffer, pos: 0 })
     }
 
+    #[expect(dead_code)]
     pub(crate) fn from_array<const N: usize>(
         a: &'a mut [crate::ffi::c_char; N],
     ) -> Result<RawWriter<'a>> {
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index c58925438c6e..7396c49174cd 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -858,7 +858,6 @@ pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
         Some(Self { buffer })
     }
 
-    #[expect(dead_code)]
     pub(crate) fn from_array<const N: usize>(
         buffer: &'a mut [crate::ffi::c_char; N],
     ) -> Option<NullTerminatedFormatter<'a>> {

-- 
2.47.2

[PATCH v3 08/16] rust: block: remove `RawWriter`

[Andreas Hindborg]

`RawWriter` is now dead code, so remove it.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block/mq.rs            |  1 -
 rust/kernel/block/mq/raw_writer.rs | 56 --------------------------------------
 2 files changed, 57 deletions(-)

diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
index fb0f393c1cea..faa3ccb5a49a 100644
--- a/rust/kernel/block/mq.rs
+++ b/rust/kernel/block/mq.rs
@@ -89,7 +89,6 @@
 
 pub mod gen_disk;
 mod operations;
-mod raw_writer;
 mod request;
 mod tag_set;
 
diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
deleted file mode 100644
index 0aef55703e71..000000000000
--- a/rust/kernel/block/mq/raw_writer.rs
+++ /dev/null
@@ -1,56 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-use core::fmt::{self, Write};
-
-use crate::error::Result;
-use crate::prelude::EINVAL;
-
-/// A mutable reference to a byte buffer where a string can be written into.
-///
-/// # Invariants
-///
-/// `buffer` is always null terminated.
-pub(crate) struct RawWriter<'a> {
-    buffer: &'a mut [u8],
-    pos: usize,
-}
-
-impl<'a> RawWriter<'a> {
-    /// Create a new `RawWriter` instance.
-    fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
-        *(buffer.last_mut().ok_or(EINVAL)?) = 0;
-
-        // INVARIANT: We null terminated the buffer above.
-        Ok(Self { buffer, pos: 0 })
-    }
-
-    #[expect(dead_code)]
-    pub(crate) fn from_array<const N: usize>(
-        a: &'a mut [crate::ffi::c_char; N],
-    ) -> Result<RawWriter<'a>> {
-        Self::new(
-            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
-            // at least `N` bytes.
-            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
-        )
-    }
-}
-
-impl Write for RawWriter<'_> {
-    fn write_str(&mut self, s: &str) -> fmt::Result {
-        let bytes = s.as_bytes();
-        let len = bytes.len();
-
-        // We do not want to overwrite our null terminator
-        if self.pos + len > self.buffer.len() - 1 {
-            return Err(fmt::Error);
-        }
-
-        // INVARIANT: We are not overwriting the last byte
-        self.buffer[self.pos..self.pos + len].copy_from_slice(bytes);
-
-        self.pos += len;
-
-        Ok(())
-    }
-}

-- 
2.47.2

[PATCH v3 09/16] rust: block: remove trait bound from `mq::Request` definition

[Andreas Hindborg]

Remove the trait bound `T:Operations` from `mq::Request`. The bound is not
required, so remove it to reduce complexity.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block/mq/request.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index 4a5b7ec914ef..2d14a6261a31 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -53,7 +53,7 @@
 /// [`struct request`]: srctree/include/linux/blk-mq.h
 ///
 #[repr(transparent)]
-pub struct Request<T: Operations>(Opaque<bindings::request>, PhantomData<T>);
+pub struct Request<T>(Opaque<bindings::request>, PhantomData<T>);
 
 impl<T: Operations> Request<T> {
     /// Create an [`ARef<Request>`] from a [`struct request`] pointer.

-- 
2.47.2

[PATCH v3 10/16] rust: block: add block related constants

[Andreas Hindborg]

Add a few block subsystem constants to the rust `kernel::block` name space.
This makes it easier to access the constants from rust code.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
index 150f710efe5b..7461adf4d7e0 100644
--- a/rust/kernel/block.rs
+++ b/rust/kernel/block.rs
@@ -3,3 +3,15 @@
 //! Types for working with the block layer.
 
 pub mod mq;
+
+/// Bit mask for masking out [`SECTOR_SIZE`]
+pub const SECTOR_MASK: u32 = bindings::SECTOR_MASK;
+
+/// Sectors are size `1 << SECTOR_SHIFT`.
+pub const SECTOR_SHIFT: u32 = bindings::SECTOR_SHIFT;
+
+/// Size of a sector.
+pub const SECTOR_SIZE: u32 = bindings::SECTOR_SIZE;
+
+/// Power of two difference in size of a page and size of a sector.
+pub const PAGE_SECTORS_SHIFT: u32 = bindings::PAGE_SECTORS_SHIFT;

-- 
2.47.2

[PATCH v3 11/16] rnull: move driver to separate directory

[Andreas Hindborg]

The rust null block driver is about to gain some additional modules. Rather
than pollute the current directory, move the driver to a subdirectory.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 MAINTAINERS                        |  2 +-
 drivers/block/Kconfig              | 10 +---------
 drivers/block/Makefile             |  4 +---
 drivers/block/rnull/Kconfig        | 13 +++++++++++++
 drivers/block/rnull/Makefile       |  3 +++
 drivers/block/{ => rnull}/rnull.rs |  0
 6 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0c1d245bf7b8..29b14aec3559 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4246,7 +4246,7 @@ W:	https://rust-for-linux.com
 B:	https://github.com/Rust-for-Linux/linux/issues
 C:	https://rust-for-linux.zulipchat.com/#narrow/stream/Block
 T:	git https://github.com/Rust-for-Linux/linux.git rust-block-next
-F:	drivers/block/rnull.rs
+F:	drivers/block/rnull/
 F:	rust/kernel/block.rs
 F:	rust/kernel/block/
 
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 0f70e2374e7f..6b50dbc0495b 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -17,6 +17,7 @@ menuconfig BLK_DEV
 if BLK_DEV
 
 source "drivers/block/null_blk/Kconfig"
+source "drivers/block/rnull/Kconfig"
 
 config BLK_DEV_FD
 	tristate "Normal floppy disk support"
@@ -354,15 +355,6 @@ config VIRTIO_BLK
 	  This is the virtual block driver for virtio.  It can be used with
           QEMU based VMMs (like KVM or Xen).  Say Y or M.
 
-config BLK_DEV_RUST_NULL
-	tristate "Rust null block driver (Experimental)"
-	depends on RUST
-	help
-	  This is the Rust implementation of the null block driver. For now it
-	  is only a minimal stub.
-
-	  If unsure, say N.
-
 config BLK_DEV_RBD
 	tristate "Rados block device (RBD)"
 	depends on INET && BLOCK
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 097707aca725..aba3e93d5014 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -9,9 +9,6 @@
 # needed for trace events
 ccflags-y				+= -I$(src)
 
-obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
-rnull_mod-y := rnull.o
-
 obj-$(CONFIG_MAC_FLOPPY)	+= swim3.o
 obj-$(CONFIG_BLK_DEV_SWIM)	+= swim_mod.o
 obj-$(CONFIG_BLK_DEV_FD)	+= floppy.o
@@ -39,6 +36,7 @@ obj-$(CONFIG_ZRAM) += zram/
 obj-$(CONFIG_BLK_DEV_RNBD)	+= rnbd/
 
 obj-$(CONFIG_BLK_DEV_NULL_BLK)	+= null_blk/
+obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull/
 
 obj-$(CONFIG_BLK_DEV_UBLK)			+= ublk_drv.o
 obj-$(CONFIG_BLK_DEV_ZONED_LOOP) += zloop.o
diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
new file mode 100644
index 000000000000..6dc5aff96bf4
--- /dev/null
+++ b/drivers/block/rnull/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Rust null block device driver configuration
+
+config BLK_DEV_RUST_NULL
+	tristate "Rust null block driver (Experimental)"
+	depends on RUST
+	help
+	  This is the Rust implementation of the null block driver. Like
+	  the C version, the driver allows the user to create virutal block
+	  devices that can be configured via various configuration options.
+
+	  If unsure, say N.
diff --git a/drivers/block/rnull/Makefile b/drivers/block/rnull/Makefile
new file mode 100644
index 000000000000..11cfa5e615dc
--- /dev/null
+++ b/drivers/block/rnull/Makefile
@@ -0,0 +1,3 @@
+
+obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
+rnull_mod-y := rnull.o
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull/rnull.rs
similarity index 100%
rename from drivers/block/rnull.rs
rename to drivers/block/rnull/rnull.rs

-- 
2.47.2

[PATCH v3 12/16] rnull: enable configuration via `configfs`

[Andreas Hindborg]

Allow rust null block devices to be configured and instantiated via
`configfs`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 drivers/block/rnull/Kconfig      |   2 +-
 drivers/block/rnull/configfs.rs  | 220 +++++++++++++++++++++++++++++++++++++++
 drivers/block/rnull/rnull.rs     |  58 ++++++-----
 rust/kernel/block/mq/gen_disk.rs |   2 +-
 4 files changed, 253 insertions(+), 29 deletions(-)

diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
index 6dc5aff96bf4..7bc5b376c128 100644
--- a/drivers/block/rnull/Kconfig
+++ b/drivers/block/rnull/Kconfig
@@ -4,7 +4,7 @@
 
 config BLK_DEV_RUST_NULL
 	tristate "Rust null block driver (Experimental)"
-	depends on RUST
+	depends on RUST && CONFIGFS_FS
 	help
 	  This is the Rust implementation of the null block driver. Like
 	  the C version, the driver allows the user to create virutal block
diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
new file mode 100644
index 000000000000..6c0e3bbb36ec
--- /dev/null
+++ b/drivers/block/rnull/configfs.rs
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::{NullBlkDevice, THIS_MODULE};
+use core::fmt::Write;
+use kernel::{
+    block::mq::gen_disk::{GenDisk, GenDiskBuilder},
+    c_str,
+    configfs::{self, AttributeOperations},
+    configfs_attrs, new_mutex,
+    page::PAGE_SIZE,
+    prelude::*,
+    str::CString,
+    sync::Mutex,
+};
+use pin_init::PinInit;
+
+pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
+    let item_type = configfs_attrs! {
+        container: configfs::Subsystem<Config>,
+        data: Config,
+        child: DeviceConfig,
+        attributes: [
+            features: 0,
+        ],
+    };
+
+    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
+}
+
+#[pin_data]
+pub(crate) struct Config {}
+
+#[vtable]
+impl AttributeOperations<0> for Config {
+    type Data = Config;
+
+    fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+        writer.write_str("blocksize,size,rotational\n")?;
+        Ok(writer.bytes_written())
+    }
+}
+
+#[vtable]
+impl configfs::GroupOperations for Config {
+    type Child = DeviceConfig;
+
+    fn make_group(
+        &self,
+        name: &CStr,
+    ) -> Result<impl PinInit<configfs::Group<DeviceConfig>, Error>> {
+        let item_type = configfs_attrs! {
+            container: configfs::Group<DeviceConfig>,
+            data: DeviceConfig,
+            attributes: [
+                // Named for compatibility with C null_blk
+                power: 0,
+                blocksize: 1,
+                rotational: 2,
+                size: 3,
+            ],
+        };
+
+        Ok(configfs::Group::new(
+            name.try_into()?,
+            item_type,
+            // TODO: cannot coerce new_mutex!() to impl PinInit<_, Error>, so put mutex inside
+            try_pin_init!( DeviceConfig {
+                data <- new_mutex!( DeviceConfigInner {
+                    powered: false,
+                    block_size: 4096,
+                    rotational: false,
+                    disk: None,
+                    capacity_mib: 4096,
+                    name: name.try_into()?,
+                }),
+            }),
+        ))
+    }
+}
+
+#[pin_data]
+pub(crate) struct DeviceConfig {
+    #[pin]
+    data: Mutex<DeviceConfigInner>,
+}
+
+#[pin_data]
+struct DeviceConfigInner {
+    powered: bool,
+    name: CString,
+    block_size: u32,
+    rotational: bool,
+    capacity_mib: u64,
+    disk: Option<GenDisk<NullBlkDevice>>,
+}
+
+#[vtable]
+impl configfs::AttributeOperations<0> for DeviceConfig {
+    type Data = DeviceConfig;
+
+    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+
+        if this.data.lock().powered {
+            writer.write_fmt(fmt!("1\n"))?;
+        } else {
+            writer.write_fmt(fmt!("0\n"))?;
+        }
+
+        Ok(writer.bytes_written())
+    }
+
+    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+        let power_op: bool = core::str::from_utf8(page)?
+            .trim()
+            .parse::<u8>()
+            .map_err(|_| kernel::error::code::EINVAL)?
+            != 0;
+
+        let mut guard = this.data.lock();
+
+        if !guard.powered && power_op {
+            guard.disk = Some(NullBlkDevice::new(
+                &guard.name,
+                guard.block_size,
+                guard.rotational,
+                guard.capacity_mib,
+            )?);
+            guard.powered = true;
+        } else if guard.powered && !power_op {
+            drop(guard.disk.take());
+            guard.powered = false;
+        }
+
+        Ok(())
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<1> for DeviceConfig {
+    type Data = DeviceConfig;
+
+    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+        writer.write_fmt(fmt!("{}\n", this.data.lock().block_size))?;
+        Ok(writer.bytes_written())
+    }
+
+    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+        if this.data.lock().powered {
+            return Err(EBUSY);
+        }
+
+        let text = core::str::from_utf8(page)?.trim();
+        let value = text
+            .parse::<u32>()
+            .map_err(|_| kernel::error::code::EINVAL)?;
+
+        GenDiskBuilder::validate_block_size(value)?;
+        this.data.lock().block_size = value;
+        Ok(())
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<2> for DeviceConfig {
+    type Data = DeviceConfig;
+
+    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+
+        if this.data.lock().rotational {
+            writer.write_fmt(fmt!("1\n"))?;
+        } else {
+            writer.write_fmt(fmt!("0\n"))?;
+        }
+
+        Ok(writer.bytes_written())
+    }
+
+    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+        if this.data.lock().powered {
+            return Err(EBUSY);
+        }
+
+        this.data.lock().rotational = core::str::from_utf8(page)?
+            .trim()
+            .parse::<u8>()
+            .map_err(|_| kernel::error::code::EINVAL)?
+            != 0;
+
+        Ok(())
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<3> for DeviceConfig {
+    type Data = DeviceConfig;
+
+    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+        writer.write_fmt(fmt!("{}\n", this.data.lock().capacity_mib))?;
+        Ok(writer.bytes_written())
+    }
+
+    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+        if this.data.lock().powered {
+            return Err(EBUSY);
+        }
+
+        let text = core::str::from_utf8(page)?.trim();
+        let value = text
+            .parse::<u64>()
+            .map_err(|_| kernel::error::code::EINVAL)?;
+
+        this.data.lock().capacity_mib = value;
+        Ok(())
+    }
+}
diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index d07e76ae2c13..d09bc77861e4 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -1,28 +1,26 @@
 // SPDX-License-Identifier: GPL-2.0
 
 //! This is a Rust implementation of the C null block driver.
-//!
-//! Supported features:
-//!
-//! - blk-mq interface
-//! - direct completion
-//! - block size 4k
-//!
-//! The driver is not configurable.
+
+mod configfs;
 
 use kernel::{
     alloc::flags,
-    block::mq::{
+    block::{
         self,
-        gen_disk::{self, GenDisk},
-        Operations, TagSet,
+        mq::{
+            self,
+            gen_disk::{self, GenDisk},
+            Operations, TagSet,
+        },
     },
     error::Result,
-    new_mutex, pr_info,
+    pr_info,
     prelude::*,
-    sync::{Arc, Mutex},
+    sync::Arc,
     types::ARef,
 };
+use pin_init::PinInit;
 
 module! {
     type: NullBlkModule,
@@ -35,33 +33,39 @@
 #[pin_data]
 struct NullBlkModule {
     #[pin]
-    _disk: Mutex<GenDisk<NullBlkDevice>>,
+    configfs_subsystem: kernel::configfs::Subsystem<configfs::Config>,
 }
 
 impl kernel::InPlaceModule for NullBlkModule {
     fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
         pr_info!("Rust null_blk loaded\n");
 
-        // Use a immediately-called closure as a stable `try` block
-        let disk = /* try */ (|| {
-            let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
-
-            gen_disk::GenDiskBuilder::new()
-                .capacity_sectors(4096 << 11)
-                .logical_block_size(4096)?
-                .physical_block_size(4096)?
-                .rotational(false)
-                .build(format_args!("rnullb{}", 0), tagset)
-        })();
-
         try_pin_init!(Self {
-            _disk <- new_mutex!(disk?, "nullb:disk"),
+            configfs_subsystem <- configfs::subsystem(),
         })
     }
 }
 
 struct NullBlkDevice;
 
+impl NullBlkDevice {
+    fn new(
+        name: &CStr,
+        block_size: u32,
+        rotational: bool,
+        capacity_mib: u64,
+    ) -> Result<GenDisk<Self>> {
+        let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
+
+        gen_disk::GenDiskBuilder::new()
+            .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
+            .logical_block_size(block_size)?
+            .physical_block_size(block_size)?
+            .rotational(rotational)
+            .build(fmt!("{}", name.to_str()?), tagset)
+    }
+}
+
 #[vtable]
 impl Operations for NullBlkDevice {
     #[inline(always)]
diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 39be2a31337f..7ab049ec591b 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -50,7 +50,7 @@ pub fn rotational(mut self, rotational: bool) -> Self {
 
     /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`,
     /// and that it is a power of two.
-    fn validate_block_size(size: u32) -> Result {
+    pub fn validate_block_size(size: u32) -> Result {
         if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() {
             Err(error::code::EINVAL)
         } else {

-- 
2.47.2

[PATCH v3 13/16] rust: block: add `GenDisk` private data support

[Andreas Hindborg]

Allow users of the rust block device driver API to install private data in
the `GenDisk` structure.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 drivers/block/rnull/rnull.rs       |  8 ++++---
 rust/kernel/block/mq.rs            |  7 +++---
 rust/kernel/block/mq/gen_disk.rs   | 32 ++++++++++++++++++++++----
 rust/kernel/block/mq/operations.rs | 46 ++++++++++++++++++++++++++++++--------
 4 files changed, 74 insertions(+), 19 deletions(-)

diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index d09bc77861e4..a012c59ecb3c 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -62,14 +62,16 @@ fn new(
             .logical_block_size(block_size)?
             .physical_block_size(block_size)?
             .rotational(rotational)
-            .build(fmt!("{}", name.to_str()?), tagset)
+            .build(fmt!("{}", name.to_str()?), tagset, ())
     }
 }
 
 #[vtable]
 impl Operations for NullBlkDevice {
+    type QueueData = ();
+
     #[inline(always)]
-    fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
+    fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
         mq::Request::end_ok(rq)
             .map_err(|_e| kernel::error::code::EIO)
             // We take no refcounts on the request, so we expect to be able to
@@ -80,5 +82,5 @@ fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
         Ok(())
     }
 
-    fn commit_rqs() {}
+    fn commit_rqs(_queue_data: ()) {}
 }
diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
index faa3ccb5a49a..34b7425fa94d 100644
--- a/rust/kernel/block/mq.rs
+++ b/rust/kernel/block/mq.rs
@@ -69,20 +69,21 @@
 //!
 //! #[vtable]
 //! impl Operations for MyBlkDevice {
+//!     type QueueData = ();
 //!
-//!     fn queue_rq(rq: ARef<Request<Self>>, _is_last: bool) -> Result {
+//!     fn queue_rq(_queue_data: (), rq: ARef<Request<Self>>, _is_last: bool) -> Result {
 //!         Request::end_ok(rq);
 //!         Ok(())
 //!     }
 //!
-//!     fn commit_rqs() {}
+//!     fn commit_rqs(_queue_data: ()) {}
 //! }
 //!
 //! let tagset: Arc<TagSet<MyBlkDevice>> =
 //!     Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
 //! let mut disk = gen_disk::GenDiskBuilder::new()
 //!     .capacity_sectors(4096)
-//!     .build(format_args!("myblk"), tagset)?;
+//!     .build(format_args!("myblk"), tagset, ())?;
 //!
 //! # Ok::<(), kernel::error::Error>(())
 //! ```
diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 7ab049ec591b..954bc64cfa59 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -12,6 +12,7 @@
     static_lock_class,
     str::NullTerminatedFormatter,
     sync::Arc,
+    types::{ForeignOwnable, ScopeGuard},
 };
 use core::fmt::{self, Write};
 
@@ -97,7 +98,14 @@ pub fn build<T: Operations>(
         self,
         name: fmt::Arguments<'_>,
         tagset: Arc<TagSet<T>>,
+        queue_data: T::QueueData,
     ) -> Result<GenDisk<T>> {
+        let data = queue_data.into_foreign();
+        let recover_data = ScopeGuard::new(|| {
+            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
+            unsafe { T::QueueData::from_foreign(data) };
+        });
+
         // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
         let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
 
@@ -112,7 +120,7 @@ pub fn build<T: Operations>(
             bindings::__blk_mq_alloc_disk(
                 tagset.raw_tag_set(),
                 &mut lim,
-                core::ptr::null_mut(),
+                data.cast(),
                 static_lock_class!().as_ptr(),
             )
         })?;
@@ -166,8 +174,12 @@ pub fn build<T: Operations>(
             },
         )?;
 
+        recover_data.dismiss();
+
         // INVARIANT: `gendisk` was initialized above.
         // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
+        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
+        // `__blk_mq_alloc_disk` above.
         Ok(GenDisk {
             _tagset: tagset,
             gendisk,
@@ -179,9 +191,10 @@ pub fn build<T: Operations>(
 ///
 /// # Invariants
 ///
-/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
-/// - `gendisk` was added to the VFS through a call to
-///   `bindings::device_add_disk`.
+///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
+///  - `gendisk` was added to the VFS through a call to
+///    `bindings::device_add_disk`.
+///  - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
 pub struct GenDisk<T: Operations> {
     _tagset: Arc<TagSet<T>>,
     gendisk: *mut bindings::gendisk,
@@ -193,9 +206,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
 
 impl<T: Operations> Drop for GenDisk<T> {
     fn drop(&mut self) {
+        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
+        // and initialized instance of `struct gendisk`, and, `queuedata` was
+        // initialized with the result of a call to
+        // `ForeignOwnable::into_foreign`.
+        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
+
         // SAFETY: By type invariant, `self.gendisk` points to a valid and
         // initialized instance of `struct gendisk`, and it was previously added
         // to the VFS.
         unsafe { bindings::del_gendisk(self.gendisk) };
+
+        // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
+        // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
+        // `ForeignOwnable::from_foreign` is only called here.
+        let _queue_data = unsafe { T::QueueData::from_foreign(queue_data.cast()) };
     }
 }
diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index 864ff379dc91..c50959d5517b 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -6,14 +6,15 @@
 
 use crate::{
     bindings,
-    block::mq::request::RequestDataWrapper,
-    block::mq::Request,
+    block::mq::{request::RequestDataWrapper, Request},
     error::{from_result, Result},
     prelude::*,
-    types::ARef,
+    types::{ARef, ForeignOwnable},
 };
 use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering};
 
+type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;
+
 /// Implement this trait to interface blk-mq as block devices.
 ///
 /// To implement a block device driver, implement this trait as described in the
@@ -26,12 +27,20 @@
 /// [module level documentation]: kernel::block::mq
 #[macros::vtable]
 pub trait Operations: Sized {
+    /// Data associated with the `struct request_queue` that is allocated for
+    /// the `GenDisk` associated with this `Operations` implementation.
+    type QueueData: ForeignOwnable;
+
     /// Called by the kernel to queue a request with the driver. If `is_last` is
     /// `false`, the driver is allowed to defer committing the request.
-    fn queue_rq(rq: ARef<Request<Self>>, is_last: bool) -> Result;
+    fn queue_rq(
+        queue_data: ForeignBorrowed<'_, Self::QueueData>,
+        rq: ARef<Request<Self>>,
+        is_last: bool,
+    ) -> Result;
 
     /// Called by the kernel to indicate that queued requests should be submitted.
-    fn commit_rqs();
+    fn commit_rqs(queue_data: ForeignBorrowed<'_, Self::QueueData>);
 
     /// Called by the kernel to poll the device for completed requests. Only
     /// used for poll queues.
@@ -70,7 +79,7 @@ impl<T: Operations> OperationsVTable<T> {
     ///   promise to not access the request until the driver calls
     ///   `bindings::blk_mq_end_request` for the request.
     unsafe extern "C" fn queue_rq_callback(
-        _hctx: *mut bindings::blk_mq_hw_ctx,
+        hctx: *mut bindings::blk_mq_hw_ctx,
         bd: *const bindings::blk_mq_queue_data,
     ) -> bindings::blk_status_t {
         // SAFETY: `bd.rq` is valid as required by the safety requirement for
@@ -88,10 +97,20 @@ impl<T: Operations> OperationsVTable<T> {
         //    reference counted by `ARef` until then.
         let rq = unsafe { Request::aref_from_raw((*bd).rq) };
 
+        // SAFETY: `hctx` is valid as required by this function.
+        let queue_data = unsafe { (*(*hctx).queue).queuedata };
+
+        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
+        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
+        // `ForeignOwnable::from_foreign()` is only called when the tagset is
+        // dropped, which happens after we are dropped.
+        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
+
         // SAFETY: We have exclusive access and we just set the refcount above.
         unsafe { Request::start_unchecked(&rq) };
 
         let ret = T::queue_rq(
+            queue_data,
             rq,
             // SAFETY: `bd` is valid as required by the safety requirement for
             // this function.
@@ -110,9 +129,18 @@ impl<T: Operations> OperationsVTable<T> {
     ///
     /// # Safety
     ///
-    /// This function may only be called by blk-mq C infrastructure.
-    unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
-        T::commit_rqs()
+    /// This function may only be called by blk-mq C infrastructure. The caller
+    /// must ensure that `hctx` is valid.
+    unsafe extern "C" fn commit_rqs_callback(hctx: *mut bindings::blk_mq_hw_ctx) {
+        // SAFETY: `hctx` is valid as required by this function.
+        let queue_data = unsafe { (*(*hctx).queue).queuedata };
+
+        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
+        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
+        // `ForeignOwnable::from_foreign()` is only called when the tagset is
+        // dropped, which happens after we are dropped.
+        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
+        T::commit_rqs(queue_data)
     }
 
     /// This function is called by the C kernel. It is not currently

-- 
2.47.2

[PATCH v3 14/16] rust: block: mq: fix spelling in a safety comment

[Andreas Hindborg]

Add code block quotes to a safety comment.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/block/mq/request.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index 2d14a6261a31..873d00db74dd 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -143,7 +143,7 @@ pub(crate) unsafe fn wrapper_ptr(this: *mut Self) -> NonNull<RequestDataWrapper>
         // valid allocation.
         let wrapper_ptr =
             unsafe { bindings::blk_mq_rq_to_pdu(request_ptr).cast::<RequestDataWrapper>() };
-        // SAFETY: By C API contract, wrapper_ptr points to a valid allocation
+        // SAFETY: By C API contract, `wrapper_ptr` points to a valid allocation
         // and is not null.
         unsafe { NonNull::new_unchecked(wrapper_ptr) }
     }

-- 
2.47.2

[PATCH v3 15/16] rust: block: add remote completion to `Request`

[Andreas Hindborg]

Allow users of rust block device driver API to schedule completion of
requests via `blk_mq_complete_request_remote`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 drivers/block/rnull/rnull.rs       |  9 +++++++++
 rust/kernel/block/mq.rs            |  6 ++++++
 rust/kernel/block/mq/operations.rs | 19 +++++++++++++++----
 rust/kernel/block/mq/request.rs    | 17 +++++++++++++++++
 4 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index a012c59ecb3c..371786be7f47 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -83,4 +83,13 @@ fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Res
     }
 
     fn commit_rqs(_queue_data: ()) {}
+
+    fn complete(rq: ARef<mq::Request<Self>>) {
+        mq::Request::end_ok(rq)
+            .map_err(|_e| kernel::error::code::EIO)
+            // We take no refcounts on the request, so we expect to be able to
+            // end the request. The request reference must be unique at this
+            // point, and so `end_ok` cannot fail.
+            .expect("Fatal error - expected to be able to end request");
+    }
 }
diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
index 34b7425fa94d..551ef38efea2 100644
--- a/rust/kernel/block/mq.rs
+++ b/rust/kernel/block/mq.rs
@@ -77,6 +77,12 @@
 //!     }
 //!
 //!     fn commit_rqs(_queue_data: ()) {}
+//!
+//!     fn complete(rq: ARef<Request<Self>>) {
+//!         Request::end_ok(rq)
+//!             .map_err(|_e| kernel::error::code::EIO)
+//!             .expect("Fatal error - expected to be able to end request");
+//!     }
 //! }
 //!
 //! let tagset: Arc<TagSet<MyBlkDevice>> =
diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index c50959d5517b..b6b26cebd4f5 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -42,6 +42,9 @@ fn queue_rq(
     /// Called by the kernel to indicate that queued requests should be submitted.
     fn commit_rqs(queue_data: ForeignBorrowed<'_, Self::QueueData>);
 
+    /// Called by the kernel when the request is completed.
+    fn complete(rq: ARef<Request<Self>>);
+
     /// Called by the kernel to poll the device for completed requests. Only
     /// used for poll queues.
     fn poll() -> bool {
@@ -143,13 +146,21 @@ impl<T: Operations> OperationsVTable<T> {
         T::commit_rqs(queue_data)
     }
 
-    /// This function is called by the C kernel. It is not currently
-    /// implemented, and there is no way to exercise this code path.
+    /// This function is called by the C kernel. A pointer to this function is
+    /// installed in the `blk_mq_ops` vtable for the driver.
     ///
     /// # Safety
     ///
-    /// This function may only be called by blk-mq C infrastructure.
-    unsafe extern "C" fn complete_callback(_rq: *mut bindings::request) {}
+    /// This function may only be called by blk-mq C infrastructure. `rq` must
+    /// point to a valid request that has been marked as completed. The pointee
+    /// of `rq` must be valid for write for the duration of this function.
+    unsafe extern "C" fn complete_callback(rq: *mut bindings::request) {
+        // SAFETY: This function can only be dispatched through
+        // `Request::complete`. We leaked a refcount then which we pick back up
+        // now.
+        let aref = unsafe { Request::aref_from_raw(rq) };
+        T::complete(aref);
+    }
 
     /// This function is called by the C kernel. A pointer to this function is
     /// installed in the `blk_mq_ops` vtable for the driver.
diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index 873d00db74dd..244578a802ce 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -130,6 +130,23 @@ pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> {
         Ok(())
     }
 
+    /// Complete the request by scheduling `Operations::complete` for
+    /// execution.
+    ///
+    /// The function may be scheduled locally, via SoftIRQ or remotely via IPMI.
+    /// See `blk_mq_complete_request_remote` in [`blk-mq.c`] for details.
+    ///
+    /// [`blk-mq.c`]: srctree/block/blk-mq.c
+    pub fn complete(this: ARef<Self>) {
+        let ptr = ARef::into_raw(this).cast::<bindings::request>().as_ptr();
+        // SAFETY: By type invariant, `self.0` is a valid `struct request`
+        if !unsafe { bindings::blk_mq_complete_request_remote(ptr) } {
+            // SAFETY: We released a refcount above that we can reclaim here.
+            let this = unsafe { Request::aref_from_raw(ptr) };
+            T::complete(this);
+        }
+    }
+
     /// Return a pointer to the [`RequestDataWrapper`] stored in the private area
     /// of the request structure.
     ///

-- 
2.47.2

[PATCH v3 16/16] rnull: add soft-irq completion support

[Andreas Hindborg]

rnull currently only supports direct completion. Add option for completing
requests across CPU nodes via soft IRQ or IPI.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 drivers/block/rnull/configfs.rs | 61 +++++++++++++++++++++++++++++++++++++++--
 drivers/block/rnull/rnull.rs    | 32 +++++++++++++--------
 2 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
index 6c0e3bbb36ec..3ae84dfc8d62 100644
--- a/drivers/block/rnull/configfs.rs
+++ b/drivers/block/rnull/configfs.rs
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use super::{NullBlkDevice, THIS_MODULE};
-use core::fmt::Write;
+use core::fmt::{Display, Write};
 use kernel::{
     block::mq::gen_disk::{GenDisk, GenDiskBuilder},
     c_str,
@@ -36,7 +36,7 @@ impl AttributeOperations<0> for Config {
 
     fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
         let mut writer = kernel::str::Formatter::new(page);
-        writer.write_str("blocksize,size,rotational\n")?;
+        writer.write_str("blocksize,size,rotational,irqmode\n")?;
         Ok(writer.bytes_written())
     }
 }
@@ -58,6 +58,7 @@ fn make_group(
                 blocksize: 1,
                 rotational: 2,
                 size: 3,
+                irqmode: 4,
             ],
         };
 
@@ -72,6 +73,7 @@ fn make_group(
                     rotational: false,
                     disk: None,
                     capacity_mib: 4096,
+                    irq_mode: IRQMode::None,
                     name: name.try_into()?,
                 }),
             }),
@@ -79,6 +81,34 @@ fn make_group(
     }
 }
 
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum IRQMode {
+    None,
+    Soft,
+}
+
+impl TryFrom<u8> for IRQMode {
+    type Error = kernel::error::Error;
+
+    fn try_from(value: u8) -> Result<Self> {
+        match value {
+            0 => Ok(Self::None),
+            1 => Ok(Self::Soft),
+            _ => Err(kernel::error::code::EINVAL),
+        }
+    }
+}
+
+impl Display for IRQMode {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        match self {
+            Self::None => f.write_str("0")?,
+            Self::Soft => f.write_str("1")?,
+        }
+        Ok(())
+    }
+}
+
 #[pin_data]
 pub(crate) struct DeviceConfig {
     #[pin]
@@ -92,6 +122,7 @@ struct DeviceConfigInner {
     block_size: u32,
     rotational: bool,
     capacity_mib: u64,
+    irq_mode: IRQMode,
     disk: Option<GenDisk<NullBlkDevice>>,
 }
 
@@ -126,6 +157,7 @@ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
                 guard.block_size,
                 guard.rotational,
                 guard.capacity_mib,
+                guard.irq_mode,
             )?);
             guard.powered = true;
         } else if guard.powered && !power_op {
@@ -218,3 +250,28 @@ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
         Ok(())
     }
 }
+
+#[vtable]
+impl configfs::AttributeOperations<4> for DeviceConfig {
+    type Data = DeviceConfig;
+
+    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
+        let mut writer = kernel::str::Formatter::new(page);
+        writer.write_fmt(fmt!("{}\n", this.data.lock().irq_mode))?;
+        Ok(writer.bytes_written())
+    }
+
+    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
+        if this.data.lock().powered {
+            return Err(EBUSY);
+        }
+
+        let text = core::str::from_utf8(page)?.trim();
+        let value = text
+            .parse::<u8>()
+            .map_err(|_| kernel::error::code::EINVAL)?;
+
+        this.data.lock().irq_mode = IRQMode::try_from(value)?;
+        Ok(())
+    }
+}
diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index 371786be7f47..85b1509a3106 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -4,6 +4,7 @@
 
 mod configfs;
 
+use configfs::IRQMode;
 use kernel::{
     alloc::flags,
     block::{
@@ -54,35 +55,44 @@ fn new(
         block_size: u32,
         rotational: bool,
         capacity_mib: u64,
+        irq_mode: IRQMode,
     ) -> Result<GenDisk<Self>> {
         let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
 
+        let queue_data = Box::new(QueueData { irq_mode }, flags::GFP_KERNEL)?;
+
         gen_disk::GenDiskBuilder::new()
             .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
             .logical_block_size(block_size)?
             .physical_block_size(block_size)?
             .rotational(rotational)
-            .build(fmt!("{}", name.to_str()?), tagset, ())
+            .build(fmt!("{}", name.to_str()?), tagset, queue_data)
     }
 }
 
+struct QueueData {
+    irq_mode: IRQMode,
+}
+
 #[vtable]
 impl Operations for NullBlkDevice {
-    type QueueData = ();
+    type QueueData = KBox<QueueData>;
 
     #[inline(always)]
-    fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
-        mq::Request::end_ok(rq)
-            .map_err(|_e| kernel::error::code::EIO)
-            // We take no refcounts on the request, so we expect to be able to
-            // end the request. The request reference must be unique at this
-            // point, and so `end_ok` cannot fail.
-            .expect("Fatal error - expected to be able to end request");
-
+    fn queue_rq(queue_data: &QueueData, rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
+        match queue_data.irq_mode {
+            IRQMode::None => mq::Request::end_ok(rq)
+                .map_err(|_e| kernel::error::code::EIO)
+                // We take no refcounts on the request, so we expect to be able to
+                // end the request. The request reference must be unique at this
+                // point, and so `end_ok` cannot fail.
+                .expect("Fatal error - expected to be able to end request"),
+            IRQMode::Soft => mq::Request::complete(rq),
+        }
         Ok(())
     }
 
-    fn commit_rqs(_queue_data: ()) {}
+    fn commit_rqs(_queue_data: &QueueData) {}
 
     fn complete(rq: ARef<mq::Request<Self>>) {
         mq::Request::end_ok(rq)

-- 
2.47.2

Re: [PATCH v3 02/16] rust: str: allow `str::Formatter` to format into `&mut [u8]`.

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:03PM +0200, Andreas Hindborg wrote:
> Improve `Formatter` so that it can write to an array or slice buffer.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +    #[expect(dead_code)]

This could just be a public type to avoid this.

Re: [PATCH v3 03/16] rust: str: expose `str::Formatter::new` publicly.

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:04PM +0200, Andreas Hindborg wrote:
> rnull is going to make use of `str::Formatter`, so expose it with public
> visibility.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 04/16] rust: str: make `RawFormatter::bytes_written` public.

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:05PM +0200, Andreas Hindborg wrote:
> rnull is going to make use of `kernel::str::RawFormatter::bytes_written`,
> so make the visibility public.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Making methods public can be part of making the type itself public. I
don't think a separate patch is needed.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:06PM +0200, Andreas Hindborg wrote:
> Add `NullTerminatedFormatter`, a formatter that writes a null terminated
> string to an array or slice buffer. Because this type needs to manage the
> trailing null marker, the existing formatters cannot be used to implement
> this type.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index b1bc584803b0..c58925438c6e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>      }
>  }
>  
> +/// A mutable reference to a byte buffer where a string can be written into.
> +///
> +/// The buffer will be automatically null terminated after the last written character.
> +///
> +/// # Invariants
> +///
> +/// `buffer` is always null terminated.

Since you modify the buffer range, the actual invariant is that the
first byte of `buffer` is zero.

> +pub(crate) struct NullTerminatedFormatter<'a> {

Isn't it called "nul" rather than "null"? My understanding is that
"null" is for the pointer case, and "nul" is the name of the ascii
character at codepoint zero.

> +    buffer: &'a mut [u8],
> +}
> +
> +impl<'a> NullTerminatedFormatter<'a> {
> +    /// Create a new [`Self`] instance.
> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
> +        *(buffer.first_mut()?) = 0;
> +
> +        // INVARIANT: We null terminated the buffer above.
> +        Some(Self { buffer })
> +    }
> +
> +    #[expect(dead_code)]
> +    pub(crate) fn from_array<const N: usize>(
> +        buffer: &'a mut [crate::ffi::c_char; N],
> +    ) -> Option<NullTerminatedFormatter<'a>> {

Can't you just call `::new` where you use this method?

> +        Self::new(buffer)
> +    }
> +}
> +
> +impl Write for NullTerminatedFormatter<'_> {
> +    fn write_str(&mut self, s: &str) -> fmt::Result {
> +        let bytes = s.as_bytes();
> +        let len = bytes.len();
> +
> +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.

overflow -> underflow

> +        if len > self.buffer.len() - 1 {

this is just `len >= self.buffer.len()`.

> +            return Err(fmt::Error);
> +        }
> +
> +        let buffer = core::mem::take(&mut self.buffer);
> +        // We break the null termination invariant for a short while.
> +        buffer[..len].copy_from_slice(bytes);
> +        self.buffer = &mut buffer[len..];
> +
> +        // INVARIANT: We null terminate the buffer.
> +        self.buffer[0] = 0;
> +
> +        Ok(())
> +    }
> +}
> +
>  /// An owned string that is guaranteed to have exactly one `NUL` byte, which is at the end.
>  ///
>  /// Used for interoperability with kernel APIs that take C strings.
> 
> -- 
> 2.47.2
> 
> 

Re: [PATCH v3 07/16] rust: block: use `NullTerminatedFormatter`

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:08PM +0200, Andreas Hindborg wrote:
> Use the new `NullTerminatedFormatter` to write the name of a `GenDisk` to
> the name buffer. This new formatter automatically adds a trailing null
> marker after the written characters, so we don't need to append that at the
> call site any longer.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 08/16] rust: block: remove `RawWriter`

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:09PM +0200, Andreas Hindborg wrote:
> `RawWriter` is now dead code, so remove it.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Could be squashed into previous commit IMO, but no big deal.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 10/16] rust: block: add block related constants

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:11PM +0200, Andreas Hindborg wrote:
> Add a few block subsystem constants to the rust `kernel::block` name space.
> This makes it easier to access the constants from rust code.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/block.rs | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
> index 150f710efe5b..7461adf4d7e0 100644
> --- a/rust/kernel/block.rs
> +++ b/rust/kernel/block.rs
> @@ -3,3 +3,15 @@
>  //! Types for working with the block layer.
>  
>  pub mod mq;
> +
> +/// Bit mask for masking out [`SECTOR_SIZE`]
> +pub const SECTOR_MASK: u32 = bindings::SECTOR_MASK;
> +
> +/// Sectors are size `1 << SECTOR_SHIFT`.
> +pub const SECTOR_SHIFT: u32 = bindings::SECTOR_SHIFT;
> +
> +/// Size of a sector.
> +pub const SECTOR_SIZE: u32 = bindings::SECTOR_SIZE;
> +
> +/// Power of two difference in size of a page and size of a sector.
> +pub const PAGE_SECTORS_SHIFT: u32 = bindings::PAGE_SECTORS_SHIFT;

I was looking for the user to double-check whether u32 was the right
choice, but I can't find it. It looks like you don't use these yet?

Alice

Re: [PATCH v3 11/16] rnull: move driver to separate directory

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:12PM +0200, Andreas Hindborg wrote:
> The rust null block driver is about to gain some additional modules. Rather
> than pollute the current directory, move the driver to a subdirectory.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 12/16] rnull: enable configuration via `configfs`

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:13PM +0200, Andreas Hindborg wrote:
> Allow rust null block devices to be configured and instantiated via
> `configfs`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  drivers/block/rnull/Kconfig      |   2 +-
>  drivers/block/rnull/configfs.rs  | 220 +++++++++++++++++++++++++++++++++++++++
>  drivers/block/rnull/rnull.rs     |  58 ++++++-----
>  rust/kernel/block/mq/gen_disk.rs |   2 +-
>  4 files changed, 253 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
> index 6dc5aff96bf4..7bc5b376c128 100644
> --- a/drivers/block/rnull/Kconfig
> +++ b/drivers/block/rnull/Kconfig
> @@ -4,7 +4,7 @@
>  
>  config BLK_DEV_RUST_NULL
>  	tristate "Rust null block driver (Experimental)"
> -	depends on RUST
> +	depends on RUST && CONFIGFS_FS
>  	help
>  	  This is the Rust implementation of the null block driver. Like
>  	  the C version, the driver allows the user to create virutal block
> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
> new file mode 100644
> index 000000000000..6c0e3bbb36ec
> --- /dev/null
> +++ b/drivers/block/rnull/configfs.rs
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::{NullBlkDevice, THIS_MODULE};
> +use core::fmt::Write;
> +use kernel::{
> +    block::mq::gen_disk::{GenDisk, GenDiskBuilder},
> +    c_str,
> +    configfs::{self, AttributeOperations},
> +    configfs_attrs, new_mutex,
> +    page::PAGE_SIZE,
> +    prelude::*,
> +    str::CString,
> +    sync::Mutex,
> +};
> +use pin_init::PinInit;
> +
> +pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
> +    let item_type = configfs_attrs! {
> +        container: configfs::Subsystem<Config>,
> +        data: Config,
> +        child: DeviceConfig,
> +        attributes: [
> +            features: 0,
> +        ],
> +    };
> +
> +    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
> +}
> +
> +#[pin_data]
> +pub(crate) struct Config {}
> +
> +#[vtable]
> +impl AttributeOperations<0> for Config {
> +    type Data = Config;
> +
> +    fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_str("blocksize,size,rotational\n")?;
> +        Ok(writer.bytes_written())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Config {
> +    type Child = DeviceConfig;
> +
> +    fn make_group(
> +        &self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<DeviceConfig>, Error>> {
> +        let item_type = configfs_attrs! {
> +            container: configfs::Group<DeviceConfig>,
> +            data: DeviceConfig,
> +            attributes: [
> +                // Named for compatibility with C null_blk
> +                power: 0,
> +                blocksize: 1,
> +                rotational: 2,
> +                size: 3,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(
> +            name.try_into()?,
> +            item_type,
> +            // TODO: cannot coerce new_mutex!() to impl PinInit<_, Error>, so put mutex inside
> +            try_pin_init!( DeviceConfig {
> +                data <- new_mutex!( DeviceConfigInner {
> +                    powered: false,
> +                    block_size: 4096,
> +                    rotational: false,
> +                    disk: None,
> +                    capacity_mib: 4096,
> +                    name: name.try_into()?,
> +                }),
> +            }),
> +        ))
> +    }
> +}
> +
> +#[pin_data]
> +pub(crate) struct DeviceConfig {
> +    #[pin]
> +    data: Mutex<DeviceConfigInner>,
> +}
> +
> +#[pin_data]
> +struct DeviceConfigInner {
> +    powered: bool,
> +    name: CString,
> +    block_size: u32,
> +    rotational: bool,
> +    capacity_mib: u64,
> +    disk: Option<GenDisk<NullBlkDevice>>,
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +
> +        if this.data.lock().powered {
> +            writer.write_fmt(fmt!("1\n"))?;
> +        } else {
> +            writer.write_fmt(fmt!("0\n"))?;

I think these can just be
writer.write_str("1\n")?;

> +        }
> +
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        let power_op: bool = core::str::from_utf8(page)?
> +            .trim()
> +            .parse::<u8>()
> +            .map_err(|_| kernel::error::code::EINVAL)?
> +            != 0;

So if I write 27, that's treated as true, but if I write 300, that's an
EINVAL?

> +        let mut guard = this.data.lock();
> +
> +        if !guard.powered && power_op {
> +            guard.disk = Some(NullBlkDevice::new(
> +                &guard.name,
> +                guard.block_size,
> +                guard.rotational,
> +                guard.capacity_mib,
> +            )?);
> +            guard.powered = true;
> +        } else if guard.powered && !power_op {
> +            drop(guard.disk.take());
> +            guard.powered = false;
> +        }
> +
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<1> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_fmt(fmt!("{}\n", this.data.lock().block_size))?;
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        let text = core::str::from_utf8(page)?.trim();
> +        let value = text
> +            .parse::<u32>()
> +            .map_err(|_| kernel::error::code::EINVAL)?;
> +
> +        GenDiskBuilder::validate_block_size(value)?;
> +        this.data.lock().block_size = value;
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<2> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +
> +        if this.data.lock().rotational {
> +            writer.write_fmt(fmt!("1\n"))?;
> +        } else {
> +            writer.write_fmt(fmt!("0\n"))?;
> +        }
> +
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        this.data.lock().rotational = core::str::from_utf8(page)?
> +            .trim()
> +            .parse::<u8>()
> +            .map_err(|_| kernel::error::code::EINVAL)?
> +            != 0;
> +
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<3> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_fmt(fmt!("{}\n", this.data.lock().capacity_mib))?;
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        let text = core::str::from_utf8(page)?.trim();
> +        let value = text
> +            .parse::<u64>()
> +            .map_err(|_| kernel::error::code::EINVAL)?;
> +
> +        this.data.lock().capacity_mib = value;
> +        Ok(())
> +    }
> +}
> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
> index d07e76ae2c13..d09bc77861e4 100644
> --- a/drivers/block/rnull/rnull.rs
> +++ b/drivers/block/rnull/rnull.rs
> @@ -1,28 +1,26 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  //! This is a Rust implementation of the C null block driver.
> -//!
> -//! Supported features:
> -//!
> -//! - blk-mq interface
> -//! - direct completion
> -//! - block size 4k
> -//!
> -//! The driver is not configurable.
> +
> +mod configfs;
>  
>  use kernel::{
>      alloc::flags,
> -    block::mq::{
> +    block::{
>          self,
> -        gen_disk::{self, GenDisk},
> -        Operations, TagSet,
> +        mq::{
> +            self,
> +            gen_disk::{self, GenDisk},
> +            Operations, TagSet,
> +        },
>      },
>      error::Result,
> -    new_mutex, pr_info,
> +    pr_info,
>      prelude::*,
> -    sync::{Arc, Mutex},
> +    sync::Arc,
>      types::ARef,
>  };
> +use pin_init::PinInit;
>  
>  module! {
>      type: NullBlkModule,
> @@ -35,33 +33,39 @@
>  #[pin_data]
>  struct NullBlkModule {
>      #[pin]
> -    _disk: Mutex<GenDisk<NullBlkDevice>>,
> +    configfs_subsystem: kernel::configfs::Subsystem<configfs::Config>,
>  }
>  
>  impl kernel::InPlaceModule for NullBlkModule {
>      fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>          pr_info!("Rust null_blk loaded\n");
>  
> -        // Use a immediately-called closure as a stable `try` block
> -        let disk = /* try */ (|| {
> -            let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
> -
> -            gen_disk::GenDiskBuilder::new()
> -                .capacity_sectors(4096 << 11)
> -                .logical_block_size(4096)?
> -                .physical_block_size(4096)?
> -                .rotational(false)
> -                .build(format_args!("rnullb{}", 0), tagset)
> -        })();
> -
>          try_pin_init!(Self {
> -            _disk <- new_mutex!(disk?, "nullb:disk"),
> +            configfs_subsystem <- configfs::subsystem(),
>          })
>      }
>  }
>  
>  struct NullBlkDevice;
>  
> +impl NullBlkDevice {
> +    fn new(
> +        name: &CStr,
> +        block_size: u32,
> +        rotational: bool,
> +        capacity_mib: u64,
> +    ) -> Result<GenDisk<Self>> {
> +        let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
> +
> +        gen_disk::GenDiskBuilder::new()
> +            .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
> +            .logical_block_size(block_size)?
> +            .physical_block_size(block_size)?
> +            .rotational(rotational)
> +            .build(fmt!("{}", name.to_str()?), tagset)
> +    }
> +}
> +
>  #[vtable]
>  impl Operations for NullBlkDevice {
>      #[inline(always)]
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 39be2a31337f..7ab049ec591b 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -50,7 +50,7 @@ pub fn rotational(mut self, rotational: bool) -> Self {
>  
>      /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`,
>      /// and that it is a power of two.
> -    fn validate_block_size(size: u32) -> Result {
> +    pub fn validate_block_size(size: u32) -> Result {
>          if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() {
>              Err(error::code::EINVAL)
>          } else {
> 
> -- 
> 2.47.2
> 
> 

Re: [PATCH v3 13/16] rust: block: add `GenDisk` private data support

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:14PM +0200, Andreas Hindborg wrote:
> Allow users of the rust block device driver API to install private data in
> the `GenDisk` structure.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  drivers/block/rnull/rnull.rs       |  8 ++++---
>  rust/kernel/block/mq.rs            |  7 +++---
>  rust/kernel/block/mq/gen_disk.rs   | 32 ++++++++++++++++++++++----
>  rust/kernel/block/mq/operations.rs | 46 ++++++++++++++++++++++++++++++--------
>  4 files changed, 74 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
> index d09bc77861e4..a012c59ecb3c 100644
> --- a/drivers/block/rnull/rnull.rs
> +++ b/drivers/block/rnull/rnull.rs
> @@ -62,14 +62,16 @@ fn new(
>              .logical_block_size(block_size)?
>              .physical_block_size(block_size)?
>              .rotational(rotational)
> -            .build(fmt!("{}", name.to_str()?), tagset)
> +            .build(fmt!("{}", name.to_str()?), tagset, ())
>      }
>  }
>  
>  #[vtable]
>  impl Operations for NullBlkDevice {
> +    type QueueData = ();
> +
>      #[inline(always)]
> -    fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
> +    fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>          mq::Request::end_ok(rq)
>              .map_err(|_e| kernel::error::code::EIO)
>              // We take no refcounts on the request, so we expect to be able to
> @@ -80,5 +82,5 @@ fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
>          Ok(())
>      }
>  
> -    fn commit_rqs() {}
> +    fn commit_rqs(_queue_data: ()) {}
>  }
> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
> index faa3ccb5a49a..34b7425fa94d 100644
> --- a/rust/kernel/block/mq.rs
> +++ b/rust/kernel/block/mq.rs
> @@ -69,20 +69,21 @@
>  //!
>  //! #[vtable]
>  //! impl Operations for MyBlkDevice {
> +//!     type QueueData = ();
>  //!
> -//!     fn queue_rq(rq: ARef<Request<Self>>, _is_last: bool) -> Result {
> +//!     fn queue_rq(_queue_data: (), rq: ARef<Request<Self>>, _is_last: bool) -> Result {
>  //!         Request::end_ok(rq);
>  //!         Ok(())
>  //!     }
>  //!
> -//!     fn commit_rqs() {}
> +//!     fn commit_rqs(_queue_data: ()) {}
>  //! }
>  //!
>  //! let tagset: Arc<TagSet<MyBlkDevice>> =
>  //!     Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
>  //! let mut disk = gen_disk::GenDiskBuilder::new()
>  //!     .capacity_sectors(4096)
> -//!     .build(format_args!("myblk"), tagset)?;
> +//!     .build(format_args!("myblk"), tagset, ())?;
>  //!
>  //! # Ok::<(), kernel::error::Error>(())
>  //! ```
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 7ab049ec591b..954bc64cfa59 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -12,6 +12,7 @@
>      static_lock_class,
>      str::NullTerminatedFormatter,
>      sync::Arc,
> +    types::{ForeignOwnable, ScopeGuard},
>  };
>  use core::fmt::{self, Write};
>  
> @@ -97,7 +98,14 @@ pub fn build<T: Operations>(
>          self,
>          name: fmt::Arguments<'_>,
>          tagset: Arc<TagSet<T>>,
> +        queue_data: T::QueueData,
>      ) -> Result<GenDisk<T>> {
> +        let data = queue_data.into_foreign();
> +        let recover_data = ScopeGuard::new(|| {
> +            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
> +            unsafe { T::QueueData::from_foreign(data) };

I think this is clearer to read as

drop(unsafe { T::QueueData::from_foreign(data) });

> +        });
> +
>          // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>          let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>  
> @@ -112,7 +120,7 @@ pub fn build<T: Operations>(
>              bindings::__blk_mq_alloc_disk(
>                  tagset.raw_tag_set(),
>                  &mut lim,
> -                core::ptr::null_mut(),
> +                data.cast(),
>                  static_lock_class!().as_ptr(),
>              )
>          })?;
> @@ -166,8 +174,12 @@ pub fn build<T: Operations>(
>              },
>          )?;
>  
> +        recover_data.dismiss();
> +
>          // INVARIANT: `gendisk` was initialized above.
>          // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
> +        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
> +        // `__blk_mq_alloc_disk` above.
>          Ok(GenDisk {
>              _tagset: tagset,
>              gendisk,
> @@ -179,9 +191,10 @@ pub fn build<T: Operations>(
>  ///
>  /// # Invariants
>  ///
> -/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
> -/// - `gendisk` was added to the VFS through a call to
> -///   `bindings::device_add_disk`.
> +///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
> +///  - `gendisk` was added to the VFS through a call to
> +///    `bindings::device_add_disk`.
> +///  - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
>  pub struct GenDisk<T: Operations> {
>      _tagset: Arc<TagSet<T>>,
>      gendisk: *mut bindings::gendisk,
> @@ -193,9 +206,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
>  
>  impl<T: Operations> Drop for GenDisk<T> {
>      fn drop(&mut self) {
> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
> +        // and initialized instance of `struct gendisk`, and, `queuedata` was
> +        // initialized with the result of a call to
> +        // `ForeignOwnable::into_foreign`.
> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
> +
>          // SAFETY: By type invariant, `self.gendisk` points to a valid and
>          // initialized instance of `struct gendisk`, and it was previously added
>          // to the VFS.
>          unsafe { bindings::del_gendisk(self.gendisk) };
> +
> +        // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
> +        // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
> +        // `ForeignOwnable::from_foreign` is only called here.
> +        let _queue_data = unsafe { T::QueueData::from_foreign(queue_data.cast()) };

Ditto here.

drop(unsafe { T::QueueData::from_foreign(queue_data.cast()) });

Also, is this cast necessary as of
https://lore.kernel.org/all/20250711-rnull-up-v6-16-v3-13-3a262b4e2921@kernel.org/
?

>      }
>  }
> diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
> index 864ff379dc91..c50959d5517b 100644
> --- a/rust/kernel/block/mq/operations.rs
> +++ b/rust/kernel/block/mq/operations.rs
> @@ -6,14 +6,15 @@
>  
>  use crate::{
>      bindings,
> -    block::mq::request::RequestDataWrapper,
> -    block::mq::Request,
> +    block::mq::{request::RequestDataWrapper, Request},
>      error::{from_result, Result},
>      prelude::*,
> -    types::ARef,
> +    types::{ARef, ForeignOwnable},
>  };
>  use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering};
>  
> +type ForeignBorrowed<'a, T> = <T as ForeignOwnable>::Borrowed<'a>;
> +
>  /// Implement this trait to interface blk-mq as block devices.
>  ///
>  /// To implement a block device driver, implement this trait as described in the
> @@ -26,12 +27,20 @@
>  /// [module level documentation]: kernel::block::mq
>  #[macros::vtable]
>  pub trait Operations: Sized {
> +    /// Data associated with the `struct request_queue` that is allocated for
> +    /// the `GenDisk` associated with this `Operations` implementation.
> +    type QueueData: ForeignOwnable;
> +
>      /// Called by the kernel to queue a request with the driver. If `is_last` is
>      /// `false`, the driver is allowed to defer committing the request.
> -    fn queue_rq(rq: ARef<Request<Self>>, is_last: bool) -> Result;
> +    fn queue_rq(
> +        queue_data: ForeignBorrowed<'_, Self::QueueData>,
> +        rq: ARef<Request<Self>>,
> +        is_last: bool,
> +    ) -> Result;
>  
>      /// Called by the kernel to indicate that queued requests should be submitted.
> -    fn commit_rqs();
> +    fn commit_rqs(queue_data: ForeignBorrowed<'_, Self::QueueData>);
>  
>      /// Called by the kernel to poll the device for completed requests. Only
>      /// used for poll queues.
> @@ -70,7 +79,7 @@ impl<T: Operations> OperationsVTable<T> {
>      ///   promise to not access the request until the driver calls
>      ///   `bindings::blk_mq_end_request` for the request.
>      unsafe extern "C" fn queue_rq_callback(
> -        _hctx: *mut bindings::blk_mq_hw_ctx,
> +        hctx: *mut bindings::blk_mq_hw_ctx,
>          bd: *const bindings::blk_mq_queue_data,
>      ) -> bindings::blk_status_t {
>          // SAFETY: `bd.rq` is valid as required by the safety requirement for
> @@ -88,10 +97,20 @@ impl<T: Operations> OperationsVTable<T> {
>          //    reference counted by `ARef` until then.
>          let rq = unsafe { Request::aref_from_raw((*bd).rq) };
>  
> +        // SAFETY: `hctx` is valid as required by this function.
> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
> +
> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
> +        // dropped, which happens after we are dropped.
> +        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
> +
>          // SAFETY: We have exclusive access and we just set the refcount above.
>          unsafe { Request::start_unchecked(&rq) };
>  
>          let ret = T::queue_rq(
> +            queue_data,
>              rq,
>              // SAFETY: `bd` is valid as required by the safety requirement for
>              // this function.
> @@ -110,9 +129,18 @@ impl<T: Operations> OperationsVTable<T> {
>      ///
>      /// # Safety
>      ///
> -    /// This function may only be called by blk-mq C infrastructure.
> -    unsafe extern "C" fn commit_rqs_callback(_hctx: *mut bindings::blk_mq_hw_ctx) {
> -        T::commit_rqs()
> +    /// This function may only be called by blk-mq C infrastructure. The caller
> +    /// must ensure that `hctx` is valid.
> +    unsafe extern "C" fn commit_rqs_callback(hctx: *mut bindings::blk_mq_hw_ctx) {
> +        // SAFETY: `hctx` is valid as required by this function.
> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
> +
> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
> +        // dropped, which happens after we are dropped.
> +        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
> +        T::commit_rqs(queue_data)
>      }
>  
>      /// This function is called by the C kernel. It is not currently
> 
> -- 
> 2.47.2
> 
> 

Re: [PATCH v3 14/16] rust: block: mq: fix spelling in a safety comment

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:15PM +0200, Andreas Hindborg wrote:
> Add code block quotes to a safety comment.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 15/16] rust: block: add remote completion to `Request`

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:16PM +0200, Andreas Hindborg wrote:
> Allow users of rust block device driver API to schedule completion of
> requests via `blk_mq_complete_request_remote`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3 16/16] rnull: add soft-irq completion support

[Alice Ryhl]

On Fri, Jul 11, 2025 at 01:43:17PM +0200, Andreas Hindborg wrote:
> rnull currently only supports direct completion. Add option for completing
> requests across CPU nodes via soft IRQ or IPI.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

>  drivers/block/rnull/configfs.rs | 61 +++++++++++++++++++++++++++++++++++++++--
>  drivers/block/rnull/rnull.rs    | 32 +++++++++++++--------
>  2 files changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
> index 6c0e3bbb36ec..3ae84dfc8d62 100644
> --- a/drivers/block/rnull/configfs.rs
> +++ b/drivers/block/rnull/configfs.rs
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  use super::{NullBlkDevice, THIS_MODULE};
> -use core::fmt::Write;
> +use core::fmt::{Display, Write};
>  use kernel::{
>      block::mq::gen_disk::{GenDisk, GenDiskBuilder},
>      c_str,
> @@ -36,7 +36,7 @@ impl AttributeOperations<0> for Config {
>  
>      fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>          let mut writer = kernel::str::Formatter::new(page);
> -        writer.write_str("blocksize,size,rotational\n")?;
> +        writer.write_str("blocksize,size,rotational,irqmode\n")?;
>          Ok(writer.bytes_written())
>      }
>  }
> @@ -58,6 +58,7 @@ fn make_group(
>                  blocksize: 1,
>                  rotational: 2,
>                  size: 3,
> +                irqmode: 4,
>              ],
>          };
>  
> @@ -72,6 +73,7 @@ fn make_group(
>                      rotational: false,
>                      disk: None,
>                      capacity_mib: 4096,
> +                    irq_mode: IRQMode::None,
>                      name: name.try_into()?,
>                  }),
>              }),
> @@ -79,6 +81,34 @@ fn make_group(
>      }
>  }
>  
> +#[derive(Debug, Clone, Copy)]
> +pub(crate) enum IRQMode {
> +    None,
> +    Soft,
> +}
> +
> +impl TryFrom<u8> for IRQMode {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(value: u8) -> Result<Self> {
> +        match value {
> +            0 => Ok(Self::None),
> +            1 => Ok(Self::Soft),
> +            _ => Err(kernel::error::code::EINVAL),
> +        }
> +    }
> +}
> +
> +impl Display for IRQMode {
> +    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +        match self {
> +            Self::None => f.write_str("0")?,
> +            Self::Soft => f.write_str("1")?,
> +        }
> +        Ok(())
> +    }
> +}
> +
>  #[pin_data]
>  pub(crate) struct DeviceConfig {
>      #[pin]
> @@ -92,6 +122,7 @@ struct DeviceConfigInner {
>      block_size: u32,
>      rotational: bool,
>      capacity_mib: u64,
> +    irq_mode: IRQMode,
>      disk: Option<GenDisk<NullBlkDevice>>,
>  }
>  
> @@ -126,6 +157,7 @@ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>                  guard.block_size,
>                  guard.rotational,
>                  guard.capacity_mib,
> +                guard.irq_mode,
>              )?);
>              guard.powered = true;
>          } else if guard.powered && !power_op {
> @@ -218,3 +250,28 @@ fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>          Ok(())
>      }
>  }
> +
> +#[vtable]
> +impl configfs::AttributeOperations<4> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_fmt(fmt!("{}\n", this.data.lock().irq_mode))?;
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        let text = core::str::from_utf8(page)?.trim();
> +        let value = text
> +            .parse::<u8>()
> +            .map_err(|_| kernel::error::code::EINVAL)?;

EINVAL is in the prelude.

> +
> +        this.data.lock().irq_mode = IRQMode::try_from(value)?;
> +        Ok(())
> +    }
> +}
> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
> index 371786be7f47..85b1509a3106 100644
> --- a/drivers/block/rnull/rnull.rs
> +++ b/drivers/block/rnull/rnull.rs
> @@ -4,6 +4,7 @@
>  
>  mod configfs;
>  
> +use configfs::IRQMode;
>  use kernel::{
>      alloc::flags,
>      block::{
> @@ -54,35 +55,44 @@ fn new(
>          block_size: u32,
>          rotational: bool,
>          capacity_mib: u64,
> +        irq_mode: IRQMode,
>      ) -> Result<GenDisk<Self>> {
>          let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
>  
> +        let queue_data = Box::new(QueueData { irq_mode }, flags::GFP_KERNEL)?;
> +
>          gen_disk::GenDiskBuilder::new()
>              .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
>              .logical_block_size(block_size)?
>              .physical_block_size(block_size)?
>              .rotational(rotational)
> -            .build(fmt!("{}", name.to_str()?), tagset, ())
> +            .build(fmt!("{}", name.to_str()?), tagset, queue_data)
>      }
>  }
>  
> +struct QueueData {
> +    irq_mode: IRQMode,
> +}
> +
>  #[vtable]
>  impl Operations for NullBlkDevice {
> -    type QueueData = ();
> +    type QueueData = KBox<QueueData>;
>  
>      #[inline(always)]
> -    fn queue_rq(_queue_data: (), rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
> -        mq::Request::end_ok(rq)
> -            .map_err(|_e| kernel::error::code::EIO)
> -            // We take no refcounts on the request, so we expect to be able to
> -            // end the request. The request reference must be unique at this
> -            // point, and so `end_ok` cannot fail.
> -            .expect("Fatal error - expected to be able to end request");
> -
> +    fn queue_rq(queue_data: &QueueData, rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
> +        match queue_data.irq_mode {
> +            IRQMode::None => mq::Request::end_ok(rq)
> +                .map_err(|_e| kernel::error::code::EIO)
> +                // We take no refcounts on the request, so we expect to be able to
> +                // end the request. The request reference must be unique at this
> +                // point, and so `end_ok` cannot fail.
> +                .expect("Fatal error - expected to be able to end request"),
> +            IRQMode::Soft => mq::Request::complete(rq),
> +        }
>          Ok(())
>      }
>  
> -    fn commit_rqs(_queue_data: ()) {}
> +    fn commit_rqs(_queue_data: &QueueData) {}
>  
>      fn complete(rq: ARef<mq::Request<Self>>) {
>          mq::Request::end_ok(rq)
> 
> -- 
> 2.47.2
> 
> 

Re: [PATCH v3 04/16] rust: str: make `RawFormatter::bytes_written` public.

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:05PM +0200, Andreas Hindborg wrote:
>> rnull is going to make use of `kernel::str::RawFormatter::bytes_written`,
>> so make the visibility public.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Making methods public can be part of making the type itself public. I
> don't think a separate patch is needed.

I'll merge them.


Best regards,
Andreas Hindborg

Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:06PM +0200, Andreas Hindborg wrote:
>> Add `NullTerminatedFormatter`, a formatter that writes a null terminated
>> string to an array or slice buffer. Because this type needs to manage the
>> trailing null marker, the existing formatters cannot be used to implement
>> this type.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index b1bc584803b0..c58925438c6e 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>>      }
>>  }
>>
>> +/// A mutable reference to a byte buffer where a string can be written into.
>> +///
>> +/// The buffer will be automatically null terminated after the last written character.
>> +///
>> +/// # Invariants
>> +///
>> +/// `buffer` is always null terminated.
>
> Since you modify the buffer range, the actual invariant is that the
> first byte of `buffer` is zero.

It is still null terminated, although your suggestion is more precise.

>
>> +pub(crate) struct NullTerminatedFormatter<'a> {
>
> Isn't it called "nul" rather than "null"? My understanding is that
> "null" is for the pointer case, and "nul" is the name of the ascii
> character at codepoint zero.

I don't know. I did a quick internet search but got no definitive
answer. Wikipedia says "Null character" [1].

[1] https://en.wikipedia.org/wiki/Null_character

>
>> +    buffer: &'a mut [u8],
>> +}
>> +
>> +impl<'a> NullTerminatedFormatter<'a> {
>> +    /// Create a new [`Self`] instance.
>> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>> +        *(buffer.first_mut()?) = 0;
>> +
>> +        // INVARIANT: We null terminated the buffer above.
>> +        Some(Self { buffer })
>> +    }
>> +
>> +    #[expect(dead_code)]
>> +    pub(crate) fn from_array<const N: usize>(
>> +        buffer: &'a mut [crate::ffi::c_char; N],
>> +    ) -> Option<NullTerminatedFormatter<'a>> {
>
> Can't you just call `::new` where you use this method?

Yes, this can be elided, thanks.

>
>> +        Self::new(buffer)
>> +    }
>> +}
>> +
>> +impl Write for NullTerminatedFormatter<'_> {
>> +    fn write_str(&mut self, s: &str) -> fmt::Result {
>> +        let bytes = s.as_bytes();
>> +        let len = bytes.len();
>> +
>> +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.
>
> overflow -> underflow

Coming from a computer architecture background, these are the same to
me. Also, core has `u16::overflowing_sub` [2].

[2] https://doc.rust-lang.org/stable/core/primitive.u16.html#method.overflowing_sub

>
>> +        if len > self.buffer.len() - 1 {
>
> this is just `len >= self.buffer.len()`.

It is, but is it better?


Best regards,
Andreas Hindborg

Re: [PATCH v3 10/16] rust: block: add block related constants

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:11PM +0200, Andreas Hindborg wrote:
>> Add a few block subsystem constants to the rust `kernel::block` name space.
>> This makes it easier to access the constants from rust code.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/block.rs | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
>> index 150f710efe5b..7461adf4d7e0 100644
>> --- a/rust/kernel/block.rs
>> +++ b/rust/kernel/block.rs
>> @@ -3,3 +3,15 @@
>>  //! Types for working with the block layer.
>>
>>  pub mod mq;
>> +
>> +/// Bit mask for masking out [`SECTOR_SIZE`]
>> +pub const SECTOR_MASK: u32 = bindings::SECTOR_MASK;
>> +
>> +/// Sectors are size `1 << SECTOR_SHIFT`.
>> +pub const SECTOR_SHIFT: u32 = bindings::SECTOR_SHIFT;
>> +
>> +/// Size of a sector.
>> +pub const SECTOR_SIZE: u32 = bindings::SECTOR_SIZE;
>> +
>> +/// Power of two difference in size of a page and size of a sector.
>> +pub const PAGE_SECTORS_SHIFT: u32 = bindings::PAGE_SECTORS_SHIFT;
>
> I was looking for the user to double-check whether u32 was the right
> choice, but I can't find it. It looks like you don't use these yet?

Only `SECTOR_SHIFT` is used in this series, the rest will be used in the
future. They are used for memory backing rnull, still out of tree [1].
Do you want me to split the patch and delay the rest of the constants? I
fully intend to upstream the user as soon as I can.

It is a #define on C side used as a shift operator on usize in Rust code
like so:

  let sector: usize = something;
  let page_offset = (sector & block::SECTOR_MASK as usize) << block::SECTOR_SHIFT;

Best regards,
Andreas Hindborg


[1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/drivers/block/rnull/rnull.rs?h=rnull-v6.15#n211

Re: [PATCH v3 01/16] rust: str: normalize imports in `str.rs`

[Daniel Almeida]

Hi Andreas,

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Clean up imports in `str.rs`. This makes future code manipulation more
> manageable.

Here’s another instance of why I think this import syntax is not good.

> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index a927db8e079c..488b0e97004e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -3,10 +3,11 @@
> //! String representations.
> 
> use crate::alloc::{flags::*, AllocError, KVec};
> -use core::fmt::{self, Write};
> -use core::ops::{self, Deref, DerefMut, Index};
> -
> use crate::prelude::*;
> +use core::{
> +    fmt::{self, Write},
> +    ops::{self, Deref, DerefMut, Index},
> +};
> 
> /// Byte string without UTF-8 validity guarantee.
> #[repr(transparent)]
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 02/16] rust: str: allow `str::Formatter` to format into `&mut [u8]`.

[Daniel Almeida]

Hi Andreas,

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Improve `Formatter` so that it can write to an array or slice buffer.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 488b0e97004e..41af456a46c8 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -6,6 +6,7 @@
> use crate::prelude::*;
> use core::{
>     fmt::{self, Write},
> +    marker::PhantomData,
>     ops::{self, Deref, DerefMut, Index},
> };
> 
> @@ -794,9 +795,9 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
> /// Allows formatting of [`fmt::Arguments`] into a raw buffer.
> ///
> /// Fails if callers attempt to write more than will fit in the buffer.
> -pub(crate) struct Formatter(RawFormatter);
> +pub(crate) struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
> 
> -impl Formatter {
> +impl Formatter<'_> {
>     /// Creates a new instance of [`Formatter`] with the given buffer.
>     ///
>     /// # Safety
> @@ -805,11 +806,19 @@ impl Formatter {
>     /// for the lifetime of the returned [`Formatter`].
>     pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
>         // SAFETY: The safety requirements of this function satisfy those of the callee.
> -        Self(unsafe { RawFormatter::from_buffer(buf, len) })
> +        Self(unsafe { RawFormatter::from_buffer(buf, len) }, PhantomData)
> +    }
> +
> +    /// Create a new [`Self`] instance.
> +    #[expect(dead_code)]
> +    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {

nit: the function above this one returns Self, and this one returns Formatter.
Perhaps this one should also return Self for consistency?
 
> +        // SAFETY: `buffer` is valid for writes for the entire length for
> +        // the lifetime of `Self`.
> +        unsafe { Formatter::from_buffer(buffer.as_mut_ptr(), buffer.len()) }
>     }
> }
> 
> -impl Deref for Formatter {
> +impl Deref for Formatter<'_> {
>     type Target = RawFormatter;
> 
>     fn deref(&self) -> &Self::Target {
> @@ -817,7 +826,7 @@ fn deref(&self) -> &Self::Target {
>     }
> }
> 
> -impl fmt::Write for Formatter {
> +impl fmt::Write for Formatter<'_> {
>     fn write_str(&mut self, s: &str) -> fmt::Result {
>         self.0.write_str(s)?;
> 
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 03/16] rust: str: expose `str::Formatter::new` publicly.

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> rnull is going to make use of `str::Formatter`, so expose it with public
> visibility.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 41af456a46c8..28a6179385fc 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -703,7 +703,7 @@ fn test_bstr_debug() -> Result {
> ///
> /// The memory region between `pos` (inclusive) and `end` (exclusive) is valid for writes if `pos`
> /// is less than `end`.
> -pub(crate) struct RawFormatter {
> +pub struct RawFormatter {
>     // Use `usize` to use `saturating_*` functions.
>     beg: usize,
>     pos: usize,
> @@ -795,7 +795,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
> /// Allows formatting of [`fmt::Arguments`] into a raw buffer.
> ///
> /// Fails if callers attempt to write more than will fit in the buffer.
> -pub(crate) struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
> +pub struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
> 
> impl Formatter<'_> {
>     /// Creates a new instance of [`Formatter`] with the given buffer.
> @@ -810,8 +810,7 @@ pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
>     }
> 
>     /// Create a new [`Self`] instance.
> -    #[expect(dead_code)]
> -    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
> +    pub fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
>         // SAFETY: `buffer` is valid for writes for the entire length for
>         // the lifetime of `Self`.
>         unsafe { Formatter::from_buffer(buffer.as_mut_ptr(), buffer.len()) }
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 04/16] rust: str: make `RawFormatter::bytes_written` public.

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> rnull is going to make use of `kernel::str::RawFormatter::bytes_written`,
> so make the visibility public.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 28a6179385fc..b1bc584803b0 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -761,7 +761,7 @@ pub(crate) fn pos(&self) -> *mut u8 {
>     }
> 
>     /// Returns the number of bytes written to the formatter.
> -    pub(crate) fn bytes_written(&self) -> usize {
> +    pub fn bytes_written(&self) -> usize {
>         self.pos - self.beg
>     }
> }
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Add `NullTerminatedFormatter`, a formatter that writes a null terminated
> string to an array or slice buffer. Because this type needs to manage the
> trailing null marker, the existing formatters cannot be used to implement
> this type.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
> 
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index b1bc584803b0..c58925438c6e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>     }
> }
> 
> +/// A mutable reference to a byte buffer where a string can be written into.
> +///
> +/// The buffer will be automatically null terminated after the last written character.

Hmm, I suppose you want this to be the only null? See below.

> +///
> +/// # Invariants
> +///
> +/// `buffer` is always null terminated.
> +pub(crate) struct NullTerminatedFormatter<'a> {
> +    buffer: &'a mut [u8],
> +}
> +
> +impl<'a> NullTerminatedFormatter<'a> {
> +    /// Create a new [`Self`] instance.
> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
> +        *(buffer.first_mut()?) = 0;
> +
> +        // INVARIANT: We null terminated the buffer above.
> +        Some(Self { buffer })
> +    }
> +
> +    #[expect(dead_code)]
> +    pub(crate) fn from_array<const N: usize>(
> +        buffer: &'a mut [crate::ffi::c_char; N],
> +    ) -> Option<NullTerminatedFormatter<'a>> {
> +        Self::new(buffer)
> +    }
> +}
> +
> +impl Write for NullTerminatedFormatter<'_> {
> +    fn write_str(&mut self, s: &str) -> fmt::Result {
> +        let bytes = s.as_bytes();
> +        let len = bytes.len();
> +
> +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.

Perhaps this should be a type invariant? I know this is a logical conclusion
from saying “buffer is always NULL terminated”, but it’s always
nice to be even more explicit.

> +        if len > self.buffer.len() - 1 {
> +            return Err(fmt::Error);
> +        }
> +
> +        let buffer = core::mem::take(&mut self.buffer);
> +        // We break the null termination invariant for a short while.
> +        buffer[..len].copy_from_slice(bytes);
> +        self.buffer = &mut buffer[len..];

As I said in my first comment, if you want this to be the only null, I
don’t think the copy above enforces it?

> +
> +        // INVARIANT: We null terminate the buffer.
> +        self.buffer[0] = 0;
> +
> +        Ok(())
> +    }
> +}
> +
> /// An owned string that is guaranteed to have exactly one `NUL` byte, which is at the end.
> ///
> /// Used for interoperability with kernel APIs that take C strings.
> 
> -- 
> 2.47.2
> 
> 
> 

— Daniel

Re: [PATCH v3 06/16] rust: block: normalize imports for `gen_disk.rs`

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Clean up the import statements in `gen_disk.rs` to make the code easier to
> maintain.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/block/mq/gen_disk.rs | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index cd54cd64ea88..679ee1bb2195 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -5,9 +5,13 @@
> //! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
> //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
> 
> -use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
> -use crate::{error, static_lock_class};
> +use crate::{
> +    bindings,
> +    block::mq::{raw_writer::RawWriter, Operations, TagSet},
> +    error::{self, from_err_ptr, Result},
> +    static_lock_class,
> +    sync::Arc,
> +};
> use core::fmt::{self, Write};
> 
> /// A builder for [`GenDisk`].
> 
> -- 
> 2.47.2
> 
> 
> 

Same comment as the preceding “import” patch: this is syntax is problematic.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 07/16] rust: block: use `NullTerminatedFormatter`

[Daniel Almeida]

Hi Andreas,

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Use the new `NullTerminatedFormatter` to write the name of a `GenDisk` to
> the name buffer. This new formatter automatically adds a trailing null
> marker after the written characters, so we don't need to append that at the
> call site any longer.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/block/mq/gen_disk.rs   | 11 ++++++-----
> rust/kernel/block/mq/raw_writer.rs |  1 +
> rust/kernel/str.rs                 |  1 -
> 3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 679ee1bb2195..39be2a31337f 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -7,9 +7,10 @@
> 
> use crate::{
>     bindings,
> -    block::mq::{raw_writer::RawWriter, Operations, TagSet},
> +    block::mq::{Operations, TagSet},
>     error::{self, from_err_ptr, Result},
>     static_lock_class,
> +    str::NullTerminatedFormatter,
>     sync::Arc,
> };
> use core::fmt::{self, Write};
> @@ -143,14 +144,14 @@ pub fn build<T: Operations>(
>         // SAFETY: `gendisk` is a valid pointer as we initialized it above
>         unsafe { (*gendisk).fops = &TABLE };
> 
> -        let mut raw_writer = RawWriter::from_array(
> +        let mut writer = NullTerminatedFormatter::from_array(
>             // SAFETY: `gendisk` points to a valid and initialized instance. We
>             // have exclusive access, since the disk is not added to the VFS
>             // yet.
>             unsafe { &mut (*gendisk).disk_name },
> -        )?;
> -        raw_writer.write_fmt(name)?;
> -        raw_writer.write_char('\0')?;
> +        )
> +        .ok_or(error::code::EINVAL)?;
> +        writer.write_fmt(name)?;
> 
>         // SAFETY: `gendisk` points to a valid and initialized instance of
>         // `struct gendisk`. `set_capacity` takes a lock to synchronize this
> diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
> index 7e2159e4f6a6..0aef55703e71 100644
> --- a/rust/kernel/block/mq/raw_writer.rs
> +++ b/rust/kernel/block/mq/raw_writer.rs
> @@ -24,6 +24,7 @@ fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
>         Ok(Self { buffer, pos: 0 })
>     }
> 
> +    #[expect(dead_code)]

Not sure I understand, is this superseded by..

>     pub(crate) fn from_array<const N: usize>(
>         a: &'a mut [crate::ffi::c_char; N],
>     ) -> Result<RawWriter<'a>> {
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index c58925438c6e..7396c49174cd 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -858,7 +858,6 @@ pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>         Some(Self { buffer })
>     }
> 
> -    #[expect(dead_code)]

… this?

>     pub(crate) fn from_array<const N: usize>(
>         buffer: &'a mut [crate::ffi::c_char; N],
>     ) -> Option<NullTerminatedFormatter<'a>> {
> 
> -- 
> 2.47.2
> 
> 
> 

— Daniel

Re: [PATCH v3 07/16] rust: block: use `NullTerminatedFormatter`

[Daniel Almeida]

> On 6 Aug 2025, at 10:22, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Hi Andreas,
> 
>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> 
>> Use the new `NullTerminatedFormatter` to write the name of a `GenDisk` to
>> the name buffer. This new formatter automatically adds a trailing null
>> marker after the written characters, so we don't need to append that at the
>> call site any longer.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/block/mq/gen_disk.rs   | 11 ++++++-----
>> rust/kernel/block/mq/raw_writer.rs |  1 +
>> rust/kernel/str.rs                 |  1 -
>> 3 files changed, 7 insertions(+), 6 deletions(-)
>> 
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index 679ee1bb2195..39be2a31337f 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -7,9 +7,10 @@
>> 
>> use crate::{
>>    bindings,
>> -    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>> +    block::mq::{Operations, TagSet},
>>    error::{self, from_err_ptr, Result},
>>    static_lock_class,
>> +    str::NullTerminatedFormatter,
>>    sync::Arc,
>> };
>> use core::fmt::{self, Write};
>> @@ -143,14 +144,14 @@ pub fn build<T: Operations>(
>>        // SAFETY: `gendisk` is a valid pointer as we initialized it above
>>        unsafe { (*gendisk).fops = &TABLE };
>> 
>> -        let mut raw_writer = RawWriter::from_array(
>> +        let mut writer = NullTerminatedFormatter::from_array(
>>            // SAFETY: `gendisk` points to a valid and initialized instance. We
>>            // have exclusive access, since the disk is not added to the VFS
>>            // yet.
>>            unsafe { &mut (*gendisk).disk_name },
>> -        )?;
>> -        raw_writer.write_fmt(name)?;
>> -        raw_writer.write_char('\0')?;
>> +        )
>> +        .ok_or(error::code::EINVAL)?;
>> +        writer.write_fmt(name)?;
>> 
>>        // SAFETY: `gendisk` points to a valid and initialized instance of
>>        // `struct gendisk`. `set_capacity` takes a lock to synchronize this
>> diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
>> index 7e2159e4f6a6..0aef55703e71 100644
>> --- a/rust/kernel/block/mq/raw_writer.rs
>> +++ b/rust/kernel/block/mq/raw_writer.rs
>> @@ -24,6 +24,7 @@ fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
>>        Ok(Self { buffer, pos: 0 })
>>    }
>> 
>> +    #[expect(dead_code)]
> 
> Not sure I understand, is this superseded by..
> 
>>    pub(crate) fn from_array<const N: usize>(
>>        a: &'a mut [crate::ffi::c_char; N],
>>    ) -> Result<RawWriter<'a>> {
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index c58925438c6e..7396c49174cd 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -858,7 +858,6 @@ pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>>        Some(Self { buffer })
>>    }
>> 
>> -    #[expect(dead_code)]
> 
> … this?
> 
>>    pub(crate) fn from_array<const N: usize>(
>>        buffer: &'a mut [crate::ffi::c_char; N],
>>    ) -> Option<NullTerminatedFormatter<'a>> {
>> 
>> -- 
>> 2.47.2
>> 
>> 
>> 
> 
> — Daniel

Ah, I just saw the next patch.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 08/16] rust: block: remove `RawWriter`

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> `RawWriter` is now dead code, so remove it.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/block/mq.rs            |  1 -
> rust/kernel/block/mq/raw_writer.rs | 56 --------------------------------------
> 2 files changed, 57 deletions(-)
> 
> diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
> index fb0f393c1cea..faa3ccb5a49a 100644
> --- a/rust/kernel/block/mq.rs
> +++ b/rust/kernel/block/mq.rs
> @@ -89,7 +89,6 @@
> 
> pub mod gen_disk;
> mod operations;
> -mod raw_writer;
> mod request;
> mod tag_set;
> 
> diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
> deleted file mode 100644
> index 0aef55703e71..000000000000
> --- a/rust/kernel/block/mq/raw_writer.rs
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -
> -use core::fmt::{self, Write};
> -
> -use crate::error::Result;
> -use crate::prelude::EINVAL;
> -
> -/// A mutable reference to a byte buffer where a string can be written into.
> -///
> -/// # Invariants
> -///
> -/// `buffer` is always null terminated.
> -pub(crate) struct RawWriter<'a> {
> -    buffer: &'a mut [u8],
> -    pos: usize,
> -}
> -
> -impl<'a> RawWriter<'a> {
> -    /// Create a new `RawWriter` instance.
> -    fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
> -        *(buffer.last_mut().ok_or(EINVAL)?) = 0;
> -
> -        // INVARIANT: We null terminated the buffer above.
> -        Ok(Self { buffer, pos: 0 })
> -    }
> -
> -    #[expect(dead_code)]
> -    pub(crate) fn from_array<const N: usize>(
> -        a: &'a mut [crate::ffi::c_char; N],
> -    ) -> Result<RawWriter<'a>> {
> -        Self::new(
> -            // SAFETY: the buffer of `a` is valid for read and write as `u8` for
> -            // at least `N` bytes.
> -            unsafe { core::slice::from_raw_parts_mut(a.as_mut_ptr().cast::<u8>(), N) },
> -        )
> -    }
> -}
> -
> -impl Write for RawWriter<'_> {
> -    fn write_str(&mut self, s: &str) -> fmt::Result {
> -        let bytes = s.as_bytes();
> -        let len = bytes.len();
> -
> -        // We do not want to overwrite our null terminator
> -        if self.pos + len > self.buffer.len() - 1 {
> -            return Err(fmt::Error);
> -        }
> -
> -        // INVARIANT: We are not overwriting the last byte
> -        self.buffer[self.pos..self.pos + len].copy_from_slice(bytes);
> -
> -        self.pos += len;
> -
> -        Ok(())
> -    }
> -}
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 02/16] rust: str: allow `str::Formatter` to format into `&mut [u8]`.

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Improve `Formatter` so that it can write to an array or slice buffer.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/str.rs | 19 ++++++++++++++-----
>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index 488b0e97004e..41af456a46c8 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -6,6 +6,7 @@
>> use crate::prelude::*;
>> use core::{
>>     fmt::{self, Write},
>> +    marker::PhantomData,
>>     ops::{self, Deref, DerefMut, Index},
>> };
>>
>> @@ -794,9 +795,9 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>> /// Allows formatting of [`fmt::Arguments`] into a raw buffer.
>> ///
>> /// Fails if callers attempt to write more than will fit in the buffer.
>> -pub(crate) struct Formatter(RawFormatter);
>> +pub(crate) struct Formatter<'a>(RawFormatter, PhantomData<&'a mut ()>);
>>
>> -impl Formatter {
>> +impl Formatter<'_> {
>>     /// Creates a new instance of [`Formatter`] with the given buffer.
>>     ///
>>     /// # Safety
>> @@ -805,11 +806,19 @@ impl Formatter {
>>     /// for the lifetime of the returned [`Formatter`].
>>     pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
>>         // SAFETY: The safety requirements of this function satisfy those of the callee.
>> -        Self(unsafe { RawFormatter::from_buffer(buf, len) })
>> +        Self(unsafe { RawFormatter::from_buffer(buf, len) }, PhantomData)
>> +    }
>> +
>> +    /// Create a new [`Self`] instance.
>> +    #[expect(dead_code)]
>> +    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
>
> nit: the function above this one returns Self, and this one returns Formatter.
> Perhaps this one should also return Self for consistency?

Thanks. Not sure about the explicit lifetime either, I'll
reformat:

@@ -844,7 +844,7 @@ pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
 
     /// Create a new [`Self`] instance.
     #[expect(dead_code)]
-    pub(crate) fn new<'a>(buffer: &'a mut [u8]) -> Formatter<'a> {
+    pub(crate) fn new(buffer: &mut [u8]) -> Self {
         // SAFETY: `buffer` is valid for writes for the entire length for
         // the lifetime of `Self`.
         unsafe { Formatter::from_buffer(buffer.as_mut_ptr(), buffer.len()) }


Best regards,
Andreas Hindborg

Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add `NullTerminatedFormatter`, a formatter that writes a null terminated
>> string to an array or slice buffer. Because this type needs to manage the
>> trailing null marker, the existing formatters cannot be used to implement
>> this type.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index b1bc584803b0..c58925438c6e 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>>     }
>> }
>>
>> +/// A mutable reference to a byte buffer where a string can be written into.
>> +///
>> +/// The buffer will be automatically null terminated after the last written character.
>
> Hmm, I suppose you want this to be the only null? See below.

This code went through some iteration. In the original code, I kept a
pointer to the beginning of the buffer and an offset. It made sense to
require the buffer to be null terminated.

In this iteration, I let go of the pointer to the beginning of the
buffer and point to the next writable byte. If this byte is zero, the
original buffer is null terminated. Alice suggested rephrase, and I put
this for the next spin:


  /// # Invariants
  ///
  /// * The first byte of `buffer` is always zero.

At any rate, the final string is allowed to have multiple null
characters in it.

>
>> +///
>> +/// # Invariants
>> +///
>> +/// `buffer` is always null terminated.
>> +pub(crate) struct NullTerminatedFormatter<'a> {
>> +    buffer: &'a mut [u8],
>> +}
>> +
>> +impl<'a> NullTerminatedFormatter<'a> {
>> +    /// Create a new [`Self`] instance.
>> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>> +        *(buffer.first_mut()?) = 0;
>> +
>> +        // INVARIANT: We null terminated the buffer above.
>> +        Some(Self { buffer })
>> +    }
>> +
>> +    #[expect(dead_code)]
>> +    pub(crate) fn from_array<const N: usize>(
>> +        buffer: &'a mut [crate::ffi::c_char; N],
>> +    ) -> Option<NullTerminatedFormatter<'a>> {
>> +        Self::new(buffer)
>> +    }
>> +}
>> +
>> +impl Write for NullTerminatedFormatter<'_> {
>> +    fn write_str(&mut self, s: &str) -> fmt::Result {
>> +        let bytes = s.as_bytes();
>> +        let len = bytes.len();
>> +
>> +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.
>
> Perhaps this should be a type invariant? I know this is a logical conclusion
> from saying “buffer is always NULL terminated”, but it’s always
> nice to be even more explicit.

I can add a minimum size 1 byte requirement 👍

>
>> +        if len > self.buffer.len() - 1 {
>> +            return Err(fmt::Error);
>> +        }
>> +
>> +        let buffer = core::mem::take(&mut self.buffer);
>> +        // We break the null termination invariant for a short while.
>> +        buffer[..len].copy_from_slice(bytes);
>> +        self.buffer = &mut buffer[len..];
>
> As I said in my first comment, if you want this to be the only null, I
> don’t think the copy above enforces it?

It does not need to be the only one, as long as there is a null at the
end of the final string, we are good.


Best regards,
Andreas Hindborg

Re: [PATCH v3 06/16] rust: block: normalize imports for `gen_disk.rs`

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Clean up the import statements in `gen_disk.rs` to make the code easier to
>> maintain.
>>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/block/mq/gen_disk.rs | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index cd54cd64ea88..679ee1bb2195 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -5,9 +5,13 @@
>> //! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
>> //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>>
>> -use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
>> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
>> -use crate::{error, static_lock_class};
>> +use crate::{
>> +    bindings,
>> +    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>> +    error::{self, from_err_ptr, Result},
>> +    static_lock_class,
>> +    sync::Arc,
>> +};
>> use core::fmt::{self, Write};
>>
>> /// A builder for [`GenDisk`].
>>
>> --
>> 2.47.2
>>
>>
>>
>
> Same comment as the preceding “import” patch: this is syntax is problematic.

I used to share your viewpoint, but I changed my opinion and now prefer
"normalized" imports (the combined form).

Now I can just blindly merge all the imports, remove duplicates and then
ask rust-analyzer to normalize imports again, and then format with
rustfmt. I find that this workflow is very low overhead.


Best regards,
Andreas Hindborg

Re: [PATCH v3 07/16] rust: block: use `NullTerminatedFormatter`

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Use the new `NullTerminatedFormatter` to write the name of a `GenDisk` to
>> the name buffer. This new formatter automatically adds a trailing null
>> marker after the written characters, so we don't need to append that at the
>> call site any longer.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/block/mq/gen_disk.rs   | 11 ++++++-----
>> rust/kernel/block/mq/raw_writer.rs |  1 +
>> rust/kernel/str.rs                 |  1 -
>> 3 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>> index 679ee1bb2195..39be2a31337f 100644
>> --- a/rust/kernel/block/mq/gen_disk.rs
>> +++ b/rust/kernel/block/mq/gen_disk.rs
>> @@ -7,9 +7,10 @@
>>
>> use crate::{
>>     bindings,
>> -    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>> +    block::mq::{Operations, TagSet},
>>     error::{self, from_err_ptr, Result},
>>     static_lock_class,
>> +    str::NullTerminatedFormatter,
>>     sync::Arc,
>> };
>> use core::fmt::{self, Write};
>> @@ -143,14 +144,14 @@ pub fn build<T: Operations>(
>>         // SAFETY: `gendisk` is a valid pointer as we initialized it above
>>         unsafe { (*gendisk).fops = &TABLE };
>>
>> -        let mut raw_writer = RawWriter::from_array(
>> +        let mut writer = NullTerminatedFormatter::from_array(
>>             // SAFETY: `gendisk` points to a valid and initialized instance. We
>>             // have exclusive access, since the disk is not added to the VFS
>>             // yet.
>>             unsafe { &mut (*gendisk).disk_name },
>> -        )?;
>> -        raw_writer.write_fmt(name)?;
>> -        raw_writer.write_char('\0')?;
>> +        )
>> +        .ok_or(error::code::EINVAL)?;
>> +        writer.write_fmt(name)?;
>>
>>         // SAFETY: `gendisk` points to a valid and initialized instance of
>>         // `struct gendisk`. `set_capacity` takes a lock to synchronize this
>> diff --git a/rust/kernel/block/mq/raw_writer.rs b/rust/kernel/block/mq/raw_writer.rs
>> index 7e2159e4f6a6..0aef55703e71 100644
>> --- a/rust/kernel/block/mq/raw_writer.rs
>> +++ b/rust/kernel/block/mq/raw_writer.rs
>> @@ -24,6 +24,7 @@ fn new(buffer: &'a mut [u8]) -> Result<RawWriter<'a>> {
>>         Ok(Self { buffer, pos: 0 })
>>     }
>>
>> +    #[expect(dead_code)]
>
> Not sure I understand, is this superseded by..
>
>>     pub(crate) fn from_array<const N: usize>(
>>         a: &'a mut [crate::ffi::c_char; N],
>>     ) -> Result<RawWriter<'a>> {
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index c58925438c6e..7396c49174cd 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -858,7 +858,6 @@ pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>>         Some(Self { buffer })
>>     }
>>
>> -    #[expect(dead_code)]
>
> … this?
>
>>     pub(crate) fn from_array<const N: usize>(
>>         buffer: &'a mut [crate::ffi::c_char; N],
>>     ) -> Option<NullTerminatedFormatter<'a>> {
>>

Yes. Alice suggested combining patch 8/16 that removes
`block::mq::raw_writer` with this one.


Best regards,
Andreas Hindborg

Re: [PATCH v3 06/16] rust: block: normalize imports for `gen_disk.rs`

[Daniel Almeida]

> On 6 Aug 2025, at 11:51, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>> 
>>> Clean up the import statements in `gen_disk.rs` to make the code easier to
>>> maintain.
>>> 
>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>> rust/kernel/block/mq/gen_disk.rs | 10 +++++++---
>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>>> index cd54cd64ea88..679ee1bb2195 100644
>>> --- a/rust/kernel/block/mq/gen_disk.rs
>>> +++ b/rust/kernel/block/mq/gen_disk.rs
>>> @@ -5,9 +5,13 @@
>>> //! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
>>> //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>>> 
>>> -use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
>>> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
>>> -use crate::{error, static_lock_class};
>>> +use crate::{
>>> +    bindings,
>>> +    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>>> +    error::{self, from_err_ptr, Result},
>>> +    static_lock_class,
>>> +    sync::Arc,
>>> +};
>>> use core::fmt::{self, Write};
>>> 
>>> /// A builder for [`GenDisk`].
>>> 
>>> --
>>> 2.47.2
>>> 
>>> 
>>> 
>> 
>> Same comment as the preceding “import” patch: this is syntax is problematic.
> 
> I used to share your viewpoint, but I changed my opinion and now prefer
> "normalized" imports (the combined form).
> 
> Now I can just blindly merge all the imports, remove duplicates and then
> ask rust-analyzer to normalize imports again, and then format with
> rustfmt. I find that this workflow is very low overhead.
> 
> 
> Best regards,
> Andreas Hindborg

That’s because you have a separate commit where you do this before applying
your work on top. If you’re rebasing on top of someone else's work, then a
lot of conflicts will pop up. And unlike the saner approach where each import
is in its own line, it’s now absolutely not clear how the conflicts should
be resolved.

The only thing that can be done then is to accept whatever the current change
is, and ask rust-analyzer to reimport everything and reformat.

IMHO, this is not great.

— Daniel

Re: [PATCH v3 09/16] rust: block: remove trait bound from `mq::Request` definition

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Remove the trait bound `T:Operations` from `mq::Request`. The bound is not
> required, so remove it to reduce complexity.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/block/mq/request.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
> index 4a5b7ec914ef..2d14a6261a31 100644
> --- a/rust/kernel/block/mq/request.rs
> +++ b/rust/kernel/block/mq/request.rs
> @@ -53,7 +53,7 @@
> /// [`struct request`]: srctree/include/linux/blk-mq.h
> ///
> #[repr(transparent)]
> -pub struct Request<T: Operations>(Opaque<bindings::request>, PhantomData<T>);
> +pub struct Request<T>(Opaque<bindings::request>, PhantomData<T>);
> 
> impl<T: Operations> Request<T> {
>     /// Create an [`ARef<Request>`] from a [`struct request`] pointer.
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 10/16] rust: block: add block related constants

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Add a few block subsystem constants to the rust `kernel::block` name space.
> This makes it easier to access the constants from rust code.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/block.rs | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
> 
> diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
> index 150f710efe5b..7461adf4d7e0 100644
> --- a/rust/kernel/block.rs
> +++ b/rust/kernel/block.rs
> @@ -3,3 +3,15 @@
> //! Types for working with the block layer.
> 
> pub mod mq;
> +
> +/// Bit mask for masking out [`SECTOR_SIZE`]

Missing period.

> +pub const SECTOR_MASK: u32 = bindings::SECTOR_MASK;
> +
> +/// Sectors are size `1 << SECTOR_SHIFT`.
> +pub const SECTOR_SHIFT: u32 = bindings::SECTOR_SHIFT;
> +
> +/// Size of a sector.
> +pub const SECTOR_SIZE: u32 = bindings::SECTOR_SIZE;
> +
> +/// Power of two difference in size of a page and size of a sector.

A bit hard to parse this.

Maybe “The difference between the size of a page and the size of a sector,
expressed as a power of two” ?

> +pub const PAGE_SECTORS_SHIFT: u32 = bindings::PAGE_SECTORS_SHIFT;
> 
> -- 
> 2.47.2
> 
> 
> 


 let sector: usize = something;
 let page_offset = (sector & block::SECTOR_MASK as usize) << block::SECTOR_SHIFT;


Wait, the parenthesis evaluate to usize, and the shift is a u32. How does this compile?

— Daniel

Re: [PATCH v3 11/16] rnull: move driver to separate directory

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> The rust null block driver is about to gain some additional modules. Rather
> than pollute the current directory, move the driver to a subdirectory.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> MAINTAINERS                        |  2 +-
> drivers/block/Kconfig              | 10 +---------
> drivers/block/Makefile             |  4 +---
> drivers/block/rnull/Kconfig        | 13 +++++++++++++
> drivers/block/rnull/Makefile       |  3 +++
> drivers/block/{ => rnull}/rnull.rs |  0
> 6 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0c1d245bf7b8..29b14aec3559 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4246,7 +4246,7 @@ W: https://rust-for-linux.com
> B: https://github.com/Rust-for-Linux/linux/issues
> C: https://rust-for-linux.zulipchat.com/#narrow/stream/Block
> T: git https://github.com/Rust-for-Linux/linux.git rust-block-next
> -F: drivers/block/rnull.rs
> +F: drivers/block/rnull/
> F: rust/kernel/block.rs
> F: rust/kernel/block/
> 
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 0f70e2374e7f..6b50dbc0495b 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -17,6 +17,7 @@ menuconfig BLK_DEV
> if BLK_DEV
> 
> source "drivers/block/null_blk/Kconfig"
> +source "drivers/block/rnull/Kconfig"
> 
> config BLK_DEV_FD
> tristate "Normal floppy disk support"
> @@ -354,15 +355,6 @@ config VIRTIO_BLK
>  This is the virtual block driver for virtio.  It can be used with
>           QEMU based VMMs (like KVM or Xen).  Say Y or M.
> 
> -config BLK_DEV_RUST_NULL
> - tristate "Rust null block driver (Experimental)"
> - depends on RUST
> - help
> -  This is the Rust implementation of the null block driver. For now it
> -  is only a minimal stub.
> -
> -  If unsure, say N.
> -
> config BLK_DEV_RBD
> tristate "Rados block device (RBD)"
> depends on INET && BLOCK
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index 097707aca725..aba3e93d5014 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -9,9 +9,6 @@
> # needed for trace events
> ccflags-y += -I$(src)
> 
> -obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
> -rnull_mod-y := rnull.o
> -
> obj-$(CONFIG_MAC_FLOPPY) += swim3.o
> obj-$(CONFIG_BLK_DEV_SWIM) += swim_mod.o
> obj-$(CONFIG_BLK_DEV_FD) += floppy.o
> @@ -39,6 +36,7 @@ obj-$(CONFIG_ZRAM) += zram/
> obj-$(CONFIG_BLK_DEV_RNBD) += rnbd/
> 
> obj-$(CONFIG_BLK_DEV_NULL_BLK) += null_blk/
> +obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull/
> 
> obj-$(CONFIG_BLK_DEV_UBLK) += ublk_drv.o
> obj-$(CONFIG_BLK_DEV_ZONED_LOOP) += zloop.o
> diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
> new file mode 100644
> index 000000000000..6dc5aff96bf4
> --- /dev/null
> +++ b/drivers/block/rnull/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Rust null block device driver configuration
> +
> +config BLK_DEV_RUST_NULL
> + tristate "Rust null block driver (Experimental)"
> + depends on RUST
> + help
> +  This is the Rust implementation of the null block driver. Like
> +  the C version, the driver allows the user to create virutal block
> +  devices that can be configured via various configuration options.
> +
> +  If unsure, say N.
> diff --git a/drivers/block/rnull/Makefile b/drivers/block/rnull/Makefile
> new file mode 100644
> index 000000000000..11cfa5e615dc
> --- /dev/null
> +++ b/drivers/block/rnull/Makefile
> @@ -0,0 +1,3 @@
> +
> +obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
> +rnull_mod-y := rnull.o
> diff --git a/drivers/block/rnull.rs b/drivers/block/rnull/rnull.rs
> similarity index 100%
> rename from drivers/block/rnull.rs
> rename to drivers/block/rnull/rnull.rs
> 
> -- 
> 2.47.2
> 
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v3 12/16] rnull: enable configuration via `configfs`

[Daniel Almeida]

> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Allow rust null block devices to be configured and instantiated via
> `configfs`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> drivers/block/rnull/Kconfig      |   2 +-
> drivers/block/rnull/configfs.rs  | 220 +++++++++++++++++++++++++++++++++++++++
> drivers/block/rnull/rnull.rs     |  58 ++++++-----
> rust/kernel/block/mq/gen_disk.rs |   2 +-
> 4 files changed, 253 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
> index 6dc5aff96bf4..7bc5b376c128 100644
> --- a/drivers/block/rnull/Kconfig
> +++ b/drivers/block/rnull/Kconfig
> @@ -4,7 +4,7 @@
> 
> config BLK_DEV_RUST_NULL
> tristate "Rust null block driver (Experimental)"
> - depends on RUST
> + depends on RUST && CONFIGFS_FS

Should this really be a dependency? IIUC, the driver still works with this
unset, it just doesn’t have this feature?

> help
>  This is the Rust implementation of the null block driver. Like
>  the C version, the driver allows the user to create virutal block
> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
> new file mode 100644
> index 000000000000..6c0e3bbb36ec
> --- /dev/null
> +++ b/drivers/block/rnull/configfs.rs
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::{NullBlkDevice, THIS_MODULE};
> +use core::fmt::Write;
> +use kernel::{
> +    block::mq::gen_disk::{GenDisk, GenDiskBuilder},
> +    c_str,
> +    configfs::{self, AttributeOperations},
> +    configfs_attrs, new_mutex,
> +    page::PAGE_SIZE,
> +    prelude::*,
> +    str::CString,
> +    sync::Mutex,
> +};
> +use pin_init::PinInit;
> +
> +pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
> +    let item_type = configfs_attrs! {
> +        container: configfs::Subsystem<Config>,
> +        data: Config,
> +        child: DeviceConfig,
> +        attributes: [
> +            features: 0,
> +        ],
> +    };
> +
> +    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
> +}
> +
> +#[pin_data]
> +pub(crate) struct Config {}

This still builds:

diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
index 3ae84dfc8d62..2e5ffa82e679 100644
--- a/drivers/block/rnull/configfs.rs
+++ b/drivers/block/rnull/configfs.rs
@@ -24,10 +24,9 @@ pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, E
         ],
     };
 
-    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
+    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, Config {})
 }
 
-#[pin_data]
 pub(crate) struct Config {}

Perhaps due to:

// SAFETY: the `__pinned_init` function always returns `Ok(())` and initializes every field of
// `slot`. Additionally, all pinning invariants of `T` are upheld.
unsafe impl<T> PinInit<T> for T {
    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), Infallible> {
        // SAFETY: `slot` is valid for writes by the safety requirements of this function.
        unsafe { slot.write(self) };
        Ok(())
    }
}


> +
> +#[vtable]
> +impl AttributeOperations<0> for Config {
> +    type Data = Config;
> +
> +    fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_str("blocksize,size,rotational\n")?;
> +        Ok(writer.bytes_written())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Config {
> +    type Child = DeviceConfig;
> +
> +    fn make_group(
> +        &self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<DeviceConfig>, Error>> {
> +        let item_type = configfs_attrs! {
> +            container: configfs::Group<DeviceConfig>,
> +            data: DeviceConfig,
> +            attributes: [
> +                // Named for compatibility with C null_blk
> +                power: 0,
> +                blocksize: 1,
> +                rotational: 2,
> +                size: 3,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(
> +            name.try_into()?,
> +            item_type,
> +            // TODO: cannot coerce new_mutex!() to impl PinInit<_, Error>, so put mutex inside

Isn’t this related to [0] ?


> +            try_pin_init!( DeviceConfig {
> +                data <- new_mutex!( DeviceConfigInner {
> +                    powered: false,
> +                    block_size: 4096,
> +                    rotational: false,
> +                    disk: None,
> +                    capacity_mib: 4096,
> +                    name: name.try_into()?,
> +                }),
> +            }),
> +        ))
> +    }
> +}
> +
> +#[pin_data]
> +pub(crate) struct DeviceConfig {
> +    #[pin]
> +    data: Mutex<DeviceConfigInner>,
> +}
> +
> +#[pin_data]
> +struct DeviceConfigInner {
> +    powered: bool,
> +    name: CString,
> +    block_size: u32,
> +    rotational: bool,
> +    capacity_mib: u64,
> +    disk: Option<GenDisk<NullBlkDevice>>,
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +
> +        if this.data.lock().powered {
> +            writer.write_fmt(fmt!("1\n"))?;
> +        } else {
> +            writer.write_fmt(fmt!("0\n"))?;
> +        }
> +
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        let power_op: bool = core::str::from_utf8(page)?
> +            .trim()
> +            .parse::<u8>()
> +            .map_err(|_| kernel::error::code::EINVAL)?

nit: I’d import that if I were you, but it’s your call.

> +            != 0;
> +
> +        let mut guard = this.data.lock(); 
> +
> +        if !guard.powered && power_op {
> +            guard.disk = Some(NullBlkDevice::new(
> +                &guard.name,
> +                guard.block_size,
> +                guard.rotational,
> +                guard.capacity_mib,
> +            )?);
> +            guard.powered = true;
> +        } else if guard.powered && !power_op {
> +            drop(guard.disk.take());
> +            guard.powered = false;
> +        }

nit: the guard is not used here, but it is still alive. This is harmless in
this case, but as I general pattern, I find that using closures cuts back on
the scope, i.e.:

this.with_locked_data(|data| {
    // use the guard
});

// Guard is already free here, no surprises. 
 
> +
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<1> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_fmt(fmt!("{}\n", this.data.lock().block_size))?;
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        let text = core::str::from_utf8(page)?.trim();
> +        let value = text
> +            .parse::<u32>()
> +            .map_err(|_| kernel::error::code::EINVAL)?;
> +
> +        GenDiskBuilder::validate_block_size(value)?;
> +        this.data.lock().block_size = value;
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<2> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +
> +        if this.data.lock().rotational {
> +            writer.write_fmt(fmt!("1\n"))?;
> +        } else {
> +            writer.write_fmt(fmt!("0\n"))?;
> +        }
> +
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        this.data.lock().rotational = core::str::from_utf8(page)?
> +            .trim()
> +            .parse::<u8>()
> +            .map_err(|_| kernel::error::code::EINVAL)?
> +            != 0;
> +
> +        Ok(())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<3> for DeviceConfig {
> +    type Data = DeviceConfig;
> +
> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
> +        let mut writer = kernel::str::Formatter::new(page);
> +        writer.write_fmt(fmt!("{}\n", this.data.lock().capacity_mib))?;
> +        Ok(writer.bytes_written())
> +    }
> +
> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
> +        if this.data.lock().powered {
> +            return Err(EBUSY);
> +        }
> +
> +        let text = core::str::from_utf8(page)?.trim();
> +        let value = text
> +            .parse::<u64>()
> +            .map_err(|_| kernel::error::code::EINVAL)?;
> +
> +        this.data.lock().capacity_mib = value;
> +        Ok(())
> +    }
> +}
> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
> index d07e76ae2c13..d09bc77861e4 100644
> --- a/drivers/block/rnull/rnull.rs
> +++ b/drivers/block/rnull/rnull.rs
> @@ -1,28 +1,26 @@
> // SPDX-License-Identifier: GPL-2.0
> 
> //! This is a Rust implementation of the C null block driver.
> -//!
> -//! Supported features:
> -//!
> -//! - blk-mq interface
> -//! - direct completion
> -//! - block size 4k

Why are these three removed?

> -//!
> -//! The driver is not configurable.
> +
> +mod configfs;
> 
> use kernel::{
>     alloc::flags,
> -    block::mq::{
> +    block::{
>         self,
> -        gen_disk::{self, GenDisk},
> -        Operations, TagSet,
> +        mq::{
> +            self,
> +            gen_disk::{self, GenDisk},
> +            Operations, TagSet,
> +        },
>     },
>     error::Result,
> -    new_mutex, pr_info,
> +    pr_info,
>     prelude::*,
> -    sync::{Arc, Mutex},
> +    sync::Arc,
>     types::ARef,
> };
> +use pin_init::PinInit;
> 
> module! {
>     type: NullBlkModule,
> @@ -35,33 +33,39 @@
> #[pin_data]
> struct NullBlkModule {
>     #[pin]
> -    _disk: Mutex<GenDisk<NullBlkDevice>>,
> +    configfs_subsystem: kernel::configfs::Subsystem<configfs::Config>,
> }
> 
> impl kernel::InPlaceModule for NullBlkModule {
>     fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>         pr_info!("Rust null_blk loaded\n");
> 
> -        // Use a immediately-called closure as a stable `try` block
> -        let disk = /* try */ (|| {
> -            let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
> -
> -            gen_disk::GenDiskBuilder::new()
> -                .capacity_sectors(4096 << 11)
> -                .logical_block_size(4096)?
> -                .physical_block_size(4096)?
> -                .rotational(false)
> -                .build(format_args!("rnullb{}", 0), tagset)
> -        })();
> -
>         try_pin_init!(Self {
> -            _disk <- new_mutex!(disk?, "nullb:disk"),
> +            configfs_subsystem <- configfs::subsystem(),
>         })
>     }
> }
> 
> struct NullBlkDevice;
> 
> +impl NullBlkDevice {
> +    fn new(
> +        name: &CStr,
> +        block_size: u32,
> +        rotational: bool,
> +        capacity_mib: u64,
> +    ) -> Result<GenDisk<Self>> {
> +        let tagset = Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
> +
> +        gen_disk::GenDiskBuilder::new()
> +            .capacity_sectors(capacity_mib << (20 - block::SECTOR_SHIFT))
> +            .logical_block_size(block_size)?
> +            .physical_block_size(block_size)?
> +            .rotational(rotational)
> +            .build(fmt!("{}", name.to_str()?), tagset)
> +    }
> +}
> +
> #[vtable]
> impl Operations for NullBlkDevice {
>     #[inline(always)]
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index 39be2a31337f..7ab049ec591b 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -50,7 +50,7 @@ pub fn rotational(mut self, rotational: bool) -> Self {
> 
>     /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`,
>     /// and that it is a power of two.
> -    fn validate_block_size(size: u32) -> Result {
> +    pub fn validate_block_size(size: u32) -> Result {
>         if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() {
>             Err(error::code::EINVAL)
>         } else {
> 
> -- 
> 2.47.2
> 
> 
> 

— Daniel

[0]: https://github.com/Rust-for-Linux/linux/issues/1181

Re: [PATCH v3 06/16] rust: block: normalize imports for `gen_disk.rs`

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 6 Aug 2025, at 11:51, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
>>
>>>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>> Clean up the import statements in `gen_disk.rs` to make the code easier to
>>>> maintain.
>>>>
>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>>> ---
>>>> rust/kernel/block/mq/gen_disk.rs | 10 +++++++---
>>>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
>>>> index cd54cd64ea88..679ee1bb2195 100644
>>>> --- a/rust/kernel/block/mq/gen_disk.rs
>>>> +++ b/rust/kernel/block/mq/gen_disk.rs
>>>> @@ -5,9 +5,13 @@
>>>> //! C header: [`include/linux/blkdev.h`](srctree/include/linux/blkdev.h)
>>>> //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>>>>
>>>> -use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
>>>> -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
>>>> -use crate::{error, static_lock_class};
>>>> +use crate::{
>>>> +    bindings,
>>>> +    block::mq::{raw_writer::RawWriter, Operations, TagSet},
>>>> +    error::{self, from_err_ptr, Result},
>>>> +    static_lock_class,
>>>> +    sync::Arc,
>>>> +};
>>>> use core::fmt::{self, Write};
>>>>
>>>> /// A builder for [`GenDisk`].
>>>>
>>>> --
>>>> 2.47.2
>>>>
>>>>
>>>>
>>>
>>> Same comment as the preceding “import” patch: this is syntax is problematic.
>>
>> I used to share your viewpoint, but I changed my opinion and now prefer
>> "normalized" imports (the combined form).
>>
>> Now I can just blindly merge all the imports, remove duplicates and then
>> ask rust-analyzer to normalize imports again, and then format with
>> rustfmt. I find that this workflow is very low overhead.
>>
>>
>> Best regards,
>> Andreas Hindborg
>
> That’s because you have a separate commit where you do this before applying
> your work on top. If you’re rebasing on top of someone else's work, then a
> lot of conflicts will pop up. And unlike the saner approach where each import
> is in its own line, it’s now absolutely not clear how the conflicts should
> be resolved.
>
> The only thing that can be done then is to accept whatever the current change
> is, and ask rust-analyzer to reimport everything and reformat.
>
> IMHO, this is not great.

If we apply the normalized format everywhere, this will not be an issue.
Merging of imports can be almost automated. Just merge with strategy
keep all, and then normalize and remove duplicates.


Best regards,
Andreas Hindborg

Re: [PATCH v3 10/16] rust: block: add block related constants

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add a few block subsystem constants to the rust `kernel::block` name space.
>> This makes it easier to access the constants from rust code.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/block.rs | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/rust/kernel/block.rs b/rust/kernel/block.rs
>> index 150f710efe5b..7461adf4d7e0 100644
>> --- a/rust/kernel/block.rs
>> +++ b/rust/kernel/block.rs
>> @@ -3,3 +3,15 @@
>> //! Types for working with the block layer.
>>
>> pub mod mq;
>> +
>> +/// Bit mask for masking out [`SECTOR_SIZE`]
>
> Missing period.

Thanks.

>
>> +pub const SECTOR_MASK: u32 = bindings::SECTOR_MASK;
>> +
>> +/// Sectors are size `1 << SECTOR_SHIFT`.
>> +pub const SECTOR_SHIFT: u32 = bindings::SECTOR_SHIFT;
>> +
>> +/// Size of a sector.
>> +pub const SECTOR_SIZE: u32 = bindings::SECTOR_SIZE;
>> +
>> +/// Power of two difference in size of a page and size of a sector.
>
> A bit hard to parse this.
>
> Maybe “The difference between the size of a page and the size of a sector,
> expressed as a power of two” ?

OK.

>
>> +pub const PAGE_SECTORS_SHIFT: u32 = bindings::PAGE_SECTORS_SHIFT;
>>
>> --
>> 2.47.2
>>
>>
>>
>
>
>  let sector: usize = something;
>  let page_offset = (sector & block::SECTOR_MASK as usize) << block::SECTOR_SHIFT;
>
>
> Wait, the parenthesis evaluate to usize, and the shift is a u32. How does this compile?

That is all good. `Shl` has a ton of impls for integers [1].

Shifting more than 64 spaces on 64 bit system makes no sense anyway.

Best regards,
Andreas Hindborg


[1] https://doc.rust-lang.org/std/ops/trait.Shl.html

Re: [PATCH v3 12/16] rnull: enable configuration via `configfs`

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Allow rust null block devices to be configured and instantiated via
>> `configfs`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> drivers/block/rnull/Kconfig      |   2 +-
>> drivers/block/rnull/configfs.rs  | 220 +++++++++++++++++++++++++++++++++++++++
>> drivers/block/rnull/rnull.rs     |  58 ++++++-----
>> rust/kernel/block/mq/gen_disk.rs |   2 +-
>> 4 files changed, 253 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/block/rnull/Kconfig b/drivers/block/rnull/Kconfig
>> index 6dc5aff96bf4..7bc5b376c128 100644
>> --- a/drivers/block/rnull/Kconfig
>> +++ b/drivers/block/rnull/Kconfig
>> @@ -4,7 +4,7 @@
>>
>> config BLK_DEV_RUST_NULL
>> tristate "Rust null block driver (Experimental)"
>> - depends on RUST
>> + depends on RUST && CONFIGFS_FS
>
> Should this really be a dependency? IIUC, the driver still works with this
> unset, it just doesn’t have this feature?

It does not and I do not intend for it to operate without configfs.

I did not try to build without configfs enabled, but the rnull driver
has calls to symbols provided by the configfs subsystem, so it really
should not work without configfs loaded.

>
>> help
>>  This is the Rust implementation of the null block driver. Like
>>  the C version, the driver allows the user to create virutal block
>> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
>> new file mode 100644
>> index 000000000000..6c0e3bbb36ec
>> --- /dev/null
>> +++ b/drivers/block/rnull/configfs.rs
>> @@ -0,0 +1,220 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +use super::{NullBlkDevice, THIS_MODULE};
>> +use core::fmt::Write;
>> +use kernel::{
>> +    block::mq::gen_disk::{GenDisk, GenDiskBuilder},
>> +    c_str,
>> +    configfs::{self, AttributeOperations},
>> +    configfs_attrs, new_mutex,
>> +    page::PAGE_SIZE,
>> +    prelude::*,
>> +    str::CString,
>> +    sync::Mutex,
>> +};
>> +use pin_init::PinInit;
>> +
>> +pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
>> +    let item_type = configfs_attrs! {
>> +        container: configfs::Subsystem<Config>,
>> +        data: Config,
>> +        child: DeviceConfig,
>> +        attributes: [
>> +            features: 0,
>> +        ],
>> +    };
>> +
>> +    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
>> +}
>> +
>> +#[pin_data]
>> +pub(crate) struct Config {}
>
> This still builds:
>
> diff --git a/drivers/block/rnull/configfs.rs b/drivers/block/rnull/configfs.rs
> index 3ae84dfc8d62..2e5ffa82e679 100644
> --- a/drivers/block/rnull/configfs.rs
> +++ b/drivers/block/rnull/configfs.rs
> @@ -24,10 +24,9 @@ pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, E
>          ],
>      };
>
> -    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, try_pin_init!(Config {}))
> +    kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, Config {})
>  }
>
> -#[pin_data]
>  pub(crate) struct Config {}
>
> Perhaps due to:
>
> // SAFETY: the `__pinned_init` function always returns `Ok(())` and initializes every field of
> // `slot`. Additionally, all pinning invariants of `T` are upheld.
> unsafe impl<T> PinInit<T> for T {
>     unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), Infallible> {
>         // SAFETY: `slot` is valid for writes by the safety requirements of this function.
>         unsafe { slot.write(self) };
>         Ok(())
>     }
> }

Hmm, when I apply this change it does not work out for me:

      RUSTC [M] drivers/block/rnull/rnull.o
    error[E0277]: the trait bound `Config: PinInit<Config, kernel::error::Error>` is not satisfied
      --> /home/aeh/src/linux-rust/rnull-up-v6.16-rc1/drivers/block/rnull/configfs.rs:27:66
        |
    27  |     kernel::configfs::Subsystem::new(c_str!("rnull"), item_type, Config {})
        |     --------------------------------                             ^^^^^^^^^ the trait `PinInit<Config, kernel::error::Error>` is not implemented for `Config`
        |     |
        |     required by a bound introduced by this call
        |
        = help: the following other types implement trait `PinInit<T, E>`:
                  <AlwaysFail<T> as PinInit<T, ()>>
                  <ChainPinInit<I, F, T, E> as PinInit<T, E>>
                  <ChainInit<I, F, T, E> as PinInit<T, E>>
                  <core::result::Result<T, E> as PinInit<T, E>>
    note: required by a bound in `Subsystem::<Data>::new`
      --> /home/aeh/src/linux-rust/rnull-up-v6.16-rc1/rust/kernel/configfs.rs:151:20
        |
    148 |     pub fn new(
        |            --- required by a bound in this associated function
    ...
    151 |         data: impl PinInit<Data, Error>,
        |                    ^^^^^^^^^^^^^^^^^^^^ required by this bound in `Subsystem::<Data>::new`

    error[E0277]: the trait bound `Config: PinInit<Config, kernel::error::Error>` is not satisfied
      --> /home/aeh/src/linux-rust/rnull-up-v6.16-rc1/drivers/block/rnull/configfs.rs:17:30
        |
    17  | pub(crate) fn subsystem() -> impl PinInit<kernel::configfs::Subsystem<Config>, Error> {
        |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `PinInit<Config, kernel::error::Error>` is not implemented for `Config`
        |
        = help: the following other types implement trait `PinInit<T, E>`:
                  <AlwaysFail<T> as PinInit<T, ()>>
                  <ChainPinInit<I, F, T, E> as PinInit<T, E>>
                  <ChainInit<I, F, T, E> as PinInit<T, E>>
                  <core::result::Result<T, E> as PinInit<T, E>>
    note: required by a bound in `Subsystem::<Data>::new`
      --> /home/aeh/src/linux-rust/rnull-up-v6.16-rc1/rust/kernel/configfs.rs:151:20
        |
    148 |     pub fn new(
        |            --- required by a bound in this associated function
    ...
    151 |         data: impl PinInit<Data, Error>,
        |                    ^^^^^^^^^^^^^^^^^^^^ required by this bound in `Subsystem::<Data>::new`

    error: aborting due to 2 previous errors

I rebased on rust-6.17. What did you apply this series to?

>
>
>> +
>> +#[vtable]
>> +impl AttributeOperations<0> for Config {
>> +    type Data = Config;
>> +
>> +    fn show(_this: &Config, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +        writer.write_str("blocksize,size,rotational\n")?;
>> +        Ok(writer.bytes_written())
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::GroupOperations for Config {
>> +    type Child = DeviceConfig;
>> +
>> +    fn make_group(
>> +        &self,
>> +        name: &CStr,
>> +    ) -> Result<impl PinInit<configfs::Group<DeviceConfig>, Error>> {
>> +        let item_type = configfs_attrs! {
>> +            container: configfs::Group<DeviceConfig>,
>> +            data: DeviceConfig,
>> +            attributes: [
>> +                // Named for compatibility with C null_blk
>> +                power: 0,
>> +                blocksize: 1,
>> +                rotational: 2,
>> +                size: 3,
>> +            ],
>> +        };
>> +
>> +        Ok(configfs::Group::new(
>> +            name.try_into()?,
>> +            item_type,
>> +            // TODO: cannot coerce new_mutex!() to impl PinInit<_, Error>, so put mutex inside
>
> Isn’t this related to [0] ?

No, I think this is a type inference problem.

>
>
>> +            try_pin_init!( DeviceConfig {
>> +                data <- new_mutex!( DeviceConfigInner {
>> +                    powered: false,
>> +                    block_size: 4096,
>> +                    rotational: false,
>> +                    disk: None,
>> +                    capacity_mib: 4096,
>> +                    name: name.try_into()?,
>> +                }),
>> +            }),
>> +        ))
>> +    }
>> +}
>> +
>> +#[pin_data]
>> +pub(crate) struct DeviceConfig {
>> +    #[pin]
>> +    data: Mutex<DeviceConfigInner>,
>> +}
>> +
>> +#[pin_data]
>> +struct DeviceConfigInner {
>> +    powered: bool,
>> +    name: CString,
>> +    block_size: u32,
>> +    rotational: bool,
>> +    capacity_mib: u64,
>> +    disk: Option<GenDisk<NullBlkDevice>>,
>> +}
>> +
>> +#[vtable]
>> +impl configfs::AttributeOperations<0> for DeviceConfig {
>> +    type Data = DeviceConfig;
>> +
>> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +
>> +        if this.data.lock().powered {
>> +            writer.write_fmt(fmt!("1\n"))?;
>> +        } else {
>> +            writer.write_fmt(fmt!("0\n"))?;
>> +        }
>> +
>> +        Ok(writer.bytes_written())
>> +    }
>> +
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        let power_op: bool = core::str::from_utf8(page)?
>> +            .trim()
>> +            .parse::<u8>()
>> +            .map_err(|_| kernel::error::code::EINVAL)?
>
> nit: I’d import that if I were you, but it’s your call.

OK.

>
>> +            != 0;
>> +
>> +        let mut guard = this.data.lock();
>> +
>> +        if !guard.powered && power_op {
>> +            guard.disk = Some(NullBlkDevice::new(
>> +                &guard.name,
>> +                guard.block_size,
>> +                guard.rotational,
>> +                guard.capacity_mib,
>> +            )?);
>> +            guard.powered = true;
>> +        } else if guard.powered && !power_op {
>> +            drop(guard.disk.take());
>> +            guard.powered = false;
>> +        }
>
> nit: the guard is not used here, but it is still alive. This is harmless in
> this case, but as I general pattern, I find that using closures cuts back on
> the scope, i.e.:
>
> this.with_locked_data(|data| {
>     // use the guard
> });
>
> // Guard is already free here, no surprises.

I don't see `with_locked_data` anywhere in the kernel crate? It would be
a method on `Mutex`? Or would you add the method to `DeviceConfig`?

>
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::AttributeOperations<1> for DeviceConfig {
>> +    type Data = DeviceConfig;
>> +
>> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +        writer.write_fmt(fmt!("{}\n", this.data.lock().block_size))?;
>> +        Ok(writer.bytes_written())
>> +    }
>> +
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        if this.data.lock().powered {
>> +            return Err(EBUSY);
>> +        }
>> +
>> +        let text = core::str::from_utf8(page)?.trim();
>> +        let value = text
>> +            .parse::<u32>()
>> +            .map_err(|_| kernel::error::code::EINVAL)?;
>> +
>> +        GenDiskBuilder::validate_block_size(value)?;
>> +        this.data.lock().block_size = value;
>> +        Ok(())
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::AttributeOperations<2> for DeviceConfig {
>> +    type Data = DeviceConfig;
>> +
>> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +
>> +        if this.data.lock().rotational {
>> +            writer.write_fmt(fmt!("1\n"))?;
>> +        } else {
>> +            writer.write_fmt(fmt!("0\n"))?;
>> +        }
>> +
>> +        Ok(writer.bytes_written())
>> +    }
>> +
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        if this.data.lock().powered {
>> +            return Err(EBUSY);
>> +        }
>> +
>> +        this.data.lock().rotational = core::str::from_utf8(page)?
>> +            .trim()
>> +            .parse::<u8>()
>> +            .map_err(|_| kernel::error::code::EINVAL)?
>> +            != 0;
>> +
>> +        Ok(())
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::AttributeOperations<3> for DeviceConfig {
>> +    type Data = DeviceConfig;
>> +
>> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +        writer.write_fmt(fmt!("{}\n", this.data.lock().capacity_mib))?;
>> +        Ok(writer.bytes_written())
>> +    }
>> +
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        if this.data.lock().powered {
>> +            return Err(EBUSY);
>> +        }
>> +
>> +        let text = core::str::from_utf8(page)?.trim();
>> +        let value = text
>> +            .parse::<u64>()
>> +            .map_err(|_| kernel::error::code::EINVAL)?;
>> +
>> +        this.data.lock().capacity_mib = value;
>> +        Ok(())
>> +    }
>> +}
>> diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
>> index d07e76ae2c13..d09bc77861e4 100644
>> --- a/drivers/block/rnull/rnull.rs
>> +++ b/drivers/block/rnull/rnull.rs
>> @@ -1,28 +1,26 @@
>> // SPDX-License-Identifier: GPL-2.0
>>
>> //! This is a Rust implementation of the C null block driver.
>> -//!
>> -//! Supported features:
>> -//!
>> -//! - blk-mq interface
>> -//! - direct completion
>> -//! - block size 4k
>
> Why are these three removed?

Because the list is stale and I did not want to maintain it.


Best regards,
Andreas Hindborg

Re: [PATCH v3 12/16] rnull: enable configuration via `configfs`

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:13PM +0200, Andreas Hindborg wrote:

..

>> +#[vtable]
>> +impl configfs::AttributeOperations<0> for DeviceConfig {
>> +    type Data = DeviceConfig;
>> +
>> +    fn show(this: &DeviceConfig, page: &mut [u8; PAGE_SIZE]) -> Result<usize> {
>> +        let mut writer = kernel::str::Formatter::new(page);
>> +
>> +        if this.data.lock().powered {
>> +            writer.write_fmt(fmt!("1\n"))?;
>> +        } else {
>> +            writer.write_fmt(fmt!("0\n"))?;
>
> I think these can just be
> writer.write_str("1\n")?;

Cool 👍

>
>> +        }
>> +
>> +        Ok(writer.bytes_written())
>> +    }
>> +
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        let power_op: bool = core::str::from_utf8(page)?
>> +            .trim()
>> +            .parse::<u8>()
>> +            .map_err(|_| kernel::error::code::EINVAL)?
>> +            != 0;
>
> So if I write 27, that's treated as true, but if I write 300, that's an
> EINVAL?

Yea. Let's do this instead:

        let power_op_str = core::str::from_utf8(page)?.trim();

        let power_op = match power_op_str {
            "0" => Ok(false),
            "1" => Ok(true),
            _ => Err(EINVAL),
        }?;

It is closer to `kstrtobool`.


Best regards,
Andreas Hindborg

Re: [PATCH v3 13/16] rust: block: add `GenDisk` private data support

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:14PM +0200, Andreas Hindborg wrote:

..

>> @@ -97,7 +98,14 @@ pub fn build<T: Operations>(
>>          self,
>>          name: fmt::Arguments<'_>,
>>          tagset: Arc<TagSet<T>>,
>> +        queue_data: T::QueueData,
>>      ) -> Result<GenDisk<T>> {
>> +        let data = queue_data.into_foreign();
>> +        let recover_data = ScopeGuard::new(|| {
>> +            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> +            unsafe { T::QueueData::from_foreign(data) };
>
> I think this is clearer to read as
>
> drop(unsafe { T::QueueData::from_foreign(data) });

OK.

>
>> +        });
>> +
>>          // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>>          let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>>
>> @@ -112,7 +120,7 @@ pub fn build<T: Operations>(
>>              bindings::__blk_mq_alloc_disk(
>>                  tagset.raw_tag_set(),
>>                  &mut lim,
>> -                core::ptr::null_mut(),
>> +                data.cast(),
>>                  static_lock_class!().as_ptr(),
>>              )
>>          })?;
>> @@ -166,8 +174,12 @@ pub fn build<T: Operations>(
>>              },
>>          )?;
>>
>> +        recover_data.dismiss();
>> +
>>          // INVARIANT: `gendisk` was initialized above.
>>          // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
>> +        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
>> +        // `__blk_mq_alloc_disk` above.
>>          Ok(GenDisk {
>>              _tagset: tagset,
>>              gendisk,
>> @@ -179,9 +191,10 @@ pub fn build<T: Operations>(
>>  ///
>>  /// # Invariants
>>  ///
>> -/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> -/// - `gendisk` was added to the VFS through a call to
>> -///   `bindings::device_add_disk`.
>> +///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> +///  - `gendisk` was added to the VFS through a call to
>> +///    `bindings::device_add_disk`.
>> +///  - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
>>  pub struct GenDisk<T: Operations> {
>>      _tagset: Arc<TagSet<T>>,
>>      gendisk: *mut bindings::gendisk,
>> @@ -193,9 +206,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
>>
>>  impl<T: Operations> Drop for GenDisk<T> {
>>      fn drop(&mut self) {
>> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
>> +        // and initialized instance of `struct gendisk`, and, `queuedata` was
>> +        // initialized with the result of a call to
>> +        // `ForeignOwnable::into_foreign`.
>> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
>> +
>>          // SAFETY: By type invariant, `self.gendisk` points to a valid and
>>          // initialized instance of `struct gendisk`, and it was previously added
>>          // to the VFS.
>>          unsafe { bindings::del_gendisk(self.gendisk) };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
>> +        // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
>> +        // `ForeignOwnable::from_foreign` is only called here.
>> +        let _queue_data = unsafe { T::QueueData::from_foreign(queue_data.cast()) };
>
> Ditto here.
>
> drop(unsafe { T::QueueData::from_foreign(queue_data.cast()) });

Thanks, I agree.

>
> Also, is this cast necessary as of
> https://lore.kernel.org/all/20250711-rnull-up-v6-16-v3-13-3a262b4e2921@kernel.org/
> ?

I think you have the wrong link, you probably refer to the recent
`ForeignOwnable` change. And yes, the cast is redundant.


Best regards,
Andreas Hindborg