[PATCH v2 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[PATCH v2 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Dirk Behme]

Add some missing intra doc linkage `[...]`.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
Changes in v2:
  * Split out this change into this extra patch (Miguel)

 rust/kernel/types.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index c3dc798221dbc..8ed802444c594 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -271,10 +271,10 @@ fn drop(&mut self) {
 
 /// Stores an opaque value.
 ///
-/// `Opaque<T>` is meant to be used with FFI objects that are never interpreted by Rust code.
+/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
 ///
 /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
-/// It gets rid of all the usual assumptions that Rust has for a value:
+/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
 ///
 /// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
 ///   [`bool`]).
@@ -286,7 +286,7 @@ fn drop(&mut self) {
 /// This has to be used for all values that the C side has access to, because it can't be ensured
 /// that the C side is adhering to the usual constraints that Rust needs.
 ///
-/// Using `Opaque<T>` allows to continue to use references on the Rust side even for values shared
+/// Using [`Opaque<T>`] allows to continue to use references on the Rust side even for values shared
 /// with C.
 ///
 /// # Examples
-- 
2.48.0

[PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Dirk Behme]

In the discussion [1] some `Opaque` documentation updates have
been proposed. In that discussion it was clarified that `Opaque`
is intended to be used for (partial) uninitialized or changing C
structs. And which consequences this has for using raw pointers
or the destruction. Improve the `Opaque` documentation by adding
these conclusions from that discussion.

Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
Changes in v2:
 * Split patch into two (Miguel)
 * Improve commit message and subject (Miguel)

 rust/kernel/types.rs | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 8ed802444c594..08ff4949d2a61 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -274,14 +274,20 @@ fn drop(&mut self) {
 /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
 ///
 /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
-/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
-///
-/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
-///   [`bool`]).
+/// This is useful for C structs that are not fully initialized (yet) or might change their
+/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
+/// Rust has for a value of type `T`:
+///
+/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
+///   `3` for a [`bool`]).
+/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
+///   valid *right now*.
 /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
 /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
 ///   the same value.
 /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
+/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
+///   be uninitialized, as described above.
 ///
 /// This has to be used for all values that the C side has access to, because it can't be ensured
 /// that the C side is adhering to the usual constraints that Rust needs.
-- 
2.48.0

Re: [PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Andreas Hindborg]

"Dirk Behme" <dirk.behme@de.bosch.com> writes:

> In the discussion [1] some `Opaque` documentation updates have
> been proposed. In that discussion it was clarified that `Opaque`
> is intended to be used for (partial) uninitialized or changing C
> structs. And which consequences this has for using raw pointers
> or the destruction. Improve the `Opaque` documentation by adding
> these conclusions from that discussion.
>
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> Changes in v2:
>  * Split patch into two (Miguel)
>  * Improve commit message and subject (Miguel)
>
>  rust/kernel/types.rs | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ed802444c594..08ff4949d2a61 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -274,14 +274,20 @@ fn drop(&mut self) {
>  /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
>  /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> -/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
> -///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// This is useful for C structs that are not fully initialized (yet) or might change their
> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> +/// Rust has for a value of type `T`:
> +///
> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> +///   `3` for a [`bool`]).
> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> +///   valid *right now*.
>  /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>  /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>  ///   the same value.
>  /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.

Thanks for clarifying the docs. What you write is correct, but I would
prefer a more terse and formal language, at least for the first
paragraph. Perhaps you can include the following in some form:

  > Opaque opts out of the following rust language invariants for the
  > contained `T`:
  > 
  > - Initialization invariant - the contained value is allowed to be
  >   uninitialized and contain invalid bit patterns.
  > - Immutability invariant - `Opaque` allows interior mutability.
  > - Uniqueness invariant - `Opaque` allows aliasing of shared references.
  > 
  > Further, `Opaque` is `!Unpin`.

And then after that paragraph, you could elaborate with examples and so forth?

Best regards,
Andreas Hindborg

Re: [PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Daniel Almeida]

Hi Andreas,

> On 19 Feb 2025, at 05:06, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
> 
>> In the discussion [1] some `Opaque` documentation updates have
>> been proposed. In that discussion it was clarified that `Opaque`
>> is intended to be used for (partial) uninitialized or changing C
>> structs. And which consequences this has for using raw pointers
>> or the destruction. Improve the `Opaque` documentation by adding
>> these conclusions from that discussion.
>> 
>> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>> Changes in v2:
>> * Split patch into two (Miguel)
>> * Improve commit message and subject (Miguel)
>> 
>> rust/kernel/types.rs | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 8ed802444c594..08ff4949d2a61 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -274,14 +274,20 @@ fn drop(&mut self) {
>> /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>> ///
>> /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
>> -/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
>> -///
>> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
>> -///   [`bool`]).
>> +/// This is useful for C structs that are not fully initialized (yet) or might change their
>> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
>> +/// Rust has for a value of type `T`:
>> +///
>> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
>> +///   `3` for a [`bool`]).
>> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
>> +///   valid *right now*.
>> /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>> /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>> ///   the same value.
>> /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
>> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
>> +///   be uninitialized, as described above.
> 
> Thanks for clarifying the docs. What you write is correct, but I would
> prefer a more terse and formal language, at least for the first
> paragraph. Perhaps you can include the following in some form:
> 
>> Opaque opts out of the following rust language invariants for the
>> contained `T`:
>> 
>> - Initialization invariant - the contained value is allowed to be
>>  uninitialized and contain invalid bit patterns.
>> - Immutability invariant - `Opaque` allows interior mutability.
>> - Uniqueness invariant - `Opaque` allows aliasing of shared references.
>> 
>> Further, `Opaque` is `!Unpin`.
> 
> And then after that paragraph, you could elaborate with examples and so forth?
> 
> Best regards,
> Andreas Hindborg

IMHO, your “terseness” suggestion goes against the very intent of this patch.

It takes something extremely clear, and places it behind some technical terms.

In as much as I agree with making things formal and professional, our first priority should
be making sure that the docs for this type are clear as day, so that people do not misuse it
to introduce bugs.

Note that this discussion sprung up because it’s hard to understand what exactly this type
does and when exactly it should be used. The code is conceptually simple, but things like


>> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
>> +///   be uninitialized, as described above.

are 100% a pitfall that should be warned against in bold letters and plain terms.


— Daniel

Re: [PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Andreas Hindborg]

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

> Hi Andreas,
>
>> On 19 Feb 2025, at 05:06, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Dirk Behme" <dirk.behme@de.bosch.com> writes:
>>
>>> In the discussion [1] some `Opaque` documentation updates have
>>> been proposed. In that discussion it was clarified that `Opaque`
>>> is intended to be used for (partial) uninitialized or changing C
>>> structs. And which consequences this has for using raw pointers
>>> or the destruction. Improve the `Opaque` documentation by adding
>>> these conclusions from that discussion.
>>>
>>> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> ---
>>> Changes in v2:
>>> * Split patch into two (Miguel)
>>> * Improve commit message and subject (Miguel)
>>>
>>> rust/kernel/types.rs | 14 ++++++++++----
>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>> index 8ed802444c594..08ff4949d2a61 100644
>>> --- a/rust/kernel/types.rs
>>> +++ b/rust/kernel/types.rs
>>> @@ -274,14 +274,20 @@ fn drop(&mut self) {
>>> /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>>> ///
>>> /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
>>> -/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
>>> -///
>>> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
>>> -///   [`bool`]).
>>> +/// This is useful for C structs that are not fully initialized (yet) or might change their
>>> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
>>> +/// Rust has for a value of type `T`:
>>> +///
>>> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
>>> +///   `3` for a [`bool`]).
>>> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
>>> +///   valid *right now*.
>>> /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
>>> /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
>>> ///   the same value.
>>> /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
>>> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
>>> +///   be uninitialized, as described above.
>>
>> Thanks for clarifying the docs. What you write is correct, but I would
>> prefer a more terse and formal language, at least for the first
>> paragraph. Perhaps you can include the following in some form:
>>
>>> Opaque opts out of the following rust language invariants for the
>>> contained `T`:
>>>
>>> - Initialization invariant - the contained value is allowed to be
>>>  uninitialized and contain invalid bit patterns.
>>> - Immutability invariant - `Opaque` allows interior mutability.
>>> - Uniqueness invariant - `Opaque` allows aliasing of shared references.
>>>
>>> Further, `Opaque` is `!Unpin`.
>>
>> And then after that paragraph, you could elaborate with examples and so forth?
>>
>> Best regards,
>> Andreas Hindborg
>
> IMHO, your “terseness” suggestion goes against the very intent of this patch.
>
> It takes something extremely clear, and places it behind some technical terms.
>
> In as much as I agree with making things formal and professional, our first priority should
> be making sure that the docs for this type are clear as day, so that people do not misuse it
> to introduce bugs.
>
> Note that this discussion sprung up because it’s hard to understand what exactly this type
> does and when exactly it should be used. The code is conceptually simple, but things like

Just to clarify, I do not suggest to yank the addition that this patch
provides. I think it is a valuable addition.

We need to strike a balance, which is why I suggested having the short,
to the point, more formal wording first - and then elaborate with the
more wordy, talkative, explanatory paragraph.

People who are very familiar with the rust language and/or relevant
invariants will immediately have the info they need after the first
"terse" paragraph.

People who need a bit more guidance can get that by reading further into
the docs.

>
>
>>> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
>>> +///   be uninitialized, as described above.
>
> are 100% a pitfall that should be warned against in bold letters and plain terms.
>

True. I would add it to the last line of my suggestion:

  Further, `Opaque` is `!Unpin` and will not run the drop method of the
  contained `T` when dropped.



Best regards,
Andreas Hindborg

Re: [PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Miguel Ojeda]

On Tue, Feb 25, 2025 at 11:36 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> People who are very familiar with the rust language and/or relevant
> invariants will immediately have the info they need after the first
> "terse" paragraph.

By the way, whether we do that or not, it could make sense to very
briefly say how that is done, e.g. "... (by using `UnsafeCell`) ..."
with a link to the relevant information. Experts already know how the
definition works, but it may help others connect the dots on what part
removes what restriction.

Cheers,
Miguel

Re: [PATCH v2 2/2] rust: types: `Opaque` doc: Add some destructor description

[Daniel Almeida]

Hi Dirk, thanks for working on this and sorry for the delay :)

Even if we ignore the “terse vs not terse” discussion momentarily, this patch doesn’t really
say when you should and shouldn’t use Opaque<T>.

> On 19 Feb 2025, at 02:55, Dirk Behme <Dirk.Behme@de.bosch.com> wrote:
> 
> In the discussion [1] some `Opaque` documentation updates have
> been proposed. In that discussion it was clarified that `Opaque`
> is intended to be used for (partial) uninitialized or changing C
> structs. And which consequences this has for using raw pointers
> or the destruction. Improve the `Opaque` documentation by adding
> these conclusions from that discussion.
> 
> Suggested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Link: https://lore.kernel.org/rust-for-linux/F8AB1160-F8CF-412F-8B88-4C79D65B53A1@collabora.com/ [1]
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> Changes in v2:
> * Split patch into two (Miguel)
> * Improve commit message and subject (Miguel)
> 
> rust/kernel/types.rs | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ed802444c594..08ff4949d2a61 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -274,14 +274,20 @@ fn drop(&mut self) {
> /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.

What does this mean, exactly?

> ///
> /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.

OK, can it be used for anything else? IOW: can we rewrite this as “It should only be used to wrap
structs from the C side, like for example `Opaque<bindings::mutex>`”

> -/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:
> -///
> -/// * The value is allowed to be uninitialized (for example have invalid bit patterns: `3` for a
> -///   [`bool`]).
> +/// This is useful for C structs that are not fully initialized (yet) or might change their
> +/// content from C side at runtime. [`Opaque<T>`] gets rid of all the usual assumptions that
> +/// Rust has for a value of type `T`:

Same here. Can we rewrite “This is useful for C structs…” as “This is meant to be used to…” ?

> +///
> +/// * The value is allowed to be uninitialized or invalid (for example have invalid bit patterns:
> +///   `3` for a [`bool`]).
> +/// * By dereferencing a raw pointer to the value you are unsafely asserting that the value is
> +///   valid *right now*.

Thanks, this is very helpful.

> /// * The value is allowed to be mutated, when a `&Opaque<T>` exists on the Rust side.
> /// * No uniqueness for mutable references: it is fine to have multiple `&mut Opaque<T>` point to
> ///   the same value.
> /// * The value is not allowed to be shared with other threads (i.e. it is `!Sync`).
> +/// * The destructor of [`Opaque<T>`] does *not* run the destructor of `T`, as `T` may
> +///   be uninitialized, as described above.

This is also very helpful.

> ///
> /// This has to be used for all values that the C side has access to, because it can't be ensured
> /// that the C side is adhering to the usual constraints that Rust needs.

Some clarification is needed here, i.e.: what constraints? I assume this is about aliasing, if so, can
this be said explicitly in the quote above?

> -- 
> 2.48.0
> 
> 

Re: [PATCH v2 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Andreas Hindborg]

"Dirk Behme" <dirk.behme@de.bosch.com> writes:

> Add some missing intra doc linkage `[...]`.
>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> Changes in v2:
>   * Split out this change into this extra patch (Miguel)
>
>  rust/kernel/types.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index c3dc798221dbc..8ed802444c594 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -271,10 +271,10 @@ fn drop(&mut self) {
>
>  /// Stores an opaque value.
>  ///
> -/// `Opaque<T>` is meant to be used with FFI objects that are never interpreted by Rust code.
> +/// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
>  /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
> -/// It gets rid of all the usual assumptions that Rust has for a value:
> +/// It gets rid of all the usual assumptions that Rust has for a value of type `T`:

You yank the effect of this change in patch 2, so maybe just leave it
till then?


Best regards,
Andreas Hindborg

Re: [PATCH v2 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Miguel Ojeda]

On Wed, Feb 19, 2025 at 9:06 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> You yank the effect of this change in patch 2, so maybe just leave it
> till then?

It is fine -- this could have been an independent patch.

Cheers,
Miguel

Re: [PATCH v2 1/2] rust: types: `Opaque` doc: Add some intra doc linkage

[Miguel Ojeda]

On Wed, Feb 19, 2025 at 11:54 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> It is fine -- this could have been an independent patch.

(Going through things again) The commit message does not seem to say
anything about this change, so it does seem like a mistake, sorry.

Cheers,
Miguel