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
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]>
---
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
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]>
---
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
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]>
---
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
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]>
---
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
Fix places where comments include code fragments that are not enclosed
in backticks.
Signed-off-by: Valentin Obst <[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 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
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]>
---
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
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 <[email protected]>
---
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
Add doclinks to existing documentation.
Signed-off-by: Valentin Obst <[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 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
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 <[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 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
Increases readability by removing `super::` from the link preview
text.
Signed-off-by: Valentin Obst <[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
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 <[email protected]>
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 <[email protected]>
> ---
> [...]
> @@ -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.
> [...]
> > [...]
> > @@ -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
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.
On Tue, Jan 16, 2024 at 5:35 PM Valentin Obst <[email protected]> wrote:
>
> 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]>
> ---
> 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 <[email protected]>
On Tue, Jan 16, 2024 at 5:36 PM Valentin Obst <[email protected]> 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 <[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
>
>
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 5:36 PM Valentin Obst <[email protected]> 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 <[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
>
>
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 5:37 PM Valentin Obst <[email protected]> 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 <[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
>
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 6:10 PM Valentin Obst <[email protected]> wrote:
>
> 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]>
On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <[email protected]> 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 <[email protected]>
> ---
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 7:16 PM Valentin Obst <[email protected]> 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 <[email protected]>
> ---
> rust/kernel/workqueue.rs | 10 ----------
> 1 file changed, 10 deletions(-)
Perhaps "reduce verbosity" -> "reduce unneeded verbosity" in the commit message?
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 7:16 PM Valentin Obst <[email protected]> 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 <[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 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)
On Tue, Jan 16, 2024 at 7:17 PM Valentin Obst <[email protected]> wrote:
>
> Increases readability by removing `super::` from the link preview
> text.
>
> Signed-off-by: Valentin Obst <[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
>
>
Reviewed-by: Trevor Gross <[email protected]>
On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <[email protected]> wrote:
>
> Add doclinks to existing documentation.
>
> Signed-off-by: Valentin Obst <[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.
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 <[email protected]>
On Tue, Jan 16, 2024 at 6:11 PM Valentin Obst <[email protected]> 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 <[email protected]>
> ---
> 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
> > /// # 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
```
> 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.
> 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
On Wed, Jan 17, 2024 at 1:16 AM Valentin Obst <[email protected]> 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.