[PATCH 00/13] rust: kernel: documentation improvements

[PATCH 00/13] rust: kernel: documentation improvements

[Valentin Obst]

This patch set aims to make small improvements to the documentation of
the kernel crate. It engages in a few different activities:
- fixing trivial typos (commit #1)
- updating code examples to better reflect an idiomatic coding style
  (commits #2,6)
- increasing the consistency within the crate's documentation as a whole
  (commits #3,5,7,8,9,12,13)
- adding more intra-doc links as well as srctree-relative links to C
  header files (commits #4,10,11)

Valentin Obst (13):
  rust: kernel: fix multiple typos in documentation
  rust: error: move unsafe block into function call
  rust: ioctl: end top level module docs with full stop
  rust: kernel: add srctree-relative doclinks
  rust: str: use `NUL` instead of 0 in doc comments
  rust: str: move SAFETY comment in front of unsafe block
  rust: kernel: unify spelling of refcount in docs
  rust: kernel: mark code fragments in docs with backticks
  rust: kernel: add blank lines in front of code blocks
  rust: kernel: add doclinks
  rust: kernel: add doclinks with html tags
  rust: kernel: remove unneeded doclink targets
  rust: locked_by: shorten doclink preview

 rust/kernel/allocator.rs          |  2 +-
 rust/kernel/error.rs              |  7 +---
 rust/kernel/init.rs               | 16 +++----
 rust/kernel/ioctl.rs              |  6 +--
 rust/kernel/lib.rs                |  2 +-
 rust/kernel/str.rs                | 15 +++----
 rust/kernel/sync/arc.rs           | 34 ++++++++-------
 rust/kernel/sync/condvar.rs       |  2 +
 rust/kernel/sync/lock.rs          | 13 ++++--
 rust/kernel/sync/lock/spinlock.rs |  2 +-
 rust/kernel/sync/locked_by.rs     |  5 ++-
 rust/kernel/task.rs               |  6 +--
 rust/kernel/types.rs              |  3 ++
 rust/kernel/workqueue.rs          | 70 +++++++++++++++----------------
 14 files changed, 98 insertions(+), 85 deletions(-)

-- 
2.43.0

[PATCH 01/13] rust: kernel: fix multiple typos in documentation

[Valentin Obst]

Fixes multiple trivial typos in documentation and comments of the
kernel crate.

allocator:
- Fix a trivial list item alignment issue in the last SAFETY comment of
`krealloc_aligned`.

init:
- Replace 'type' with 'trait' in the doc comments of the `PinInit` and
`Init` traits.
- Add colons before starting lists.
- Add spaces between the type and equal sign to respect the code
formatting rules in example code.
- End a sentence with a full stop instead of a colon.

ioctl:
- Replace 'an' with 'a' where appropriate.

str:
- Replace 'Return' with 'Returns' in the doc comment of `bytes_written`
as the text describes what the function does.

sync/lock/spinlock:
- The code in this module operates on spinlocks, not mutexes. Thus,
replace 'mutex' with 'spinlock' in the SAFETY comment of `unlock`.

workqueue:
- Replace "wont" with "won't" in the doc comment of `__enqueue`.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/allocator.rs          |  2 +-
 rust/kernel/init.rs               | 16 ++++++++--------
 rust/kernel/ioctl.rs              |  4 ++--
 rust/kernel/str.rs                |  2 +-
 rust/kernel/sync/lock/spinlock.rs |  2 +-
 rust/kernel/workqueue.rs          |  2 +-
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 4b057e837358..01ad139e19bc 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -35,7 +35,7 @@ unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gf
     // - `ptr` is either null or a pointer returned from a previous `k{re}alloc()` by the
     //   function safety requirement.
     // - `size` is greater than 0 since it's either a `layout.size()` (which cannot be zero
-    //    according to the function safety requirement) or a result from `next_power_of_two()`.
+    //   according to the function safety requirement) or a result from `next_power_of_two()`.
     unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags) as *mut u8 }
 }
 
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 65be9ae57b80..16a99984622c 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -751,10 +751,10 @@ macro_rules! try_init {
 ///
 /// # Safety
 ///
-/// When implementing this type you will need to take great care. Also there are probably very few
+/// When implementing this trait you will need to take great care. Also there are probably very few
 /// cases where a manual implementation is necessary. Use [`pin_init_from_closure`] where possible.
 ///
-/// The [`PinInit::__pinned_init`] function
+/// The [`PinInit::__pinned_init`] function:
 /// - returns `Ok(())` if it initialized every field of `slot`,
 /// - returns `Err(err)` if it encountered an error and then cleaned `slot`, this means:
 ///     - `slot` can be deallocated without UB occurring,
@@ -861,10 +861,10 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
 ///
 /// # Safety
 ///
-/// When implementing this type you will need to take great care. Also there are probably very few
+/// When implementing this trait you will need to take great care. Also there are probably very few
 /// cases where a manual implementation is necessary. Use [`init_from_closure`] where possible.
 ///
-/// The [`Init::__init`] function
+/// The [`Init::__init`] function:
 /// - returns `Ok(())` if it initialized every field of `slot`,
 /// - returns `Err(err)` if it encountered an error and then cleaned `slot`, this means:
 ///     - `slot` can be deallocated without UB occurring,
@@ -1013,7 +1013,7 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
 ///
 /// ```rust
 /// use kernel::{error::Error, init::init_array_from_fn};
-/// let array: Box<[usize; 1_000]>= Box::init::<Error>(init_array_from_fn(|i| i)).unwrap();
+/// let array: Box<[usize; 1_000]> = Box::init::<Error>(init_array_from_fn(|i| i)).unwrap();
 /// assert_eq!(array.len(), 1_000);
 /// ```
 pub fn init_array_from_fn<I, const N: usize, T, E>(
@@ -1027,7 +1027,7 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
         // Counts the number of initialized elements and when dropped drops that many elements from
         // `slot`.
         let mut init_count = ScopeGuard::new_with_data(0, |i| {
-            // We now free every element that has been initialized before:
+            // We now free every element that has been initialized before.
             // SAFETY: The loop initialized exactly the values from 0..i and since we
             // return `Err` below, the caller will consider the memory at `slot` as
             // uninitialized.
@@ -1056,7 +1056,7 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
 ///
 /// ```rust
 /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
-/// let array: Arc<[Mutex<usize>; 1_000]>=
+/// let array: Arc<[Mutex<usize>; 1_000]> =
 ///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i))).unwrap();
 /// assert_eq!(array.len(), 1_000);
 /// ```
@@ -1071,7 +1071,7 @@ pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
         // Counts the number of initialized elements and when dropped drops that many elements from
         // `slot`.
         let mut init_count = ScopeGuard::new_with_data(0, |i| {
-            // We now free every element that has been initialized before:
+            // We now free every element that has been initialized before.
             // SAFETY: The loop initialized exactly the values from 0..i and since we
             // return `Err` below, the caller will consider the memory at `slot` as
             // uninitialized.
diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
index f1d42ab69972..59050e5f5a5a 100644
--- a/rust/kernel/ioctl.rs
+++ b/rust/kernel/ioctl.rs
@@ -28,13 +28,13 @@ pub const fn _IO(ty: u32, nr: u32) -> u32 {
     _IOC(uapi::_IOC_NONE, ty, nr, 0)
 }
 
-/// Build an ioctl number for an read-only ioctl.
+/// Build an ioctl number for a read-only ioctl.
 #[inline(always)]
 pub const fn _IOR<T>(ty: u32, nr: u32) -> u32 {
     _IOC(uapi::_IOC_READ, ty, nr, core::mem::size_of::<T>())
 }
 
-/// Build an ioctl number for an write-only ioctl.
+/// Build an ioctl number for a write-only ioctl.
 #[inline(always)]
 pub const fn _IOW<T>(ty: u32, nr: u32) -> u32 {
     _IOC(uapi::_IOC_WRITE, ty, nr, core::mem::size_of::<T>())
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 7d848b83add4..0a8569594fc3 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -449,7 +449,7 @@ pub(crate) fn pos(&self) -> *mut u8 {
         self.pos as _
     }
 
-    /// Return the number of bytes written to the formatter.
+    /// Returns the number of bytes written to the formatter.
     pub(crate) fn bytes_written(&self) -> usize {
         self.pos - self.beg
     }
diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
index 068535ce1b29..e5e0bf621988 100644
--- a/rust/kernel/sync/lock/spinlock.rs
+++ b/rust/kernel/sync/lock/spinlock.rs
@@ -112,7 +112,7 @@ unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState {
 
     unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
         // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
-        // caller is the owner of the mutex.
+        // caller is the owner of the spinlock.
         unsafe { bindings::spin_unlock(ptr) }
     }
 }
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 498397877376..8775c34d12a5 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -253,7 +253,7 @@ fn run(mut this: Pin<Box<Self>>) {
 /// actual value of the id is not important as long as you use different ids for different fields
 /// of the same struct. (Fields of different structs need not use different ids.)
 ///
-/// Note that the id is used only to select the right method to call during compilation. It wont be
+/// Note that the id is used only to select the right method to call during compilation. It won't be
 /// part of the final executable.
 ///
 /// # Safety
-- 
2.43.0

[PATCH 02/13] rust: error: move unsafe block into function call

[Valentin Obst]

The `from_err_ptr` function is safe. There is no need for the call to it
to be inside the unsafe block.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/error.rs | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 4f0c1edd63b7..6f6676bc0eb9 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -265,12 +265,7 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
 ///     index: u32,
 /// ) -> Result<*mut core::ffi::c_void> {
 ///     // SAFETY: FFI call.
-///     unsafe {
-///         from_err_ptr(bindings::devm_platform_ioremap_resource(
-///             pdev.to_ptr(),
-///             index,
-///         ))
-///     }
+///     from_err_ptr(unsafe { bindings::devm_platform_ioremap_resource(pdev.to_ptr(), index) })
 /// }
 /// ```
 // TODO: Remove `dead_code` marker once an in-kernel client is available.
-- 
2.43.0

[PATCH 03/13] rust: ioctl: end top level module docs with full stop

[Valentin Obst]

Every other module ends its first line of documentation with a full
stop. Adapt the only outlier.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/ioctl.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
index 59050e5f5a5a..5987ad6d38a7 100644
--- a/rust/kernel/ioctl.rs
+++ b/rust/kernel/ioctl.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-//! ioctl() number definitions
+//! ioctl() number definitions.
 //!
 //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h)
 
-- 
2.43.0

[PATCH 04/13] rust: kernel: add srctree-relative doclinks

[Valentin Obst]

Convert existing references to C header files to make use of
Commit bc2e7d5c298a ("rust: support `srctree`-relative links").

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/lib.rs          | 2 +-
 rust/kernel/sync/condvar.rs | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e6aff80b521f..0d365c71cae2 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -75,7 +75,7 @@ pub trait Module: Sized + Sync {
 
 /// Equivalent to `THIS_MODULE` in the C API.
 ///
-/// C header: `include/linux/export.h`
+/// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
 pub struct ThisModule(*mut bindings::module);
 
 // SAFETY: `THIS_MODULE` may be used from all threads within a module.
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index f65e19d5a37c..0bb76400efd9 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -77,6 +77,8 @@ pub struct CondVar {
 
     /// A condvar needs to be pinned because it contains a [`struct list_head`] that is
     /// self-referential, so it cannot be safely moved once it is initialised.
+    ///
+    /// [`struct list_head`]: srctree/include/linux/types.h
     #[pin]
     _pin: PhantomPinned,
 }
-- 
2.43.0

[PATCH 05/13] rust: str: use `NUL` instead of 0 in doc comments

[Valentin Obst]

Throughout the module, bytes with the value zero are referred to as
`NUL` bytes. Adapt the only two outliers.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/str.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 0a8569594fc3..843ffeec9b3e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -149,13 +149,13 @@ pub const fn as_char_ptr(&self) -> *const core::ffi::c_char {
         self.0.as_ptr() as _
     }
 
-    /// Convert the string to a byte slice without the trailing 0 byte.
+    /// Convert the string to a byte slice without the trailing `NUL` byte.
     #[inline]
     pub fn as_bytes(&self) -> &[u8] {
         &self.0[..self.len()]
     }
 
-    /// Convert the string to a byte slice containing the trailing 0 byte.
+    /// Convert the string to a byte slice containing the trailing `NUL` byte.
     #[inline]
     pub const fn as_bytes_with_nul(&self) -> &[u8] {
         &self.0
-- 
2.43.0

[PATCH 06/13] rust: str: move SAFETY comment in front of unsafe block

[Valentin Obst]

SAFETY comments should immediately precede the unsafe block they
justify. Move assignment to `bar` past comment as it is safe.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 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 843ffeec9b3e..fec5c4314758 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -191,9 +191,9 @@ pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> {
     /// ```
     /// # use kernel::c_str;
     /// # use kernel::str::CStr;
+    /// let bar = c_str!("ツ");
     /// // SAFETY: String literals are guaranteed to be valid UTF-8
     /// // by the Rust compiler.
-    /// let bar = c_str!("ツ");
     /// assert_eq!(unsafe { bar.as_str_unchecked() }, "ツ");
     /// ```
     #[inline]
-- 
2.43.0

[PATCH 07/13] rust: kernel: unify spelling of refcount in docs

[Valentin Obst]

Replace instances of 'ref-count[ed]' with 'refcount[ed]' to increase
consistency within the Rust documentation. The latter form is used more
widely in the rest of the kernel:

```console
$ rg '(\*|//).*?\srefcount(|ed)[\s,.]' | wc -l
1605
$ rg '(\*|//).*?\sref-count(|ed)[\s,.]' | wc -l
43
```

(numbers are for Commit 052d534373b7)

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/sync/arc.rs | 8 ++++----
 rust/kernel/task.rs     | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 77cdbcf7bd2e..6c46b1affca5 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -56,7 +56,7 @@
 ///     b: u32,
 /// }
 ///
-/// // Create a ref-counted instance of `Example`.
+/// // Create a refcounted instance of `Example`.
 /// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
 ///
 /// // Get a new pointer to `obj` and increment the refcount.
@@ -510,7 +510,7 @@ fn deref(&self) -> &Self::Target {
 /// # test().unwrap();
 /// ```
 ///
-/// In the following example we first allocate memory for a ref-counted `Example` but we don't
+/// In the following example we first allocate memory for a refcounted `Example` but we don't
 /// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`],
 /// followed by a conversion to `Arc<Example>`. This is particularly useful when allocation happens
 /// in one context (e.g., sleepable) and initialisation in another (e.g., atomic):
@@ -560,7 +560,7 @@ impl<T> UniqueArc<T> {
     /// Tries to allocate a new [`UniqueArc`] instance.
     pub fn try_new(value: T) -> Result<Self, AllocError> {
         Ok(Self {
-            // INVARIANT: The newly-created object has a ref-count of 1.
+            // INVARIANT: The newly-created object has a refcount of 1.
             inner: Arc::try_new(value)?,
         })
     }
@@ -574,7 +574,7 @@ pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
             data <- init::uninit::<T, AllocError>(),
         }? AllocError))?;
         Ok(UniqueArc {
-            // INVARIANT: The newly-created object has a ref-count of 1.
+            // INVARIANT: The newly-created object has a refcount of 1.
             // SAFETY: The pointer from the `Box` is valid.
             inner: unsafe { Arc::from_inner(Box::leak(inner).into()) },
         })
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 9451932d5d86..818ac51b06b6 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -23,7 +23,7 @@ macro_rules! current {
 ///
 /// All instances are valid tasks created by the C portion of the kernel.
 ///
-/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
+/// Instances of this type are always refcounted, that is, a call to `get_task_struct` ensures
 /// that the allocation remains valid at least until the matching call to `put_task_struct`.
 ///
 /// # Examples
@@ -147,7 +147,7 @@ pub fn wake_up(&self) {
     }
 }
 
-// SAFETY: The type invariants guarantee that `Task` is always ref-counted.
+// SAFETY: The type invariants guarantee that `Task` is always refcounted.
 unsafe impl crate::types::AlwaysRefCounted for Task {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
-- 
2.43.0

[PATCH 08/13] rust: kernel: mark code fragments in docs with backticks

[Valentin Obst]

Fix places where comments include code fragments that are not enclosed
in backticks.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/ioctl.rs     | 2 +-
 rust/kernel/sync/lock.rs | 2 +-
 rust/kernel/task.rs      | 2 +-
 rust/kernel/workqueue.rs | 9 +++++----
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
index 5987ad6d38a7..cfa7d080b531 100644
--- a/rust/kernel/ioctl.rs
+++ b/rust/kernel/ioctl.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-//! ioctl() number definitions.
+//! `ioctl()` number definitions.
 //!
 //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h)
 
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f12a684bc957..467249b39f71 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -28,7 +28,7 @@ pub unsafe trait Backend {
     /// The state required by the lock.
     type State;
 
-    /// The state required to be kept between lock and unlock.
+    /// The state required to be kept between `lock` and `unlock`.
     type GuardState;
 
     /// Initialises the lock.
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 818ac51b06b6..d4b0d232480d 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -116,7 +116,7 @@ fn deref(&self) -> &Self::Target {
     /// Returns the group leader of the given task.
     pub fn group_leader(&self) -> &Task {
         // SAFETY: By the type invariant, we know that `self.0` is a valid task. Valid tasks always
-        // have a valid group_leader.
+        // have a valid `group_leader`.
         let ptr = unsafe { *ptr::addr_of!((*self.0.get()).group_leader) };
 
         // SAFETY: The lifetime of the returned task reference is tied to the lifetime of `self`,
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 8775c34d12a5..d900dc911149 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -168,7 +168,7 @@ impl Queue {
     /// # Safety
     ///
     /// The caller must ensure that the provided raw pointer is not dangling, that it points at a
-    /// valid workqueue, and that it remains valid until the end of 'a.
+    /// valid workqueue, and that it remains valid until the end of `'a`.
     pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue {
         // SAFETY: The `Queue` type is `#[repr(transparent)]`, so the pointer cast is valid. The
         // caller promises that the pointer is not dangling.
@@ -414,8 +414,8 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
 ///
 /// # Safety
 ///
-/// The [`OFFSET`] constant must be the offset of a field in Self of type [`Work<T, ID>`]. The methods on
-/// this trait must have exactly the behavior that the definitions given below have.
+/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
+/// methods on this trait must have exactly the behavior that the definitions given below have.
 ///
 /// [`Work<T, ID>`]: Work
 /// [`impl_has_work!`]: crate::impl_has_work
@@ -428,7 +428,8 @@ pub unsafe trait HasWork<T, const ID: u64 = 0> {
 
     /// Returns the offset of the [`Work<T, ID>`] field.
     ///
-    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not Sized.
+    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
+    /// `Sized`.
     ///
     /// [`Work<T, ID>`]: Work
     /// [`OFFSET`]: HasWork::OFFSET
-- 
2.43.0

[PATCH 09/13] rust: kernel: add blank lines in front of code blocks

[Valentin Obst]

Throughout the code base, blank lines are used before starting a code
block. Adapt outliers to improve consistency within the kernel crate.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/types.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index fdb778e65d79..8aabe348b194 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -90,6 +90,7 @@ unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
 ///
 /// In the example below, we have multiple exit paths and we want to log regardless of which one is
 /// taken:
+///
 /// ```
 /// # use kernel::types::ScopeGuard;
 /// fn example1(arg: bool) {
@@ -108,6 +109,7 @@ unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
 ///
 /// In the example below, we want to log the same message on all early exits but a different one on
 /// the main exit path:
+///
 /// ```
 /// # use kernel::types::ScopeGuard;
 /// fn example2(arg: bool) {
@@ -129,6 +131,7 @@ unsafe fn from_foreign(_: *const core::ffi::c_void) -> Self {}
 ///
 /// In the example below, we need a mutable object (the vector) to be accessible within the log
 /// function, so we wrap it in the [`ScopeGuard`]:
+///
 /// ```
 /// # use kernel::types::ScopeGuard;
 /// fn example3(arg: bool) -> Result {
-- 
2.43.0

[PATCH 10/13] rust: kernel: add doclinks

[Valentin Obst]

Add doclinks to existing documentation.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/sync/arc.rs  |  6 +++---
 rust/kernel/sync/lock.rs | 13 +++++++++---
 rust/kernel/workqueue.rs | 45 ++++++++++++++++++++++++----------------
 3 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 6c46b1affca5..936bc549a082 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -365,12 +365,12 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
 /// A borrowed reference to an [`Arc`] instance.
 ///
 /// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
-/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
+/// to use just `&T`, which we can trivially get from an [`Arc<T>`] instance.
 ///
 /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
 /// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
-/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
-/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
+/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
+/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
 /// needed.
 ///
 /// # Invariants
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 467249b39f71..f14179d19d4e 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -21,14 +21,21 @@
 /// # Safety
 ///
 /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
-/// is owned, that is, between calls to `lock` and `unlock`.
-/// - Implementers must also ensure that `relock` uses the same locking method as the original
+/// is owned, that is, between calls to [`lock`] and [`unlock`].
+/// - Implementers must also ensure that [`relock`] uses the same locking method as the original
 /// lock operation.
+///
+/// [`lock`]: Backend::lock
+/// [`unlock`]: Backend::unlock
+/// [`relock`]: Backend::relock
 pub unsafe trait Backend {
     /// The state required by the lock.
     type State;
 
-    /// The state required to be kept between `lock` and `unlock`.
+    /// The state required to be kept between [`lock`] and [`unlock`].
+    ///
+    /// [`lock`]: Backend::lock
+    /// [`unlock`]: Backend::unlock
     type GuardState;
 
     /// Initialises the lock.
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index d900dc911149..ed3af3491b47 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -12,19 +12,19 @@
 //!
 //! # The raw API
 //!
-//! The raw API consists of the `RawWorkItem` trait, where the work item needs to provide an
+//! The raw API consists of the [`RawWorkItem`] trait, where the work item needs to provide an
 //! arbitrary function that knows how to enqueue the work item. It should usually not be used
 //! directly, but if you want to, you can use it without using the pieces from the safe API.
 //!
 //! # The safe API
 //!
-//! The safe API is used via the `Work` struct and `WorkItem` traits. Furthermore, it also includes
-//! a trait called `WorkItemPointer`, which is usually not used directly by the user.
+//! The safe API is used via the [`Work`] struct and [`WorkItem`] traits. Furthermore, it also
+//! includes a trait called [`WorkItemPointer`], which is usually not used directly by the user.
 //!
-//!  * The `Work` struct is the Rust wrapper for the C `work_struct` type.
-//!  * The `WorkItem` trait is implemented for structs that can be enqueued to a workqueue.
-//!  * The `WorkItemPointer` trait is implemented for the pointer type that points at a something
-//!    that implements `WorkItem`.
+//!  * The [`Work`] struct is the Rust wrapper for the C `work_struct` type.
+//!  * The [`WorkItem`] trait is implemented for structs that can be enqueued to a workqueue.
+//!  * The [`WorkItemPointer`] trait is implemented for the pointer type that points at a something
+//!    that implements [`WorkItem`].
 //!
 //! ## Example
 //!
@@ -218,7 +218,9 @@ pub fn try_spawn<T: 'static + Send + FnOnce()>(&self, func: T) -> Result<(), All
     }
 }
 
-/// A helper type used in `try_spawn`.
+/// A helper type used in [`try_spawn`].
+///
+/// [`try_spawn`]: Queue::try_spawn
 #[pin_data]
 struct ClosureWork<T> {
     #[pin]
@@ -258,9 +260,11 @@ fn run(mut this: Pin<Box<Self>>) {
 ///
 /// # Safety
 ///
-/// Implementers must ensure that any pointers passed to a `queue_work_on` closure by `__enqueue`
+/// Implementers must ensure that any pointers passed to a `queue_work_on` closure by [`__enqueue`]
 /// remain valid for the duration specified in the guarantees section of the documentation for
-/// `__enqueue`.
+/// [`__enqueue`].
+///
+/// [`__enqueue`]: RawWorkItem::__enqueue
 pub unsafe trait RawWorkItem<const ID: u64> {
     /// The return type of [`Queue::enqueue`].
     type EnqueueOutput;
@@ -290,10 +294,11 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
 
 /// Defines the method that should be called directly when a work item is executed.
 ///
-/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
+/// This trait is implemented by `Pin<Box<T>>` and [`Arc<T>`], and is mainly intended to be
 /// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
-/// instead. The `run` method on this trait will usually just perform the appropriate
-/// `container_of` translation and then call into the `run` method from the [`WorkItem`] trait.
+/// instead. The [`run`] method on this trait will usually just perform the appropriate
+/// `container_of` translation and then call into the [`run`][WorkItem::run] method from the
+/// [`WorkItem`] trait.
 ///
 /// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
 ///
@@ -309,8 +314,10 @@ pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
     ///
     /// # Safety
     ///
-    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
-    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
+    /// The provided `work_struct` pointer must originate from a previous call to [`__enqueue`]
+    /// where the `queue_work_on` closure returned true, and the pointer must still be valid.
+    ///
+    /// [`__enqueue`]: RawWorkItem::__enqueue
     unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
 }
 
@@ -328,12 +335,14 @@ pub trait WorkItem<const ID: u64 = 0> {
 
 /// Links for a work item.
 ///
-/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
+/// This struct contains a function pointer to the [`run`] function from the [`WorkItemPointer`]
 /// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
 ///
 /// Wraps the kernel's C `struct work_struct`.
 ///
 /// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
+///
+/// [`run`]: WorkItemPointer::run
 #[repr(transparent)]
 pub struct Work<T: ?Sized, const ID: u64 = 0> {
     work: Opaque<bindings::work_struct>,
@@ -409,7 +418,7 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
 /// }
 /// ```
 ///
-/// Note that since the `Work` type is annotated with an id, you can have several `work_struct`
+/// Note that since the [`Work`] type is annotated with an id, you can have several `work_struct`
 /// fields by using a different id for each one.
 ///
 /// # Safety
@@ -429,7 +438,7 @@ pub unsafe trait HasWork<T, const ID: u64 = 0> {
     /// Returns the offset of the [`Work<T, ID>`] field.
     ///
     /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
-    /// `Sized`.
+    /// [`Sized`].
     ///
     /// [`Work<T, ID>`]: Work
     /// [`OFFSET`]: HasWork::OFFSET
-- 
2.43.0

[PATCH 11/13] rust: kernel: add doclinks with html tags

[Valentin Obst]

Add doclinks to existing documentation. Use html 'code' tags to add
links to items that cannot be linked with the normal syntax.

The use of html tags is a tradeoff between the readability of the
documentation's source code and the ergonomics of the generated content.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/str.rs       |  7 ++++---
 rust/kernel/sync/arc.rs  | 24 +++++++++++++-----------
 rust/kernel/workqueue.rs | 10 +++++-----
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index fec5c4314758..f95fd2ba19fb 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -14,7 +14,8 @@
 
 /// Byte string without UTF-8 validity guarantee.
 ///
-/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
+/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
+/// semantical meaning.
 pub type BStr = [u8];
 
 /// Creates a new [`BStr`] from a string literal.
@@ -106,7 +107,7 @@ pub unsafe fn from_char_ptr<'a>(ptr: *const core::ffi::c_char) -> &'a Self {
         unsafe { Self::from_bytes_with_nul_unchecked(bytes) }
     }
 
-    /// Creates a [`CStr`] from a `[u8]`.
+    /// Creates a [`CStr`] from a <code>[[u8]]</code>.
     ///
     /// The provided slice must be `NUL`-terminated, does not contain any
     /// interior `NUL` bytes.
@@ -130,7 +131,7 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError
         Ok(unsafe { Self::from_bytes_with_nul_unchecked(bytes) })
     }
 
-    /// Creates a [`CStr`] from a `[u8]` without performing any additional
+    /// Creates a [`CStr`] from a <code>[[u8]]</code> without performing any additional
     /// checks.
     ///
     /// # Safety
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 936bc549a082..5fcd4b0fd84b 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -368,10 +368,10 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
 /// to use just `&T`, which we can trivially get from an [`Arc<T>`] instance.
 ///
 /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
-/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
-/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
-/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
-/// needed.
+/// over <code>&[`Arc<T>`]</code> because the latter results in a double-indirection: a pointer
+/// (shared reference) to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates
+/// this double indirection while still allowing one to increment the refcount and getting an
+/// [`Arc<T>`] when/if needed.
 ///
 /// # Invariants
 ///
@@ -489,8 +489,8 @@ fn deref(&self) -> &Self::Target {
 /// # Examples
 ///
 /// In the following example, we make changes to the inner object before turning it into an
-/// `Arc<Test>` object (after which point, it cannot be mutated directly). Note that `x.into()`
-/// cannot fail.
+/// <code>[Arc]\<Test\></code> object (after which point, it cannot be mutated directly).
+/// Note that `x.into()` cannot fail.
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -512,8 +512,9 @@ fn deref(&self) -> &Self::Target {
 ///
 /// In the following example we first allocate memory for a refcounted `Example` but we don't
 /// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`],
-/// followed by a conversion to `Arc<Example>`. This is particularly useful when allocation happens
-/// in one context (e.g., sleepable) and initialisation in another (e.g., atomic):
+/// followed by a conversion to <code>[Arc]\<Example\></code>. This is particularly useful when
+/// allocation happens in one context (e.g., sleepable) and initialisation in another
+/// (e.g., atomic):
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -532,8 +533,8 @@ fn deref(&self) -> &Self::Target {
 /// ```
 ///
 /// In the last example below, the caller gets a pinned instance of `Example` while converting to
-/// `Arc<Example>`; this is useful in scenarios where one needs a pinned reference during
-/// initialisation, for example, when initialising fields that are wrapped in locks.
+/// <code>[Arc]\<Example\></code>; this is useful in scenarios where one needs a pinned reference
+/// during initialisation, for example, when initialising fields that are wrapped in locks.
 ///
 /// ```
 /// use kernel::sync::{Arc, UniqueArc};
@@ -582,7 +583,8 @@ pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
 }
 
 impl<T> UniqueArc<MaybeUninit<T>> {
-    /// Converts a `UniqueArc<MaybeUninit<T>>` into a `UniqueArc<T>` by writing a value into it.
+    /// Converts a <code>UniqueArc<[`MaybeUninit<T>`]></code> into a `UniqueArc<T>`
+    /// by writing a value into it.
     pub fn write(mut self, value: T) -> UniqueArc<T> {
         self.deref_mut().write(value);
         // SAFETY: We just wrote the value to be initialized.
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index ed3af3491b47..aedf47f258bd 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -294,9 +294,9 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
 
 /// Defines the method that should be called directly when a work item is executed.
 ///
-/// This trait is implemented by `Pin<Box<T>>` and [`Arc<T>`], and is mainly intended to be
-/// implemented for smart pointer types. For your own structs, you would implement [`WorkItem`]
-/// instead. The [`run`] method on this trait will usually just perform the appropriate
+/// This trait is implemented by <code>[Pin]<[`Box<T>`]></code> and [`Arc<T>`], and is mainly
+/// intended to be implemented for smart pointer types. For your own structs, you would implement
+/// [`WorkItem`] instead. The [`run`] method on this trait will usually just perform the appropriate
 /// `container_of` translation and then call into the [`run`][WorkItem::run] method from the
 /// [`WorkItem`] trait.
 ///
@@ -325,8 +325,8 @@ pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
 ///
 /// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
 pub trait WorkItem<const ID: u64 = 0> {
-    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
-    /// `Pin<Box<Self>>`.
+    /// The pointer type that this struct is wrapped in. This will typically be
+    /// <code>[Arc]\<Self\></code> or <code>[Pin]<[Box]\<Self\>></code>.
     type Pointer: WorkItemPointer<ID>;
 
     /// The method that should be called when this work item is executed.
-- 
2.43.0

[PATCH 12/13] rust: kernel: remove unneeded doclink targets

[Valentin Obst]

Remove explicit targets for doclinks in cases where rustdoc can
determine the correct target by itself. The goal is to reduce verbosity
in the source code.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/workqueue.rs | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index aedf47f258bd..f63190b563d8 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -426,13 +426,10 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
 /// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
 /// methods on this trait must have exactly the behavior that the definitions given below have.
 ///
-/// [`Work<T, ID>`]: Work
 /// [`impl_has_work!`]: crate::impl_has_work
 /// [`OFFSET`]: HasWork::OFFSET
 pub unsafe trait HasWork<T, const ID: u64 = 0> {
     /// The offset of the [`Work<T, ID>`] field.
-    ///
-    /// [`Work<T, ID>`]: Work
     const OFFSET: usize;
 
     /// Returns the offset of the [`Work<T, ID>`] field.
@@ -440,7 +437,6 @@ pub unsafe trait HasWork<T, const ID: u64 = 0> {
     /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
     /// [`Sized`].
     ///
-    /// [`Work<T, ID>`]: Work
     /// [`OFFSET`]: HasWork::OFFSET
     #[inline]
     fn get_work_offset(&self) -> usize {
@@ -452,8 +448,6 @@ fn get_work_offset(&self) -> usize {
     /// # Safety
     ///
     /// The provided pointer must point at a valid struct of type `Self`.
-    ///
-    /// [`Work<T, ID>`]: Work
     #[inline]
     unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
         // SAFETY: The caller promises that the pointer is valid.
@@ -465,8 +459,6 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
     /// # Safety
     ///
     /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
-    ///
-    /// [`Work<T, ID>`]: Work
     #[inline]
     unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
     where
@@ -495,8 +487,6 @@ unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
 ///     impl HasWork<MyStruct, 17> for MyStruct { self.work_field }
 /// }
 /// ```
-///
-/// [`HasWork<T, ID>`]: HasWork
 #[macro_export]
 macro_rules! impl_has_work {
     ($(impl$(<$($implarg:ident),*>)?
-- 
2.43.0

[PATCH 13/13] rust: locked_by: shorten doclink preview

[Valentin Obst]

Increases readability by removing `super::` from the link preview
text.

Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
 rust/kernel/sync/locked_by.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
index b17ee5cd98f3..22c38993bf63 100644
--- a/rust/kernel/sync/locked_by.rs
+++ b/rust/kernel/sync/locked_by.rs
@@ -9,7 +9,7 @@
 /// Allows access to some data to be serialised by a lock that does not wrap it.
 ///
 /// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
-/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
+/// [`Mutex`] or [`SpinLock`]. [`LockedBy`] is meant for cases when this is not
 /// possible. For example, if a container has a lock and some data in the contained elements needs
 /// to be protected by the same lock.
 ///
@@ -17,6 +17,9 @@
 /// when the caller shows evidence that the 'external' lock is locked. It panics if the evidence
 /// refers to the wrong instance of the lock.
 ///
+/// [`Mutex`]: super::Mutex
+/// [`SpinLock`]: super::SpinLock
+///
 /// # Examples
 ///
 /// The following is an example for illustrative purposes: `InnerDirectory::bytes_used` is an
-- 
2.43.0

Re: [PATCH 00/13] rust: kernel: documentation improvements

[Martin Rodriguez Reboredo]

On 1/16/24 13:01, Valentin Obst wrote:
> This patch set aims to make small improvements to the documentation of
> the kernel crate. It engages in a few different activities:
> - fixing trivial typos (commit #1)
> - updating code examples to better reflect an idiomatic coding style
>    (commits #2,6)
> - increasing the consistency within the crate's documentation as a whole
>    (commits #3,5,7,8,9,12,13)
> - adding more intra-doc links as well as srctree-relative links to C
>    header files (commits #4,10,11)
> 
> Valentin Obst (13):
>    rust: kernel: fix multiple typos in documentation
>    rust: error: move unsafe block into function call
>    rust: ioctl: end top level module docs with full stop
>    rust: kernel: add srctree-relative doclinks
>    rust: str: use `NUL` instead of 0 in doc comments
>    rust: str: move SAFETY comment in front of unsafe block
>    rust: kernel: unify spelling of refcount in docs
>    rust: kernel: mark code fragments in docs with backticks
>    rust: kernel: add blank lines in front of code blocks
>    rust: kernel: add doclinks
>    rust: kernel: add doclinks with html tags
>    rust: kernel: remove unneeded doclink targets
>    rust: locked_by: shorten doclink preview
> 
>   rust/kernel/allocator.rs          |  2 +-
>   rust/kernel/error.rs              |  7 +---
>   rust/kernel/init.rs               | 16 +++----
>   rust/kernel/ioctl.rs              |  6 +--
>   rust/kernel/lib.rs                |  2 +-
>   rust/kernel/str.rs                | 15 +++----
>   rust/kernel/sync/arc.rs           | 34 ++++++++-------
>   rust/kernel/sync/condvar.rs       |  2 +
>   rust/kernel/sync/lock.rs          | 13 ++++--
>   rust/kernel/sync/lock/spinlock.rs |  2 +-
>   rust/kernel/sync/locked_by.rs     |  5 ++-
>   rust/kernel/task.rs               |  6 +--
>   rust/kernel/types.rs              |  3 ++
>   rust/kernel/workqueue.rs          | 70 +++++++++++++++----------------
>   14 files changed, 98 insertions(+), 85 deletions(-)
> 

Add my `Reviewed-By` to all the patch series except [PATCH 11/13].

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH 11/13] rust: kernel: add doclinks with html tags

[Martin Rodriguez Reboredo]

On 1/16/24 20:11, Valentin Obst wrote:
> Add doclinks to existing documentation. Use html 'code' tags to add
> links to items that cannot be linked with the normal syntax.
> 
> The use of html tags is a tradeoff between the readability of the
> documentation's source code and the ergonomics of the generated content.
> 
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
> [...]
> @@ -14,7 +14,8 @@
>   
>   /// Byte string without UTF-8 validity guarantee.
>   ///
> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> +/// semantical meaning.

Isn't there a way to escape square brackets with backslashes with
mbBook? Like `\[qux\]` or something? I ask this because this affects the
readability of the doc comment so if that could be omitted it'll be
really good.

>   pub type BStr = [u8];
>   
>   /// Creates a new [`BStr`] from a string literal.
> [...]

Re: [PATCH 11/13] rust: kernel: add doclinks with html tags

[Valentin Obst]

> > [...]
> > @@ -14,7 +14,8 @@
> >     /// Byte string without UTF-8 validity guarantee.
> >   ///
> > -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> > +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> > +/// semantical meaning.

> Isn't there a way to escape square brackets with backslashes with
> mbBook? Like `\[qux\]` or something? I ask this because this affects the
> readability of the doc comment so if that could be omitted it'll be
> really good.

Here are the things that I tried that did not produce a link:
[`[u8]`], `[[u8]]`, `[\[u8\]]`, `\[u8\]`, [`\[u8\]`], `[[u8](u8)]`,
`[[u8][u8]]`,

This results in a link, but it includes the square brackets:
[`[u8]`][u8], [`[u8]`](u8)

This results in a link that only includes the `u8`, but it is not
formatted as code:
[[u8]]

The only other examples of linked slices that I found are in the
standard library [1].

My assuption is that crate documentation is much more often consumed in
its rendered form, which is why I think the reduced readability is ok.
However, if that is not the case this change might be a bad idea.

[1]: https://doc.rust-lang.org/src/alloc/ffi/c_str.rs.html#58

Re: [PATCH 11/13] rust: kernel: add doclinks with html tags

[Martin Rodriguez Reboredo]

On 1/17/24 06:10, Valentin Obst wrote:
>>> [...]
>>> @@ -14,7 +14,8 @@
>>>      /// Byte string without UTF-8 validity guarantee.
>>>    ///
>>> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
>>> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
>>> +/// semantical meaning.
> 
>> Isn't there a way to escape square brackets with backslashes with
>> mbBook? Like `\[qux\]` or something? I ask this because this affects the
>> readability of the doc comment so if that could be omitted it'll be
>> really good.
> 
> Here are the things that I tried that did not produce a link:
> [`[u8]`], `[[u8]]`, `[\[u8\]]`, `\[u8\]`, [`\[u8\]`], `[[u8](u8)]`,
> `[[u8][u8]]`,
> 
> This results in a link, but it includes the square brackets:
> [`[u8]`][u8], [`[u8]`](u8)
> 
> This results in a link that only includes the `u8`, but it is not
> formatted as code:
> [[u8]]
> 
> The only other examples of linked slices that I found are in the
> standard library [1].
> 
> My assuption is that crate documentation is much more often consumed in
> its rendered form, which is why I think the reduced readability is ok.
> However, if that is not the case this change might be a bad idea.
> 
> [1]: https://doc.rust-lang.org/src/alloc/ffi/c_str.rs.html#58

I have an idea, let's just omit links to sliced types if they already
have their underlying type linked nearby. As for `[u8]` I think that we
can omit it too since readers of the documentation should be
familiarized with slices.

Re: [PATCH 02/13] rust: error: move unsafe block into function call

[Trevor Gross]

On Tue, Jan 16, 2024 at 11:05 AM Valentin Obst <kernel@valentinobst.de> wrote:
>
> The `from_err_ptr` function is safe. There is no need for the call to it
> to be inside the unsafe block.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/error.rs | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 4f0c1edd63b7..6f6676bc0eb9 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -265,12 +265,7 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>  ///     index: u32,
>  /// ) -> Result<*mut core::ffi::c_void> {
>  ///     // SAFETY: FFI call.
> -///     unsafe {
> -///         from_err_ptr(bindings::devm_platform_ioremap_resource(
> -///             pdev.to_ptr(),
> -///             index,
> -///         ))
> -///     }
> +///     from_err_ptr(unsafe { bindings::devm_platform_ioremap_resource(pdev.to_ptr(), index) })
>  /// }
>  /// ```
>  // TODO: Remove `dead_code` marker once an in-kernel client is available.
> --
> 2.43.0
>
>

If you're up for it, that safety comment could also be improved. Something like

    // SAFETY: `pdev` points to a valid platform device

But that is noncritical.

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 03/13] rust: ioctl: end top level module docs with full stop

[Trevor Gross]

On Tue, Jan 16, 2024 at 11:05 AM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Every other module ends its first line of documentation with a full
> stop. Adapt the only outlier.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/ioctl.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
> index 59050e5f5a5a..5987ad6d38a7 100644
> --- a/rust/kernel/ioctl.rs
> +++ b/rust/kernel/ioctl.rs
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> -//! ioctl() number definitions
> +//! ioctl() number definitions.

Would you mind also adding backticks to `ioctl()` while you're here?

- Trevor

>  //!
>  //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h)
>
> --
> 2.43.0
>

Re: [PATCH 04/13] rust: kernel: add srctree-relative doclinks

[Trevor Gross]

On Tue, Jan 16, 2024 at 5:35 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Convert existing references to C header files to make use of
> Commit bc2e7d5c298a ("rust: support `srctree`-relative links").
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/lib.rs          | 2 +-
>  rust/kernel/sync/condvar.rs | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e6aff80b521f..0d365c71cae2 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -75,7 +75,7 @@ pub trait Module: Sized + Sync {
>
>  /// Equivalent to `THIS_MODULE` in the C API.
>  ///
> -/// C header: `include/linux/export.h`
> +/// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
>  pub struct ThisModule(*mut bindings::module);
>
>  // SAFETY: `THIS_MODULE` may be used from all threads within a module.
> diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
> index f65e19d5a37c..0bb76400efd9 100644
> --- a/rust/kernel/sync/condvar.rs
> +++ b/rust/kernel/sync/condvar.rs
> @@ -77,6 +77,8 @@ pub struct CondVar {
>
>      /// A condvar needs to be pinned because it contains a [`struct list_head`] that is
>      /// self-referential, so it cannot be safely moved once it is initialised.
> +    ///
> +    /// [`struct list_head`]: srctree/include/linux/types.h

Hm, I wonder if we could figure out a way to make links point to
specific definitions in the C headers with # anchors. I'm not sure
what the intended platform to view these links is.

>      #[pin]
>      _pin: PhantomPinned,
>  }
> --
> 2.43.0
>
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 05/13] rust: str: use `NUL` instead of 0 in doc comments

[Trevor Gross]

On Tue, Jan 16, 2024 at 5:36 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Throughout the module, bytes with the value zero are referred to as
> `NUL` bytes. Adapt the only two outliers.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/str.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 0a8569594fc3..843ffeec9b3e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -149,13 +149,13 @@ pub const fn as_char_ptr(&self) -> *const core::ffi::c_char {
>          self.0.as_ptr() as _
>      }
>
> -    /// Convert the string to a byte slice without the trailing 0 byte.
> +    /// Convert the string to a byte slice without the trailing `NUL` byte.
>      #[inline]
>      pub fn as_bytes(&self) -> &[u8] {
>          &self.0[..self.len()]
>      }
>
> -    /// Convert the string to a byte slice containing the trailing 0 byte.
> +    /// Convert the string to a byte slice containing the trailing `NUL` byte.
>      #[inline]
>      pub const fn as_bytes_with_nul(&self) -> &[u8] {
>          &self.0
> --
> 2.43.0
>
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 06/13] rust: str: move SAFETY comment in front of unsafe block

[Trevor Gross]

On Tue, Jan 16, 2024 at 5:36 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> SAFETY comments should immediately precede the unsafe block they
> justify. Move assignment to `bar` past comment as it is safe.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  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 843ffeec9b3e..fec5c4314758 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -191,9 +191,9 @@ pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> {
>      /// ```
>      /// # use kernel::c_str;
>      /// # use kernel::str::CStr;
> +    /// let bar = c_str!("ツ");
>      /// // SAFETY: String literals are guaranteed to be valid UTF-8
>      /// // by the Rust compiler.
> -    /// let bar = c_str!("ツ");
>      /// assert_eq!(unsafe { bar.as_str_unchecked() }, "ツ");
>      /// ```
>      #[inline]
> --
> 2.43.0
>
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 07/13] rust: kernel: unify spelling of refcount in docs

[Trevor Gross]

On Tue, Jan 16, 2024 at 5:37 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Replace instances of 'ref-count[ed]' with 'refcount[ed]' to increase
> consistency within the Rust documentation. The latter form is used more
> widely in the rest of the kernel:
>
> ```console
> $ rg '(\*|//).*?\srefcount(|ed)[\s,.]' | wc -l
> 1605
> $ rg '(\*|//).*?\sref-count(|ed)[\s,.]' | wc -l
> 43
> ```
>
> (numbers are for Commit 052d534373b7)
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/sync/arc.rs | 8 ++++----
>  rust/kernel/task.rs     | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 77cdbcf7bd2e..6c46b1affca5 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -56,7 +56,7 @@
>  ///     b: u32,
>  /// }
>  ///
> -/// // Create a ref-counted instance of `Example`.
> +/// // Create a refcounted instance of `Example`.
>  /// let obj = Arc::try_new(Example { a: 10, b: 20 })?;
>  ///
>  /// // Get a new pointer to `obj` and increment the refcount.
> @@ -510,7 +510,7 @@ fn deref(&self) -> &Self::Target {
>  /// # test().unwrap();
>  /// ```
>  ///
> -/// In the following example we first allocate memory for a ref-counted `Example` but we don't
> +/// In the following example we first allocate memory for a refcounted `Example` but we don't
>  /// initialise it on allocation. We do initialise it later with a call to [`UniqueArc::write`],
>  /// followed by a conversion to `Arc<Example>`. This is particularly useful when allocation happens
>  /// in one context (e.g., sleepable) and initialisation in another (e.g., atomic):
> @@ -560,7 +560,7 @@ impl<T> UniqueArc<T> {
>      /// Tries to allocate a new [`UniqueArc`] instance.
>      pub fn try_new(value: T) -> Result<Self, AllocError> {
>          Ok(Self {
> -            // INVARIANT: The newly-created object has a ref-count of 1.
> +            // INVARIANT: The newly-created object has a refcount of 1.
>              inner: Arc::try_new(value)?,
>          })
>      }
> @@ -574,7 +574,7 @@ pub fn try_new_uninit() -> Result<UniqueArc<MaybeUninit<T>>, AllocError> {
>              data <- init::uninit::<T, AllocError>(),
>          }? AllocError))?;
>          Ok(UniqueArc {
> -            // INVARIANT: The newly-created object has a ref-count of 1.
> +            // INVARIANT: The newly-created object has a refcount of 1.
>              // SAFETY: The pointer from the `Box` is valid.
>              inner: unsafe { Arc::from_inner(Box::leak(inner).into()) },
>          })
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 9451932d5d86..818ac51b06b6 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -23,7 +23,7 @@ macro_rules! current {
>  ///
>  /// All instances are valid tasks created by the C portion of the kernel.
>  ///
> -/// Instances of this type are always ref-counted, that is, a call to `get_task_struct` ensures
> +/// Instances of this type are always refcounted, that is, a call to `get_task_struct` ensures
>  /// that the allocation remains valid at least until the matching call to `put_task_struct`.
>  ///
>  /// # Examples
> @@ -147,7 +147,7 @@ pub fn wake_up(&self) {
>      }
>  }
>
> -// SAFETY: The type invariants guarantee that `Task` is always ref-counted.
> +// SAFETY: The type invariants guarantee that `Task` is always refcounted.
>  unsafe impl crate::types::AlwaysRefCounted for Task {
>      fn inc_ref(&self) {
>          // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> --
> 2.43.0
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 03/13] rust: ioctl: end top level module docs with full stop

[Trevor Gross]

On Wed, Jan 17, 2024 at 7:32 PM Trevor Gross <tmgross@umich.edu> wrote:
>
> On Tue, Jan 16, 2024 at 11:05 AM Valentin Obst <kernel@valentinobst.de> wrote:
> >
> > Every other module ends its first line of documentation with a full
> > stop. Adapt the only outlier.
> >
> > Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> > ---
> >  rust/kernel/ioctl.rs | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/ioctl.rs b/rust/kernel/ioctl.rs
> > index 59050e5f5a5a..5987ad6d38a7 100644
> > --- a/rust/kernel/ioctl.rs
> > +++ b/rust/kernel/ioctl.rs
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >
> > -//! ioctl() number definitions
> > +//! ioctl() number definitions.
>
> Would you mind also adding backticks to `ioctl()` while you're here?
>
> - Trevor
>
> >  //!
> >  //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h)
> >
> > --
> > 2.43.0
> >

Nevermind, I see you got this in patch 8/13.

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 08/13] rust: kernel: mark code fragments in docs with backticks

[Trevor Gross]

On Tue, Jan 16, 2024 at 6:10 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Fix places where comments include code fragments that are not enclosed
> in backticks.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 09/13] rust: kernel: add blank lines in front of code blocks

[Trevor Gross]

On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Throughout the code base, blank lines are used before starting a code
> block. Adapt outliers to improve consistency within the kernel crate.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 12/13] rust: kernel: remove unneeded doclink targets

[Trevor Gross]

On Tue, Jan 16, 2024 at 7:16 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Remove explicit targets for doclinks in cases where rustdoc can
> determine the correct target by itself. The goal is to reduce verbosity
> in the source code.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/workqueue.rs | 10 ----------
>  1 file changed, 10 deletions(-)

Perhaps "reduce verbosity" -> "reduce unneeded verbosity" in the commit message?

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 13/13] rust: locked_by: shorten doclink preview

[Trevor Gross]

On Tue, Jan 16, 2024 at 7:17 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Increases readability by removing `super::` from the link preview
> text.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/sync/locked_by.rs | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
> index b17ee5cd98f3..22c38993bf63 100644
> --- a/rust/kernel/sync/locked_by.rs
> +++ b/rust/kernel/sync/locked_by.rs
> @@ -9,7 +9,7 @@
>  /// Allows access to some data to be serialised by a lock that does not wrap it.
>  ///
>  /// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
> -/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
> +/// [`Mutex`] or [`SpinLock`]. [`LockedBy`] is meant for cases when this is not
>  /// possible. For example, if a container has a lock and some data in the contained elements needs
>  /// to be protected by the same lock.
>  ///
> @@ -17,6 +17,9 @@
>  /// when the caller shows evidence that the 'external' lock is locked. It panics if the evidence
>  /// refers to the wrong instance of the lock.
>  ///
> +/// [`Mutex`]: super::Mutex
> +/// [`SpinLock`]: super::SpinLock
> +///
>  /// # Examples
>  ///
>  /// The following is an example for illustrative purposes: `InnerDirectory::bytes_used` is an
> --
> 2.43.0
>
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 12/13] rust: kernel: remove unneeded doclink targets

[Trevor Gross]

On Tue, Jan 16, 2024 at 7:16 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Remove explicit targets for doclinks in cases where rustdoc can
> determine the correct target by itself. The goal is to reduce verbosity
> in the source code.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/workqueue.rs | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index aedf47f258bd..f63190b563d8 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -426,13 +426,10 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
>  /// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
>  /// methods on this trait must have exactly the behavior that the definitions given below have.
>  ///
> -/// [`Work<T, ID>`]: Work
> [...]

Just for reference, this behavior is described at
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html#valid-links:

> You can also refer to items with generic parameters like Vec<T>. The
> link will resolve as if you had written [`Vec<T>`](Vec)

Re: [PATCH 10/13] rust: kernel: add doclinks

[Trevor Gross]

On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Add doclinks to existing documentation.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/sync/arc.rs  |  6 +++---
>  rust/kernel/sync/lock.rs | 13 +++++++++---
>  rust/kernel/workqueue.rs | 45 ++++++++++++++++++++++++----------------
>  3 files changed, 40 insertions(+), 24 deletions(-)
>
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 6c46b1affca5..936bc549a082 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -365,12 +365,12 @@ fn from(item: Pin<UniqueArc<T>>) -> Self {
>  /// A borrowed reference to an [`Arc`] instance.
>  ///
>  /// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler
> -/// to use just `&T`, which we can trivially get from an `Arc<T>` instance.
> +/// to use just `&T`, which we can trivially get from an [`Arc<T>`] instance.
>  ///
>  /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
>  /// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> -/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double
> -/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if
> +/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
> +/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
>  /// needed.

I think it is usually okay to only link the first usage in a section
or paragraph. No harm having more of course.

>  /// # Invariants
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 467249b39f71..f14179d19d4e 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -21,14 +21,21 @@
>  /// # Safety
>  ///
>  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
> -/// is owned, that is, between calls to `lock` and `unlock`.
> -/// - Implementers must also ensure that `relock` uses the same locking method as the original
> +/// is owned, that is, between calls to [`lock`] and [`unlock`].
> +/// - Implementers must also ensure that [`relock`] uses the same locking method as the original
>  /// lock operation.

The second lines of these list items should probably be indented
(doesn't have to be in this patch).

> [...]
> @@ -290,10 +294,11 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
>
>  /// Defines the method that should be called directly when a work item is executed.
>  ///
> -/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
> +/// This trait is implemented by `Pin<Box<T>>` and [`Arc<T>`], and is mainly intended to be

`Pin` could be linked too.

> [...]

Just a few nits for this one, nothing blocking.

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 01/13] rust: kernel: fix multiple typos in documentation

[Trevor Gross]

On Tue, Jan 16, 2024 at 11:06 AM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Fixes multiple trivial typos in documentation and comments of the
> kernel crate.
>
> allocator:
> - Fix a trivial list item alignment issue in the last SAFETY comment of
> `krealloc_aligned`.
>
> init:
> - Replace 'type' with 'trait' in the doc comments of the `PinInit` and
> `Init` traits.
> - Add colons before starting lists.
> - Add spaces between the type and equal sign to respect the code
> formatting rules in example code.
> - End a sentence with a full stop instead of a colon.
>
> ioctl:
> - Replace 'an' with 'a' where appropriate.
>
> str:
> - Replace 'Return' with 'Returns' in the doc comment of `bytes_written`
> as the text describes what the function does.
>
> sync/lock/spinlock:
> - The code in this module operates on spinlocks, not mutexes. Thus,
> replace 'mutex' with 'spinlock' in the SAFETY comment of `unlock`.
>
> workqueue:
> - Replace "wont" with "won't" in the doc comment of `__enqueue`.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/allocator.rs          |  2 +-
>  rust/kernel/init.rs               | 16 ++++++++--------
>  rust/kernel/ioctl.rs              |  4 ++--
>  rust/kernel/str.rs                |  2 +-
>  rust/kernel/sync/lock/spinlock.rs |  2 +-
>  rust/kernel/workqueue.rs          |  2 +-
>  6 files changed, 14 insertions(+), 14 deletions(-)

What an excellent commit message :)

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH 11/13] rust: kernel: add doclinks with html tags

[Trevor Gross]

On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <kernel@valentinobst.de> wrote:
>
> Add doclinks to existing documentation. Use html 'code' tags to add
> links to items that cannot be linked with the normal syntax.
>
> The use of html tags is a tradeoff between the readability of the
> documentation's source code and the ergonomics of the generated content.
>
> Signed-off-by: Valentin Obst <kernel@valentinobst.de>
> ---
>  rust/kernel/str.rs       |  7 ++++---
>  rust/kernel/sync/arc.rs  | 24 +++++++++++++-----------
>  rust/kernel/workqueue.rs | 10 +++++-----
>  3 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index fec5c4314758..f95fd2ba19fb 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -14,7 +14,8 @@
>
>  /// Byte string without UTF-8 validity guarantee.
>  ///
> -/// `BStr` is simply an alias to `[u8]`, but has a more evident semantical meaning.
> +/// `BStr` is simply an alias to <code>[[u8]]</code>, but has a more evident
> +/// semantical meaning.
>
> [...]
>
>  /// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>`
> -/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference)
> -/// to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates this double
> -/// indirection while still allowing one to increment the refcount and getting an [`Arc<T>`] when/if
> -/// needed.
> +/// over <code>&[`Arc<T>`]</code> because the latter results in a double-indirection: a pointer
> +/// (shared reference) to a pointer ([`Arc<T>`]) to the object (`T`). An [`ArcBorrow`] eliminates
> +/// this double indirection while still allowing one to increment the refcount and getting an
> +/// [`Arc<T>`] when/if needed.

Std sometimes does something like this, which links to the slice primitive.

    [`&[u8]`](prim@slice)

What exactly is going on with Arc, is it not getting linked correctly
when it has generics? I don't quite follow what <code> does.

I agree with Martin, we don't need to try too hard to link these types
- slices and numeric types are background knowledge, and it is easy
enough to search for the other types (Arc, Test) if the links don't
work for whatever reason.

If rustdoc just isn't making good choices in certain places or isn't
flexible enough, could you write issues in the Rust repo? Better to
get inconveniences fixed upstream if possible.

- Trevor

Re: [PATCH 02/13] rust: error: move unsafe block into function call

[Valentin Obst]

> If you're up for it, that safety comment could also be improved.
> Something like
>
>    // SAFETY: `pdev` points to a valid platform device

Thanks, will include something like that in a v2.

Just to make sure I got it correctly: Index is bounds checked [1] and
thus there is no need to include it in the comment. Please object if
that is wrong.

[1]: https://elixir.bootlin.com/linux/v6.7/source/drivers/base/platform.c#L63

Re: [PATCH 04/13] rust: kernel: add srctree-relative doclinks

[Valentin Obst]

> Hm, I wonder if we could figure out a way to make links point to
> specific definitions in the C headers with # anchors. I'm not sure
> what the intended platform to view these links is.

Currently the links simply open the plain .h file from your local tree in your
browser, i.e.,

```rust
/// [`struct wait_queue_head`]: srctree/include/linux/wait.h
```

becomes something like:

file:///mnt/build/rust-for-linux/rust4lx/include/linux/wait.h

and fragments won't work on that.

I agree that it would be nice to link to type definitions, variables,
functions, ect. in the file, maybe with something like:

```
#(type|var|func);<identifier>
```

However, I think this will require some parsing and embedding the C
source file into some html.

Re: [PATCH 10/13] rust: kernel: add doclinks

[Valentin Obst]

> >  /// # Invariants
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 467249b39f71..f14179d19d4e 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -21,14 +21,21 @@
> >  /// # Safety
> >  ///
> >  /// - Implementers must ensure that only one thread/CPU may access the protected data once the lock
> > -/// is owned, that is, between calls to `lock` and `unlock`.
> > -/// - Implementers must also ensure that `relock` uses the same locking method as the original
> > +/// is owned, that is, between calls to [`lock`] and [`unlock`].
> > +/// - Implementers must also ensure that [`relock`] uses the same locking method as the original
> >  /// lock operation.

> The second lines of these list items should probably be indented
> (doesn't have to be in this patch).

Indeed. Will include it in the first commit in v2.

> > [...]
> > @@ -290,10 +294,11 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput
> >
> >  /// Defines the method that should be called directly when a work item is executed.
> >  ///
> > -/// This trait is implemented by `Pin<Box<T>>` and `Arc<T>`, and is mainly intended to be
> > +/// This trait is implemented by `Pin<Box<T>>` and [`Arc<T>`], and is mainly intended to be

> `Pin` could be linked too.

This requires the 'code' tags in the next commit and is done by linking
both `Pin` and `Box<T>`:

```rust
/// This trait is implemented by <code>[Pin]<[`Box<T>`]></code> and [`Arc<T>`], and is mainly
```

Re: [PATCH 11/13] rust: kernel: add doclinks with html tags

[Valentin Obst]

> Std sometimes does something like this, which links to the slice primitive.
>
>     [`&[u8]`](prim@slice)

This would indeed link `&[u8]` to the slice type. But I agree, both,
linking to slice and to `u8` is not necessary as it is common knowledge.
However, if it is a slice over a more complicated/custom type it might
be worth linking to it, and in that case the 'code' tag syntax would be
the only option we have at the moment.

> What exactly is going on with Arc, is it not getting linked correctly
> when it has generics? I don't quite follow what <code> does.

In this case it is about the `&`:

<code>&[`Arc<T>`]</code>

Here, `&Arc<T>` is formatted as code, but only the `Arc<T>` is a
clickable link. While

[`&Arc<T>`]

results in:

```
/// over [&`Arc<T>`] because the latter results in a double-indirection: a pointer
    |           ^^^^^^^^ no item named `&Arc` in scope
```

using:

&[`Arc<T>`]

would result in a link, but `&` is not formatted as code. Finally,

[`&Arc<T>`](Arc)

would work but `&` is part of the clickable link now. Thus,
using the html tag here is the only way I found to get the
'cleanest' form in the rendered document.

> If rustdoc just isn't making good choices in certain places or isn't
> flexible enough, could you write issues in the Rust repo? Better to
> get inconveniences fixed upstream if possible.

I like the idea of finding a proper solution to that in rustdoc
instead of cluttering the source code with html tags. If nobody
strongly objects I'd drop the whole patch in v2 and open an issue
in the rust repo.

	- Valentin

Re: [PATCH 13/13] rust: locked_by: shorten doclink preview

[Alice Ryhl]

On Wed, Jan 17, 2024 at 1:16 AM Valentin Obst <kernel@valentinobst.de> wrote:
>  /// In most cases, data protected by a lock is wrapped by the appropriate lock type, e.g.,
> -/// [`super::Mutex`] or [`super::SpinLock`]. [`LockedBy`] is meant for cases when this is not
> +/// [`Mutex`] or [`SpinLock`]. [`LockedBy`] is meant for cases when this is not
>  /// possible. For example, if a container has a lock and some data in the contained elements needs
>  /// to be protected by the same lock.

It looks like the text should be reflowed here. The "possible" word
fits on the previous line.