2024-01-23 15:04:35

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 00/12] rust: kernel: documentation improvements

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,11,12),
- adding more intra-doc links as well as srctree-relative links to C
header files (commits #4,10).

---
Changes in v2:
- Drop commit "rust: kernel: add doclinks with html tags" in response to
review.
- Fix another list item alignment issue pointed out during review of v1.
Was added to commit "rust: kernel: fix multiple typos in
documentation".
- Commit "rust: error: move unsafe block into function call" is now
"rust: error: improve unsafe code in example" and also rewords the
SAFETY comment of the code example.
- Did not add 'Reviewed-By' tags offered in v1 tags due to changes.
- Link to v1: https://lore.kernel.org/lkml/[email protected]/
---
Valentin Obst (12):
rust: kernel: fix multiple typos in documentation
rust: error: improve unsafe code in example
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: remove unneeded doclink targets
rust: locked_by: shorten doclink preview

rust/kernel/allocator.rs | 2 +-
rust/kernel/error.rs | 10 ++---
rust/kernel/init.rs | 16 ++++----
rust/kernel/ioctl.rs | 6 +--
rust/kernel/lib.rs | 2 +-
rust/kernel/str.rs | 8 ++--
rust/kernel/sync/arc.rs | 14 +++----
rust/kernel/sync/condvar.rs | 2 +
rust/kernel/sync/lock.rs | 15 ++++++--
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 | 64 +++++++++++++++----------------
14 files changed, 83 insertions(+), 72 deletions(-)

--
2.43.0



2024-01-23 15:05:11

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 05/12] rust: str: use `NUL` instead of 0 in doc comments

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

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


2024-01-23 15:05:34

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 04/12] rust: kernel: add srctree-relative doclinks

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

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


2024-01-23 15:07:06

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 02/12] rust: error: improve unsafe code in example

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

Reword the SAFETY comment to provide a better justification of why the
FFI call is safe.

Signed-off-by: Valentin Obst <[email protected]>
---
rust/kernel/error.rs | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 4f0c1edd63b7..4786d3ee1e92 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -264,13 +264,9 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
/// pdev: &mut PlatformDevice,
/// index: u32,
/// ) -> Result<*mut core::ffi::c_void> {
-/// // SAFETY: FFI call.
-/// unsafe {
-/// from_err_ptr(bindings::devm_platform_ioremap_resource(
-/// pdev.to_ptr(),
-/// index,
-/// ))
-/// }
+/// // SAFETY: `pdev` points to a valid platform device. There are no safety requirements
+/// // on `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


2024-01-23 15:09:31

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 01/12] rust: kernel: fix multiple typos in documentation

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:
- Fix a trivial list item alignment issue in the Safety section of the
`Backend` trait's description.

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 <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
rust/kernel/allocator.rs | 2 +-
rust/kernel/init.rs | 16 ++++++++--------
rust/kernel/ioctl.rs | 4 ++--
rust/kernel/str.rs | 2 +-
rust/kernel/sync/lock.rs | 4 ++--
rust/kernel/sync/lock/spinlock.rs | 2 +-
rust/kernel/workqueue.rs | 2 +-
7 files changed, 16 insertions(+), 16 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.rs b/rust/kernel/sync/lock.rs
index f12a684bc957..87e111c0da17 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -21,9 +21,9 @@
/// # 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`.
+/// 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 operation.
pub unsafe trait Backend {
/// The state required by the lock.
type State;
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


2024-01-23 16:08:05

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 06/12] rust: str: move SAFETY comment in front of unsafe block

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 <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


2024-01-23 16:08:28

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 07/12] rust: kernel: unify spelling of refcount in docs

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 <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


2024-01-23 16:11:34

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 08/12] rust: kernel: mark code fragments in docs with backticks

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

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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 87e111c0da17..67588654c22f 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


2024-01-23 16:11:48

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 09/12] rust: kernel: add blank lines in front of code blocks

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 <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


2024-01-23 16:11:57

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 10/12] rust: kernel: add doclinks

Add doclinks to existing documentation.

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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 67588654c22f..956a0618ecc6 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


2024-01-23 16:11:59

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 11/12] rust: kernel: remove unneeded doclink targets

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

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
rust/kernel/workqueue.rs | 10 ----------
1 file changed, 10 deletions(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index ed3af3491b47..73d6fa544ca6 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


2024-01-23 16:12:13

by Valentin Obst

[permalink] [raw]
Subject: [PATCH v2 12/12] rust: locked_by: shorten doclink preview

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

Signed-off-by: Valentin Obst <[email protected]>
Reviewed-by: Trevor Gross <[email protected]>
Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
---
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


Subject: Re: [PATCH v2 02/12] rust: error: improve unsafe code in example

On 1/23/24 12:00, Valentin Obst wrote:
> The `from_err_ptr` function is safe. There is no need for the call to it
> to be inside the unsafe block.
>
> Reword the SAFETY comment to provide a better justification of why the
> FFI call is safe.
>
> Signed-off-by: Valentin Obst <[email protected]>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <[email protected]>

2024-01-30 09:28:36

by Alice Ryhl

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] rust: kernel: documentation improvements

Valentin Obst <[email protected]> writes:
> 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,11,12),
> - adding more intra-doc links as well as srctree-relative links to C
> header files (commits #4,10).

I left one comment [1] on the last patch. With that fixed, you may add:

Reviewed-by: Alice Ryhl <[email protected]>

to all of the patches.

(I responded on v1 by accident, but it was v2 that I have reviewed.)

[1]: https://lore.kernel.org/rust-for-linux/CAH5fLghSaorRgDDuqNCN-BhQ86ysX96b=nKM_cZAN0_E6Ai04A@mail.gmail.com/

2024-01-30 10:47:36

by Valentin Obst

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] rust: kernel: documentation improvements

> I left one comment [1] on the last patch. With that fixed, you may add:
>
> Reviewed-by: Alice Ryhl <[email protected]>
>
> to all of the patches.

Thanks, I'll fix that, rebase with `rust-next` and send out a v3.

- Valentin

>
> (I responded on v1 by accident, but it was v2 that I have reviewed.)
>
> [1]: https://lore.kernel.org/rust-for-linux/CAH5fLghSaorRgDDuqNCN-BhQ86ysX96b=nKM_cZAN0_E6Ai04A@mail.gmail.com/