`UnsafePinned<T>` is useful for cases where a value might be shared with
C code but not directly used by it. In particular this is added for
storing additional data in the `MiscDeviceRegistration` which will be
shared between `fops->open` and the containing struct.

Similar to `Opaque` but guarantees that the value is always initialized
and that the inner value is dropped when `UnsafePinned` is dropped.

This was originally proposed for the IRQ abstractions [0] and is also
useful for other where the inner data may be aliased, but is always
valid and automatic `Drop` is desired.

Since then the `UnsafePinned` type was added to upstream Rust [1] by Sky
as a unstable feature, therefore this patch implements the subset of the
upstream API for the `UnsafePinned` type required for additional data in
`MiscDeviceRegistration` and in the implementation of the `Opaque` type.

Some differences to the upstream type definition are required in the
kernel implementation, because upstream type uses some compiler changes
to opt out of certain optimizations, this is documented in the
documentation and a comment on the `UnsafePinned` type.

The documentation on is based on the upstream rust documentation with
minor modifications for the kernel implementation.

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
Link: https://github.com/rust-lang/rust/pull/137043 [1]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Co-developed-by: Sky <sky@sky9.dev>
Signed-off-by: Sky <sky@sky9.dev>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/types.rs               |   6 ++
 rust/kernel/types/unsafe_pinned.rs | 111 +++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 9d0471afc9648f2973235488b441eb109069adb1..705f420fdfbc4a576de1c4546578f2f04cdf615e 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -253,6 +253,9 @@ fn drop(&mut self) {
 ///
 /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
 ///
+/// In cases where the contained data is only used by Rust, is not allowed to be
+/// uninitialized and automatic [`Drop`] is desired [`UnsafePinned`] should be used instead.
+///
 /// 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:
 ///
@@ -578,3 +581,6 @@ pub enum Either<L, R> {
 /// [`NotThreadSafe`]: type@NotThreadSafe
 #[allow(non_upper_case_globals)]
 pub const NotThreadSafe: NotThreadSafe = PhantomData;
+
+mod unsafe_pinned;
+pub use unsafe_pinned::UnsafePinned;
diff --git a/rust/kernel/types/unsafe_pinned.rs b/rust/kernel/types/unsafe_pinned.rs
new file mode 100644
index 0000000000000000000000000000000000000000..315248cb88c089239bd672c889b5107060175ec3
--- /dev/null
+++ b/rust/kernel/types/unsafe_pinned.rs
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: Apache-2.0 OR MIT
+
+//! The contents of this file partially come from the Rust standard library, hosted in
+//! the <https://github.com/rust-lang/rust> repository, licensed under
+//! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
+//! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
+//!
+//! This file provides a implementation / polyfill of a subset of the upstream
+//! rust `UnsafePinned` type. For details on the difference to the upstream
+//! implementation see the comment on the [`UnsafePinned`] struct definition.
+
+use core::{cell::UnsafeCell, marker::PhantomPinned};
+use pin_init::{cast_pin_init, PinInit, Wrapper};
+
+/// This type provides a way to opt-out of typical aliasing rules;
+/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
+///
+/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
+/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
+/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
+/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
+/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
+/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
+///
+/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
+/// the public API of a library. It is an internal implementation detail of libraries that need to
+/// support aliasing mutable references.
+///
+/// Further note that this does *not* lift the requirement that shared references must be read-only!
+/// Use [`UnsafeCell`] for that.
+///
+/// This type blocks niches the same way [`UnsafeCell`] does.
+///
+/// # Kernel implementation notes
+///
+/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
+/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
+/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
+/// to future rust versions only this polyfill of this type should be used when this behavior is
+/// required.
+//
+// As opposed to the upstream Rust type this contains a `PhantomPinned` and `UnsafeCell<T>`
+// - `PhantomPinned` to ensure the struct always is `!Unpin` and thus enables the `!Unpin` hack.
+//   This causes the LLVM `noalias` and `dereferenceable` attributes to be removed from
+//   `&mut !Unpin` types.
+// - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
+//   the upstream version however currently does not. This will most likely change in the future
+//   but for now we don't expose this in the documentation, since adding the guarantee is simpler
+//   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
+//   must not be relied on (Other than the niche blocking).
+//   See this Rust tracking issue: https://github.com/rust-lang/rust/issues/137750
+#[repr(transparent)]
+pub struct UnsafePinned<T: ?Sized> {
+    _ph: PhantomPinned,
+    value: UnsafeCell<T>,
+}
+
+impl<T> UnsafePinned<T> {
+    /// Constructs a new instance of [`UnsafePinned`] which will wrap the specified value.
+    ///
+    /// All access to the inner value through `&UnsafePinned<T>` or `&mut UnsafePinned<T>` or
+    /// `Pin<&mut UnsafePinned<T>>` requires `unsafe` code.
+    #[inline(always)]
+    #[must_use]
+    pub const fn new(value: T) -> Self {
+        UnsafePinned {
+            value: UnsafeCell::new(value),
+            _ph: PhantomPinned,
+        }
+    }
+}
+impl<T: ?Sized> UnsafePinned<T> {
+    /// Get read-only access to the contents of a shared `UnsafePinned`.
+    ///
+    /// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
+    /// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
+    /// [`UnsafeCell`] if you also need interior mutability.
+    ///
+    /// [`UnsafeCell`]: core::cell::UnsafeCell
+    ///
+    /// ```rust,no_run
+    /// use kernel::types::UnsafePinned;
+    ///
+    /// unsafe {
+    ///     let mut x = UnsafePinned::new(0);
+    ///     let ptr = x.get(); // read-only pointer, assumes immutability
+    ///     # // Upstream Rust uses `UnsafePinned::get_mut_unchecked` here.
+    ///     UnsafePinned::raw_get_mut(&raw mut x).write(1);
+    ///     ptr.read(); // UB!
+    /// }
+    /// ```
+    #[inline(always)]
+    #[must_use]
+    pub const fn get(&self) -> *const T {
+        self.value.get()
+    }
+
+    /// Gets a mutable pointer to the wrapped value.
+    #[inline(always)]
+    #[must_use]
+    pub const fn raw_get_mut(this: *mut Self) -> *mut T {
+        this as *mut T
+    }
+}
+
+impl<T> Wrapper<T> for UnsafePinned<T> {
+    fn pin_init<E>(init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        // SAFETY: `UnsafePinned<T>` has a compatible layout to `T`.
+        unsafe { cast_pin_init(init) }
+    }
+}

-- 
2.49.0

On 11.05.25 8:21 PM, Christian Schrefl wrote:
> `UnsafePinned<T>` is useful for cases where a value might be shared with
> C code but not directly used by it. In particular this is added for
> storing additional data in the `MiscDeviceRegistration` which will be
> shared between `fops->open` and the containing struct.
> 
> Similar to `Opaque` but guarantees that the value is always initialized
> and that the inner value is dropped when `UnsafePinned` is dropped.
> 
> This was originally proposed for the IRQ abstractions [0] and is also
> useful for other where the inner data may be aliased, but is always
> valid and automatic `Drop` is desired.
> 
> Since then the `UnsafePinned` type was added to upstream Rust [1] by Sky
> as a unstable feature, therefore this patch implements the subset of the
> upstream API for the `UnsafePinned` type required for additional data in
> `MiscDeviceRegistration` and in the implementation of the `Opaque` type.
> 
> Some differences to the upstream type definition are required in the
> kernel implementation, because upstream type uses some compiler changes
> to opt out of certain optimizations, this is documented in the
> documentation and a comment on the `UnsafePinned` type.
> 
> The documentation on is based on the upstream rust documentation with
> minor modifications for the kernel implementation.
> 
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
> Link: https://github.com/rust-lang/rust/pull/137043 [1]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
>  rust/kernel/types.rs               |   6 ++
>  rust/kernel/types/unsafe_pinned.rs | 111 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 9d0471afc9648f2973235488b441eb109069adb1..705f420fdfbc4a576de1c4546578f2f04cdf615e 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -253,6 +253,9 @@ fn drop(&mut self) {
>  ///
>  /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
> +/// In cases where the contained data is only used by Rust, is not allowed to be
> +/// uninitialized and automatic [`Drop`] is desired [`UnsafePinned`] should be used instead.
> +///
>  /// 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:
>  ///
> @@ -578,3 +581,6 @@ pub enum Either<L, R> {
>  /// [`NotThreadSafe`]: type@NotThreadSafe
>  #[allow(non_upper_case_globals)]
>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
> +
> +mod unsafe_pinned;
> +pub use unsafe_pinned::UnsafePinned;
> diff --git a/rust/kernel/types/unsafe_pinned.rs b/rust/kernel/types/unsafe_pinned.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..315248cb88c089239bd672c889b5107060175ec3
> --- /dev/null
> +++ b/rust/kernel/types/unsafe_pinned.rs
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: Apache-2.0 OR MIT
> +
> +//! The contents of this file partially come from the Rust standard library, hosted in
> +//! the <https://github.com/rust-lang/rust> repository, licensed under
> +//! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
> +//! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
> +//!
> +//! This file provides a implementation / polyfill of a subset of the upstream
> +//! rust `UnsafePinned` type. For details on the difference to the upstream
> +//! implementation see the comment on the [`UnsafePinned`] struct definition.
> +
> +use core::{cell::UnsafeCell, marker::PhantomPinned};
> +use pin_init::{cast_pin_init, PinInit, Wrapper};
> +
> +/// This type provides a way to opt-out of typical aliasing rules;
> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
> +///
> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
> +///
> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
> +/// the public API of a library. It is an internal implementation detail of libraries that need to
> +/// support aliasing mutable references.
> +///
> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
> +/// Use [`UnsafeCell`] for that.

The upstream rust PR [0] that changes this was just merged. So now `UnsafePinned` includes
`UnsafeCell` semantics. It's probably best to also change this in the kernel docs.
Though it's still the case that removing the guarantee is simpler than adding it back later,
so let me know what you all think.

[0]: https://github.com/rust-lang/rust/pull/140638

> +///
> +/// This type blocks niches the same way [`UnsafeCell`] does.
> +///
> +/// # Kernel implementation notes
> +///
> +/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
> +/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
> +/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
> +/// to future rust versions only this polyfill of this type should be used when this behavior is
> +/// required.
> +//
> +// As opposed to the upstream Rust type this contains a `PhantomPinned` and `UnsafeCell<T>`
> +// - `PhantomPinned` to ensure the struct always is `!Unpin` and thus enables the `!Unpin` hack.
> +//   This causes the LLVM `noalias` and `dereferenceable` attributes to be removed from
> +//   `&mut !Unpin` types.
> +// - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
> +//   the upstream version however currently does not. This will most likely change in the future
> +//   but for now we don't expose this in the documentation, since adding the guarantee is simpler
> +//   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
> +//   must not be relied on (Other than the niche blocking).
> +//   See this Rust tracking issue: https://github.com/rust-lang/rust/issues/137750
> +#[repr(transparent)]
> +pub struct UnsafePinned<T: ?Sized> {
> +    _ph: PhantomPinned,
> +    value: UnsafeCell<T>,
> +}
> +
> +impl<T> UnsafePinned<T> {
> +    /// Constructs a new instance of [`UnsafePinned`] which will wrap the specified value.
> +    ///
> +    /// All access to the inner value through `&UnsafePinned<T>` or `&mut UnsafePinned<T>` or
> +    /// `Pin<&mut UnsafePinned<T>>` requires `unsafe` code.
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn new(value: T) -> Self {
> +        UnsafePinned {
> +            value: UnsafeCell::new(value),
> +            _ph: PhantomPinned,
> +        }
> +    }
> +}
> +impl<T: ?Sized> UnsafePinned<T> {
> +    /// Get read-only access to the contents of a shared `UnsafePinned`.
> +    ///
> +    /// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
> +    /// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
> +    /// [`UnsafeCell`] if you also need interior mutability.
> +    ///
> +    /// [`UnsafeCell`]: core::cell::UnsafeCell
> +    ///
> +    /// ```rust,no_run
> +    /// use kernel::types::UnsafePinned;
> +    ///
> +    /// unsafe {
> +    ///     let mut x = UnsafePinned::new(0);
> +    ///     let ptr = x.get(); // read-only pointer, assumes immutability
> +    ///     # // Upstream Rust uses `UnsafePinned::get_mut_unchecked` here.
> +    ///     UnsafePinned::raw_get_mut(&raw mut x).write(1);
> +    ///     ptr.read(); // UB!
> +    /// }
> +    /// ```
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn get(&self) -> *const T {
> +        self.value.get()
> +    }
> +
> +    /// Gets a mutable pointer to the wrapped value.
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn raw_get_mut(this: *mut Self) -> *mut T {
> +        this as *mut T
> +    }
> +}
> +
> +impl<T> Wrapper<T> for UnsafePinned<T> {
> +    fn pin_init<E>(init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        // SAFETY: `UnsafePinned<T>` has a compatible layout to `T`.
> +        unsafe { cast_pin_init(init) }
> +    }
> +}
> 

On Thu Jun 5, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
> On 11.05.25 8:21 PM, Christian Schrefl wrote:
>> +/// This type provides a way to opt-out of typical aliasing rules;
>> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
>> +///
>> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
>> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
>> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
>> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
>> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
>> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
>> +///
>> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
>> +/// the public API of a library. It is an internal implementation detail of libraries that need to
>> +/// support aliasing mutable references.
>> +///
>> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
>> +/// Use [`UnsafeCell`] for that.
>
> The upstream rust PR [0] that changes this was just merged. So now `UnsafePinned` includes
> `UnsafeCell` semantics. It's probably best to also change this in the kernel docs.
> Though it's still the case that removing the guarantee is simpler than adding it back later,
> so let me know what you all think.

Depends on how "stable" this decision is. I haven't followed the
discussion, but given that this once changed to the "non-backwards"
compatible case it feels permanent.

How close is it to stabilization?

If it's close-ish, then I'd suggest we change this to reflect the new
semantics. If not, then we should leave it as-is.

---
Cheers,
Benno

> [0]: https://github.com/rust-lang/rust/pull/140638

On 05.06.25 7:30 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
>> On 11.05.25 8:21 PM, Christian Schrefl wrote:
>>> +/// This type provides a way to opt-out of typical aliasing rules;
>>> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
>>> +///
>>> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
>>> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
>>> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
>>> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
>>> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
>>> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
>>> +///
>>> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
>>> +/// the public API of a library. It is an internal implementation detail of libraries that need to
>>> +/// support aliasing mutable references.
>>> +///
>>> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
>>> +/// Use [`UnsafeCell`] for that.
>>
>> The upstream rust PR [0] that changes this was just merged. So now `UnsafePinned` includes
>> `UnsafeCell` semantics. It's probably best to also change this in the kernel docs.
>> Though it's still the case that removing the guarantee is simpler than adding it back later,
>> so let me know what you all think.
> 
> Depends on how "stable" this decision is. I haven't followed the
> discussion, but given that this once changed to the "non-backwards"
> compatible case it feels permanent.

It seems pretty permanent, from what I understand its hard to
define the exact semantics `UnsafePinned` without `UnsafeCell`
in a way that is sound and because of some interactions with
`Pin::deref` it would have some backwards compatibility issues.
See this comment by Ralf on github [1].

[1]: https://github.com/rust-lang/rust/pull/137043#discussion_r1973978597

> 
> How close is it to stabilization?
> 
> If it's close-ish, then I'd suggest we change this to reflect the new
> semantics. If not, then we should leave it as-is.

It's pretty new, I'm not sure how long it's going to stay in nightly,
but it's probably going to be quite some time.

I wouldn't change it if it would already be in the kernel, but I think
its probably good to add the current state of the feature. This
would also reduce the difference between docs and implementation.

Cheers
Christian

On Thu Jun 5, 2025 at 7:57 PM CEST, Christian Schrefl wrote:
> On 05.06.25 7:30 PM, Benno Lossin wrote:
>> On Thu Jun 5, 2025 at 7:03 PM CEST, Christian Schrefl wrote:
>>> On 11.05.25 8:21 PM, Christian Schrefl wrote:
>>>> +/// This type provides a way to opt-out of typical aliasing rules;
>>>> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
>>>> +///
>>>> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
>>>> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
>>>> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
>>>> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
>>>> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
>>>> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
>>>> +///
>>>> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
>>>> +/// the public API of a library. It is an internal implementation detail of libraries that need to
>>>> +/// support aliasing mutable references.
>>>> +///
>>>> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
>>>> +/// Use [`UnsafeCell`] for that.
>>>
>>> The upstream rust PR [0] that changes this was just merged. So now `UnsafePinned` includes
>>> `UnsafeCell` semantics. It's probably best to also change this in the kernel docs.
>>> Though it's still the case that removing the guarantee is simpler than adding it back later,
>>> so let me know what you all think.
>> 
>> Depends on how "stable" this decision is. I haven't followed the
>> discussion, but given that this once changed to the "non-backwards"
>> compatible case it feels permanent.
>
> It seems pretty permanent, from what I understand its hard to
> define the exact semantics `UnsafePinned` without `UnsafeCell`
> in a way that is sound and because of some interactions with
> `Pin::deref` it would have some backwards compatibility issues.
> See this comment by Ralf on github [1].

Oh yeah that seems like a done deal.

> [1]: https://github.com/rust-lang/rust/pull/137043#discussion_r1973978597
>
>> 
>> How close is it to stabilization?
>> 
>> If it's close-ish, then I'd suggest we change this to reflect the new
>> semantics. If not, then we should leave it as-is.
>
> It's pretty new, I'm not sure how long it's going to stay in nightly,
> but it's probably going to be quite some time.
>
> I wouldn't change it if it would already be in the kernel, but I think
> its probably good to add the current state of the feature. This
> would also reduce the difference between docs and implementation.

Makes sense, I agree with changing it. Thanks for your work!

---
Cheers,
Benno

On Thu, Jun 5, 2025 at 7:03 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> The upstream rust PR [0] that changes this was just merged. So now `UnsafePinned` includes
> `UnsafeCell` semantics. It's probably best to also change this in the kernel docs.
> Though it's still the case that removing the guarantee is simpler than adding it back later,
> so let me know what you all think.

Since upstream's will imply `UnsafeCell`, then I assume they will not
take it away, and thus we should just document it the same way, so
that eventually we can just alias the upstream one.

But that last part can only happen in a long time, when our minimum
upgrades past 1.89, since otherwise we would lose the `UnsafeCell`
with an alias.

If we really wanted a type that does not do that, then we could have
another one, with a different name.

Thanks!

(By the way, please try to trim unneeded quotes in replies; otherwise,
threads become harder to read in clients such as lore.kernel.org, and
it also becomes harder to reply)

Cheers,
Miguel

On Sun, May 11, 2025 at 8:21 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Signed-off-by: Sky <sky@sky9.dev>

Apologies for not noticing this earlier...

Since this is a Signed-off-by, the DCO applies, and it requires that
the name is a "known identity":

    https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

Sky: is that name one you use to sign paperwork etc.? If so, that is
fine (and apologies in that case!) -- please let me know. If not,
please feel free to ping me in private if needed.

Thanks!

Cheers,
Miguel

On 20.05.25 11:26 PM, Miguel Ojeda wrote:
> On Sun, May 11, 2025 at 8:21 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> Signed-off-by: Sky <sky@sky9.dev>
> 
> Apologies for not noticing this earlier...
> 
> Since this is a Signed-off-by, the DCO applies, and it requires that
> the name is a "known identity":
> 
>     https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
> 
> Sky: is that name one you use to sign paperwork etc.? If so, that is
> fine (and apologies in that case!) -- please let me know. If not,
> please feel free to ping me in private if needed.

Since it seems like Sky has not responded for 10 days
is should be fine to just drop their COB & SOB.

I only offered to add it since the upstream implementation
that this is based on was entirely done by them.

If you want to wait for some more time that's fine as well.

Cheers
Christian

On Fri, May 30, 2025 at 10:22 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Since it seems like Sky has not responded for 10 days
> is should be fine to just drop their COB & SOB.
>
> I only offered to add it since the upstream implementation
> that this is based on was entirely done by them.
>
> If you want to wait for some more time that's fine as well.

So Sky and I shared a couple emails in private a week ago, but I am
waiting on a reply. There is no rush, since this is not going into
this merge window, so it is fine.

As for the tags -- it is up to you both; just please make sure it is
fair and that the DCO is respected. You can also consider another tag
that is not a SoB, such as Inspired-by, if that is more fitting.

Thanks!

Cheers,
Miguel

On 30.05.25 10:22 PM, Christian Schrefl wrote:
> On 20.05.25 11:26 PM, Miguel Ojeda wrote:
>> On Sun, May 11, 2025 at 8:21 PM Christian Schrefl
>> <chrisi.schrefl@gmail.com> wrote:
>>>
>>> Signed-off-by: Sky <sky@sky9.dev>
>>
>> Apologies for not noticing this earlier...
>>
>> Since this is a Signed-off-by, the DCO applies, and it requires that
>> the name is a "known identity":
>>
>>     https://docs.kernel.org/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
>>
>> Sky: is that name one you use to sign paperwork etc.? If so, that is
>> fine (and apologies in that case!) -- please let me know. If not,
>> please feel free to ping me in private if needed.
> 
> Since it seems like Sky has not responded for 10 days
> is should be fine to just drop their COB & SOB.
> 
> I only offered to add it since the upstream implementation
> that this is based on was entirely done by them.
> 
> If you want to wait for some more time that's fine as well.

It might also make sense to wait for this PR [0]
and use the new code as the basis for the kernel
implementation, since that will change `UnsafePinned`
to include `UnsafeCell` semantics.

[0]: https://github.com/rust-lang/rust/pull/140638

> 
> Cheers
> Christian

On Sun May 11, 2025 at 8:21 PM CEST, Christian Schrefl wrote:
> `UnsafePinned<T>` is useful for cases where a value might be shared with
> C code but not directly used by it. In particular this is added for
> storing additional data in the `MiscDeviceRegistration` which will be
> shared between `fops->open` and the containing struct.
>
> Similar to `Opaque` but guarantees that the value is always initialized
> and that the inner value is dropped when `UnsafePinned` is dropped.
>
> This was originally proposed for the IRQ abstractions [0] and is also
> useful for other where the inner data may be aliased, but is always
> valid and automatic `Drop` is desired.
>
> Since then the `UnsafePinned` type was added to upstream Rust [1] by Sky
> as a unstable feature, therefore this patch implements the subset of the
> upstream API for the `UnsafePinned` type required for additional data in
> `MiscDeviceRegistration` and in the implementation of the `Opaque` type.
>
> Some differences to the upstream type definition are required in the
> kernel implementation, because upstream type uses some compiler changes
> to opt out of certain optimizations, this is documented in the
> documentation and a comment on the `UnsafePinned` type.
>
> The documentation on is based on the upstream rust documentation with
> minor modifications for the kernel implementation.
>
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
> Link: https://github.com/rust-lang/rust/pull/137043 [1]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

One nit below, with that fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  rust/kernel/types.rs               |   6 ++
>  rust/kernel/types/unsafe_pinned.rs | 111 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 9d0471afc9648f2973235488b441eb109069adb1..705f420fdfbc4a576de1c4546578f2f04cdf615e 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -578,3 +581,6 @@ pub enum Either<L, R> {
>  /// [`NotThreadSafe`]: type@NotThreadSafe
>  #[allow(non_upper_case_globals)]
>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
> +
> +mod unsafe_pinned;
> +pub use unsafe_pinned::UnsafePinned;

I would put `mod` to the top of the 

---
Cheers,
Benno

Hi Benno,

On 13.05.25 10:51 PM, Benno Lossin wrote:
> On Sun May 11, 2025 at 8:21 PM CEST, Christian Schrefl wrote:
>> `UnsafePinned<T>` is useful for cases where a value might be shared with
>> C code but not directly used by it. In particular this is added for
>> storing additional data in the `MiscDeviceRegistration` which will be
>> shared between `fops->open` and the containing struct.
>>
>> Similar to `Opaque` but guarantees that the value is always initialized
>> and that the inner value is dropped when `UnsafePinned` is dropped.
>>
>> This was originally proposed for the IRQ abstractions [0] and is also
>> useful for other where the inner data may be aliased, but is always
>> valid and automatic `Drop` is desired.
>>
>> Since then the `UnsafePinned` type was added to upstream Rust [1] by Sky
>> as a unstable feature, therefore this patch implements the subset of the
>> upstream API for the `UnsafePinned` type required for additional data in
>> `MiscDeviceRegistration` and in the implementation of the `Opaque` type.
>>
>> Some differences to the upstream type definition are required in the
>> kernel implementation, because upstream type uses some compiler changes
>> to opt out of certain optimizations, this is documented in the
>> documentation and a comment on the `UnsafePinned` type.
>>
>> The documentation on is based on the upstream rust documentation with
>> minor modifications for the kernel implementation.
>>
>> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
>> Link: https://github.com/rust-lang/rust/pull/137043 [1]
>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Co-developed-by: Sky <sky@sky9.dev>
>> Signed-off-by: Sky <sky@sky9.dev>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
> One nit below, with that fixed:
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> 
>> ---
>>  rust/kernel/types.rs               |   6 ++
>>  rust/kernel/types/unsafe_pinned.rs | 111 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 117 insertions(+)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 9d0471afc9648f2973235488b441eb109069adb1..705f420fdfbc4a576de1c4546578f2f04cdf615e 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -578,3 +581,6 @@ pub enum Either<L, R> {
>>  /// [`NotThreadSafe`]: type@NotThreadSafe
>>  #[allow(non_upper_case_globals)]
>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>> +
>> +mod unsafe_pinned;
>> +pub use unsafe_pinned::UnsafePinned;
> 
> I would put `mod` to the top of the 

Your sentence was cut off, I assume you mean:

> I would put `mod` to the top of the file.

I can do that, let me know if I should send a
new version or if this will be fixed when applying.

Cheers
Christian

On Sat May 17, 2025 at 1:36 PM CEST, Christian Schrefl wrote:
> Hi Benno,
>
> On 13.05.25 10:51 PM, Benno Lossin wrote:
>> On Sun May 11, 2025 at 8:21 PM CEST, Christian Schrefl wrote:
>>> `UnsafePinned<T>` is useful for cases where a value might be shared with
>>> C code but not directly used by it. In particular this is added for
>>> storing additional data in the `MiscDeviceRegistration` which will be
>>> shared between `fops->open` and the containing struct.
>>>
>>> Similar to `Opaque` but guarantees that the value is always initialized
>>> and that the inner value is dropped when `UnsafePinned` is dropped.
>>>
>>> This was originally proposed for the IRQ abstractions [0] and is also
>>> useful for other where the inner data may be aliased, but is always
>>> valid and automatic `Drop` is desired.
>>>
>>> Since then the `UnsafePinned` type was added to upstream Rust [1] by Sky
>>> as a unstable feature, therefore this patch implements the subset of the
>>> upstream API for the `UnsafePinned` type required for additional data in
>>> `MiscDeviceRegistration` and in the implementation of the `Opaque` type.
>>>
>>> Some differences to the upstream type definition are required in the
>>> kernel implementation, because upstream type uses some compiler changes
>>> to opt out of certain optimizations, this is documented in the
>>> documentation and a comment on the `UnsafePinned` type.
>>>
>>> The documentation on is based on the upstream rust documentation with
>>> minor modifications for the kernel implementation.
>>>
>>> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
>>> Link: https://github.com/rust-lang/rust/pull/137043 [1]
>>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>> Co-developed-by: Sky <sky@sky9.dev>
>>> Signed-off-by: Sky <sky@sky9.dev>
>>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
>> 
>> One nit below, with that fixed:
>> 
>> Reviewed-by: Benno Lossin <lossin@kernel.org>
>> 
>>> ---
>>>  rust/kernel/types.rs               |   6 ++
>>>  rust/kernel/types/unsafe_pinned.rs | 111 +++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 117 insertions(+)
>>>
>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>> index 9d0471afc9648f2973235488b441eb109069adb1..705f420fdfbc4a576de1c4546578f2f04cdf615e 100644
>>> --- a/rust/kernel/types.rs
>>> +++ b/rust/kernel/types.rs
>>> @@ -578,3 +581,6 @@ pub enum Either<L, R> {
>>>  /// [`NotThreadSafe`]: type@NotThreadSafe
>>>  #[allow(non_upper_case_globals)]
>>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>>> +
>>> +mod unsafe_pinned;
>>> +pub use unsafe_pinned::UnsafePinned;
>> 
>> I would put `mod` to the top of the 
>
> Your sentence was cut off, I assume you mean:
>
>> I would put `mod` to the top of the file.

Oh yeah sorry about that.

> I can do that, let me know if I should send a
> new version or if this will be fixed when applying.

I think Miguel can do this when picking the patch :)

---
Cheers,
Benno