[PATCH 0/4] Alloc and drm::Device fixes

[PATCH 0/4] Alloc and drm::Device fixes

[Danilo Krummrich]

This patch series replaces aligned_size() with Kmalloc::aligned_layout(), which
is subsequently used to obtain a kmalloc() compatible Layout for
__drm_dev_alloc() in drm::Device::new().

It also contains two additional drm::Device fixes; the commits are based on
next-20250731.

Danilo Krummrich (4):
  rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()
  rust: drm: ensure kmalloc() compatible Layout
  rust: drm: remove pin annotations from drm::Device
  rust: drm: don't pass the address of drm::Device to drm_dev_put()

 rust/kernel/alloc/allocator.rs | 30 ++++++++++++++++++------------
 rust/kernel/drm/device.rs      | 32 +++++++++++++++++++++++++-------
 2 files changed, 43 insertions(+), 19 deletions(-)


base-commit: 84b92a499e7eca54ba1df6f6c6e01766025943f1
-- 
2.50.0

[PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Danilo Krummrich]

aligned_size() dates back to when Rust did support kmalloc() only, but
is now used in ReallocFunc::call() and hence for all allocators.

However, the additional padding applied by aligned_size() is only
required by the kmalloc() allocator backend.

Hence, replace aligned_size() with Kmalloc::aligned_layout() and use it
for the affected allocators, i.e. kmalloc() and kvmalloc(), only.

While at it, make Kmalloc::aligned_layout() public, such that Rust
abstractions, which have to call subsystem specific kmalloc() based
allocation primitives directly, can make use of it.

Fixes: 8a799831fc63 ("rust: alloc: implement `ReallocFunc`")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/alloc/allocator.rs | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/alloc/allocator.rs b/rust/kernel/alloc/allocator.rs
index aa2dfa9dca4c..3331ef338f3b 100644
--- a/rust/kernel/alloc/allocator.rs
+++ b/rust/kernel/alloc/allocator.rs
@@ -43,17 +43,6 @@
 /// For more details see [self].
 pub struct KVmalloc;
 
-/// Returns a proper size to alloc a new object aligned to `new_layout`'s alignment.
-fn aligned_size(new_layout: Layout) -> usize {
-    // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
-    let layout = new_layout.pad_to_align();
-
-    // Note that `layout.size()` (after padding) is guaranteed to be a multiple of `layout.align()`
-    // which together with the slab guarantees means the `krealloc` will return a properly aligned
-    // object (see comments in `kmalloc()` for more information).
-    layout.size()
-}
-
 /// # Invariants
 ///
 /// One of the following: `krealloc`, `vrealloc`, `kvrealloc`.
@@ -88,7 +77,7 @@ unsafe fn call(
         old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
-        let size = aligned_size(layout);
+        let size = layout.size();
         let ptr = match ptr {
             Some(ptr) => {
                 if old_layout.size() == 0 {
@@ -123,6 +112,17 @@ unsafe fn call(
     }
 }
 
+impl Kmalloc {
+    /// Returns a [`Layout`] that makes [`Kmalloc`] fulfill the requested size and alignment of
+    /// `layout`.
+    pub const fn aligned_layout(layout: Layout) -> Layout {
+        // Note that `layout.size()` (after padding) is guaranteed to be a multiple of
+        // `layout.align()` which together with the slab guarantees means that `Kmalloc` will return
+        // a properly aligned object (see comments in `kmalloc()` for more information).
+        layout.pad_to_align()
+    }
+}
+
 // SAFETY: `realloc` delegates to `ReallocFunc::call`, which guarantees that
 // - memory remains valid until it is explicitly freed,
 // - passing a pointer to a valid memory allocation is OK,
@@ -135,6 +135,8 @@ unsafe fn realloc(
         old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
+        let layout = Kmalloc::aligned_layout(layout);
+
         // SAFETY: `ReallocFunc::call` has the same safety requirements as `Allocator::realloc`.
         unsafe { ReallocFunc::KREALLOC.call(ptr, layout, old_layout, flags) }
     }
@@ -176,6 +178,10 @@ unsafe fn realloc(
         old_layout: Layout,
         flags: Flags,
     ) -> Result<NonNull<[u8]>, AllocError> {
+        // `KVmalloc` may use the `Kmalloc` backend, hence we have to enforce a `Kmalloc`
+        // compatible layout.
+        let layout = Kmalloc::aligned_layout(layout);
+
         // TODO: Support alignments larger than PAGE_SIZE.
         if layout.align() > bindings::PAGE_SIZE {
             pr_warn!("KVmalloc does not support alignments larger than PAGE_SIZE yet.\n");
-- 
2.50.0

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Alice Ryhl]

On Thu, Jul 31, 2025 at 05:48:06PM +0200, Danilo Krummrich wrote:
> aligned_size() dates back to when Rust did support kmalloc() only, but
> is now used in ReallocFunc::call() and hence for all allocators.
> 
> However, the additional padding applied by aligned_size() is only
> required by the kmalloc() allocator backend.
> 
> Hence, replace aligned_size() with Kmalloc::aligned_layout() and use it
> for the affected allocators, i.e. kmalloc() and kvmalloc(), only.
> 
> While at it, make Kmalloc::aligned_layout() public, such that Rust
> abstractions, which have to call subsystem specific kmalloc() based
> allocation primitives directly, can make use of it.
> 
> Fixes: 8a799831fc63 ("rust: alloc: implement `ReallocFunc`")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

I guess vmalloc handles alignment in a different way ... ok makes sense
to me.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Miguel Ojeda]

On Thu, Jul 31, 2025 at 5:49 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> aligned_size() dates back to when Rust did support kmalloc() only, but
> is now used in ReallocFunc::call() and hence for all allocators.
>
> However, the additional padding applied by aligned_size() is only
> required by the kmalloc() allocator backend.
>
> Hence, replace aligned_size() with Kmalloc::aligned_layout() and use it
> for the affected allocators, i.e. kmalloc() and kvmalloc(), only.
>
> While at it, make Kmalloc::aligned_layout() public, such that Rust
> abstractions, which have to call subsystem specific kmalloc() based
> allocation primitives directly, can make use of it.
>
> Fixes: 8a799831fc63 ("rust: alloc: implement `ReallocFunc`")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Did this need Cc: stable or was it skipped since it is just extra padding?

(i.e. not sure what you usually do for DRM, but I was looking at this
since it is alloc since I would normally pick it.)

Thanks!

Cheers,
Miguel

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Danilo Krummrich]

On Tue Aug 12, 2025 at 9:52 PM CEST, Miguel Ojeda wrote:
> Did this need Cc: stable or was it skipped since it is just extra padding?

I don't think so, it just lead to pad to the alignment for Vmalloc too.

Technically, this makes no difference, since Vmalloc is always PAGE_SIZE aligned
and the size always a multiple of PAGE_SIZE.

However, the patch is a prerequisite for the DRM device fix in patch 2.

- Danilo

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Miguel Ojeda]

On Tue, Aug 12, 2025 at 10:00 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I don't think so, it just lead to pad to the alignment for Vmalloc too.
>
> Technically, this makes no difference, since Vmalloc is always PAGE_SIZE aligned
> and the size always a multiple of PAGE_SIZE.

Got it, thanks for the quick reply! Then I guess we could have skipped
the Fixes in this one, but it is not a big deal and as usual it
depends on how one defines "fix".

Cheers,
Miguel

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Danilo Krummrich]

On Tue Aug 12, 2025 at 10:12 PM CEST, Miguel Ojeda wrote:
> On Tue, Aug 12, 2025 at 10:00 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> I don't think so, it just lead to pad to the alignment for Vmalloc too.
>>
>> Technically, this makes no difference, since Vmalloc is always PAGE_SIZE aligned
>> and the size always a multiple of PAGE_SIZE.
>
> Got it, thanks for the quick reply! Then I guess we could have skipped
> the Fixes in this one, but it is not a big deal and as usual it
> depends on how one defines "fix".

Yeah, in the past I was more on the "'Fixes:' for actual bugs only" side of
things, but I changed my mind a bit; I find it useful to have this as a
reference even for minor issues that might not be actual bugs, such as this
one.

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Miguel Ojeda]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Thu, Jul 31, 2025 at 5:49 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> +impl Kmalloc {
> +    /// Returns a [`Layout`] that makes [`Kmalloc`] fulfill the requested size and alignment of
> +    /// `layout`.
> +    pub const fn aligned_layout(layout: Layout) -> Layout {

I think this `const fn` here was removed when applying to make it work
with older compilers, right?

I was fixing another `rusttest` thing and noticed while applying
these. I had a patch to fix it, since we can actually just use the
feature, and then I noticed it wasn't in the tree. Since I have it, I
am attaching it for reference in case the now-stable feature is
needed, e.g. if you want to make that `const fn` again.

Cheers,
Miguel

[-- Attachment #2: 0001-rust-kernel-use-const_alloc_layout-feature-to-fix-Ru.patch --]
[-- Type: application/x-patch, Size: 1625 bytes --]

Re: [PATCH 1/4] rust: alloc: replace aligned_size() with Kmalloc::aligned_layout()

[Danilo Krummrich]

On Sat Aug 16, 2025 at 10:40 PM CEST, Miguel Ojeda wrote:
> On Thu, Jul 31, 2025 at 5:49 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> +impl Kmalloc {
>> +    /// Returns a [`Layout`] that makes [`Kmalloc`] fulfill the requested size and alignment of
>> +    /// `layout`.
>> +    pub const fn aligned_layout(layout: Layout) -> Layout {
>
> I think this `const fn` here was removed when applying to make it work
> with older compilers, right?

Yes, that's correct.

> I was fixing another `rusttest` thing and noticed while applying
> these. I had a patch to fix it, since we can actually just use the
> feature, and then I noticed it wasn't in the tree. Since I have it, I
> am attaching it for reference in case the now-stable feature is
> needed, e.g. if you want to make that `const fn` again.

I think it doesn't add much value for this to be a const function anyways, but
maybe it is more useful elsewhere? In that case, I think it also doesn't hurt
to Kmalloc::aligned_layout() const again.

[PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout

[Danilo Krummrich]

drm::Device is allocated through __drm_dev_alloc() (which uses
kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
initialized in-place.

Due to the order of fields in drm::Device

  pub struct Device<T: drm::Driver> {
     dev: Opaque<bindings::drm_device>,
     data: T::Data,
  }

even with an arbitrary large alignment requirement of T::Data it can't
happen that the size of Device is smaller than its alignment requirement.

However, let's not rely on this subtle circumstance and create a proper
kmalloc() compatible Layout.

Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/drm/device.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index 3bb7c83966cf..d19410deaf6c 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -5,6 +5,7 @@
 //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
 
 use crate::{
+    alloc::allocator::Kmalloc,
     bindings, device, drm,
     drm::driver::AllocImpl,
     error::from_err_ptr,
@@ -12,7 +13,7 @@
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
-use core::{mem, ops::Deref, ptr, ptr::NonNull};
+use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
 
 #[cfg(CONFIG_DRM_LEGACY)]
 macro_rules! drm_legacy_fields {
@@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
 
     /// Create a new `drm::Device` for a `drm::Driver`.
     pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
+        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
+        // compatible `Layout`.
+        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
+
         // SAFETY:
         // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
         // - `dev` is valid by its type invarants,
@@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
             bindings::__drm_dev_alloc(
                 dev.as_raw(),
                 &Self::VTABLE,
-                mem::size_of::<Self>(),
+                layout.size(),
                 mem::offset_of!(Self, dev),
             )
         }
-- 
2.50.0

Re: [PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout

[Alice Ryhl]

On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
> drm::Device is allocated through __drm_dev_alloc() (which uses
> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> initialized in-place.
> 
> Due to the order of fields in drm::Device
> 
>   pub struct Device<T: drm::Driver> {
>      dev: Opaque<bindings::drm_device>,
>      data: T::Data,
>   }

I'm not convinced this patch is right.

Imagine this scenario: T::Data has size and alignment both equal to 16,
and lets say that drm_device has a size that is a multiple of 8 but not
16 such as 72. In that case, you will allocate 72+16=88 bytes for
Device, but actually the size of Device is 96 because there is 8 bytes
of padding between dev and data.

Alice

> even with an arbitrary large alignment requirement of T::Data it can't
> happen that the size of Device is smaller than its alignment requirement.
> 
> However, let's not rely on this subtle circumstance and create a proper
> kmalloc() compatible Layout.
> 
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/drm/device.rs | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3bb7c83966cf..d19410deaf6c 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -5,6 +5,7 @@
>  //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>  
>  use crate::{
> +    alloc::allocator::Kmalloc,
>      bindings, device, drm,
>      drm::driver::AllocImpl,
>      error::from_err_ptr,
> @@ -12,7 +13,7 @@
>      prelude::*,
>      types::{ARef, AlwaysRefCounted, Opaque},
>  };
> -use core::{mem, ops::Deref, ptr, ptr::NonNull};
> +use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>  
>  #[cfg(CONFIG_DRM_LEGACY)]
>  macro_rules! drm_legacy_fields {
> @@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
>  
>      /// Create a new `drm::Device` for a `drm::Driver`.
>      pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
> +        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
> +        // compatible `Layout`.
> +        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
> +
>          // SAFETY:
>          // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
>          // - `dev` is valid by its type invarants,
> @@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>              bindings::__drm_dev_alloc(
>                  dev.as_raw(),
>                  &Self::VTABLE,
> -                mem::size_of::<Self>(),
> +                layout.size(),
>                  mem::offset_of!(Self, dev),
>              )
>          }
> -- 
> 2.50.0
> 

Re: [PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout

[Danilo Krummrich]

On Fri Aug 1, 2025 at 11:18 AM CEST, Alice Ryhl wrote:
> On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
>> drm::Device is allocated through __drm_dev_alloc() (which uses
>> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
>> initialized in-place.
>> 
>> Due to the order of fields in drm::Device
>> 
>>   pub struct Device<T: drm::Driver> {
>>      dev: Opaque<bindings::drm_device>,
>>      data: T::Data,
>>   }
>
> I'm not convinced this patch is right.
>
> Imagine this scenario: T::Data has size and alignment both equal to 16,
> and lets say that drm_device has a size that is a multiple of 8 but not
> 16 such as 72. In that case, you will allocate 72+16=88 bytes for
> Device, but actually the size of Device is 96 because there is 8 bytes
> of padding between dev and data.

Are you saying that there is an issue with

  (1) the existing implementation with uses mem::size_of::<Self>() or

  (2) the proper one that uses Kmalloc::aligned_layout(Layout::new::<Self>())?

I think neither has, because we're not allocating
size_of::<Opaque<bindings::drm_device>>() + size_of::<T::Data>() as you seem to
assume above, but size_of::<Device<T>>().

>> even with an arbitrary large alignment requirement of T::Data it can't
>> happen that the size of Device is smaller than its alignment requirement.
>> 
>> However, let's not rely on this subtle circumstance and create a proper
>> kmalloc() compatible Layout.
>> 
>> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/drm/device.rs | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
>> index 3bb7c83966cf..d19410deaf6c 100644
>> --- a/rust/kernel/drm/device.rs
>> +++ b/rust/kernel/drm/device.rs
>> @@ -5,6 +5,7 @@
>>  //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>>  
>>  use crate::{
>> +    alloc::allocator::Kmalloc,
>>      bindings, device, drm,
>>      drm::driver::AllocImpl,
>>      error::from_err_ptr,
>> @@ -12,7 +13,7 @@
>>      prelude::*,
>>      types::{ARef, AlwaysRefCounted, Opaque},
>>  };
>> -use core::{mem, ops::Deref, ptr, ptr::NonNull};
>> +use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>>  
>>  #[cfg(CONFIG_DRM_LEGACY)]
>>  macro_rules! drm_legacy_fields {
>> @@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
>>  
>>      /// Create a new `drm::Device` for a `drm::Driver`.
>>      pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
>> +        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
>> +        // compatible `Layout`.
>> +        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
>> +
>>          // SAFETY:
>>          // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
>>          // - `dev` is valid by its type invarants,
>> @@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>              bindings::__drm_dev_alloc(
>>                  dev.as_raw(),
>>                  &Self::VTABLE,
>> -                mem::size_of::<Self>(),
>> +                layout.size(),
>>                  mem::offset_of!(Self, dev),
>>              )
>>          }
>> -- 
>> 2.50.0
>> 

Re: [PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout

[Alice Ryhl]

On Fri, Aug 1, 2025 at 11:29 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri Aug 1, 2025 at 11:18 AM CEST, Alice Ryhl wrote:
> > On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
> >> drm::Device is allocated through __drm_dev_alloc() (which uses
> >> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> >> initialized in-place.
> >>
> >> Due to the order of fields in drm::Device
> >>
> >>   pub struct Device<T: drm::Driver> {
> >>      dev: Opaque<bindings::drm_device>,
> >>      data: T::Data,
> >>   }
> >
> > I'm not convinced this patch is right.
> >
> > Imagine this scenario: T::Data has size and alignment both equal to 16,
> > and lets say that drm_device has a size that is a multiple of 8 but not
> > 16 such as 72. In that case, you will allocate 72+16=88 bytes for
> > Device, but actually the size of Device is 96 because there is 8 bytes
> > of padding between dev and data.
>
> Are you saying that there is an issue with
>
>   (1) the existing implementation with uses mem::size_of::<Self>() or
>
>   (2) the proper one that uses Kmalloc::aligned_layout(Layout::new::<Self>())?
>
> I think neither has, because we're not allocating
> size_of::<Opaque<bindings::drm_device>>() + size_of::<T::Data>() as you seem to
> assume above, but size_of::<Device<T>>().

Yes, you're right. I misunderstood how __drm_dev_alloc works.

Alice

Re: [PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout

[Alice Ryhl]

On Thu, Jul 31, 2025 at 5:49 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> drm::Device is allocated through __drm_dev_alloc() (which uses
> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> initialized in-place.
>
> Due to the order of fields in drm::Device
>
>   pub struct Device<T: drm::Driver> {
>      dev: Opaque<bindings::drm_device>,
>      data: T::Data,
>   }
>
> even with an arbitrary large alignment requirement of T::Data it can't
> happen that the size of Device is smaller than its alignment requirement.
>
> However, let's not rely on this subtle circumstance and create a proper
> kmalloc() compatible Layout.
>
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH 3/4] rust: drm: remove pin annotations from drm::Device

[Danilo Krummrich]

The #[pin_data] and #[pin] annotations are not necessary for
drm::Device, since we don't use any pin-init macros, but only
__pinned_init() on the impl PinInit<T::Data, Error> argument of
drm::Device::new().

Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/drm/device.rs | 2 --
 1 file changed, 2 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index d19410deaf6c..d0a9528121f1 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -54,10 +54,8 @@ macro_rules! drm_legacy_fields {
 ///
 /// `self.dev` is a valid instance of a `struct device`.
 #[repr(C)]
-#[pin_data]
 pub struct Device<T: drm::Driver> {
     dev: Opaque<bindings::drm_device>,
-    #[pin]
     data: T::Data,
 }
 
-- 
2.50.0

Re: [PATCH 3/4] rust: drm: remove pin annotations from drm::Device

[Benno Lossin]

On Thu Jul 31, 2025 at 5:48 PM CEST, Danilo Krummrich wrote:
> The #[pin_data] and #[pin] annotations are not necessary for
> drm::Device, since we don't use any pin-init macros, but only
> __pinned_init() on the impl PinInit<T::Data, Error> argument of
> drm::Device::new().

But you're still pinning `Device`, right?

> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/drm/device.rs | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index d19410deaf6c..d0a9528121f1 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -54,10 +54,8 @@ macro_rules! drm_legacy_fields {
>  ///
>  /// `self.dev` is a valid instance of a `struct device`.
>  #[repr(C)]
> -#[pin_data]
>  pub struct Device<T: drm::Driver> {
>      dev: Opaque<bindings::drm_device>,
> -    #[pin]
>      data: T::Data,

Looking at this code again, I also noticed that it was wrong before this
patch: `Device<T>` implemented `Unpin` if `T::Data` did which is most
likely wrong (or is `drm_device` not address sensitive?).

So good to see that fixed, thanks!

---
Cheers,
Benno

>  }
>  

Re: [PATCH 3/4] rust: drm: remove pin annotations from drm::Device

[Danilo Krummrich]

On Thu Jul 31, 2025 at 8:54 PM CEST, Benno Lossin wrote:
> On Thu Jul 31, 2025 at 5:48 PM CEST, Danilo Krummrich wrote:
>> The #[pin_data] and #[pin] annotations are not necessary for
>> drm::Device, since we don't use any pin-init macros, but only
>> __pinned_init() on the impl PinInit<T::Data, Error> argument of
>> drm::Device::new().
>
> But you're still pinning `Device`, right?

A drm::Device instance never exists other than as ARef<drm::Device>.

>> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/drm/device.rs | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
>> index d19410deaf6c..d0a9528121f1 100644
>> --- a/rust/kernel/drm/device.rs
>> +++ b/rust/kernel/drm/device.rs
>> @@ -54,10 +54,8 @@ macro_rules! drm_legacy_fields {
>>  ///
>>  /// `self.dev` is a valid instance of a `struct device`.
>>  #[repr(C)]
>> -#[pin_data]
>>  pub struct Device<T: drm::Driver> {
>>      dev: Opaque<bindings::drm_device>,
>> -    #[pin]
>>      data: T::Data,
>
> Looking at this code again, I also noticed that it was wrong before this
> patch: `Device<T>` implemented `Unpin` if `T::Data` did which is most
> likely wrong (or is `drm_device` not address sensitive?).

It is, but as mentioned above a drm::Device only ever exists as
ARef<drm::Device>.

So, in drm::Device::new() we allocate the drm::Device with __drm_dev_alloc(),
initialize data in-place within this allocated memory and create an
ARef<drm::Device> directly from the raw pointer returned by __drm_dev_alloc().

> So good to see that fixed, thanks!
>
> ---
> Cheers,
> Benno
>
>>  }
>>  

Re: [PATCH 3/4] rust: drm: remove pin annotations from drm::Device

[Benno Lossin]

On Fri Aug 1, 2025 at 10:21 AM CEST, Danilo Krummrich wrote:
> On Thu Jul 31, 2025 at 8:54 PM CEST, Benno Lossin wrote:
>> On Thu Jul 31, 2025 at 5:48 PM CEST, Danilo Krummrich wrote:
>>>  #[repr(C)]
>>> -#[pin_data]
>>>  pub struct Device<T: drm::Driver> {
>>>      dev: Opaque<bindings::drm_device>,
>>> -    #[pin]
>>>      data: T::Data,
>>
>> Looking at this code again, I also noticed that it was wrong before this
>> patch: `Device<T>` implemented `Unpin` if `T::Data` did which is most
>> likely wrong (or is `drm_device` not address sensitive?).
>
> It is, but as mentioned above a drm::Device only ever exists as
> ARef<drm::Device>.

Yeah the `Unpin` thing isn't a problem for `ARef`, but we are
theoretically allowed to implement moving out of an `ARef` (given that
it is unique) when the type is `Unpin`.

Thanks for confirming.

---
Cheers,
Benno

Re: [PATCH 3/4] rust: drm: remove pin annotations from drm::Device

[Benno Lossin]

On Thu Jul 31, 2025 at 5:48 PM CEST, Danilo Krummrich wrote:
> The #[pin_data] and #[pin] annotations are not necessary for
> drm::Device, since we don't use any pin-init macros, but only
> __pinned_init() on the impl PinInit<T::Data, Error> argument of
> drm::Device::new().
>
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

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

---
Cheers,
Benno

> ---
>  rust/kernel/drm/device.rs | 2 --
>  1 file changed, 2 deletions(-)

[PATCH 4/4] rust: drm: don't pass the address of drm::Device to drm_dev_put()

[Danilo Krummrich]

In drm_dev_put() call in AlwaysRefCounted::dec_ref() we rely on struct
drm_device to be the first field in drm::Device, whereas everywhere
else we correctly obtain the address of the actual struct drm_device.

Analogous to the from_drm_device() helper, provide the
into_drm_device() helper in order to address this.

Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/drm/device.rs | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index d0a9528121f1..d29c477e89a8 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -120,9 +120,13 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
         // - `raw_data` is a valid pointer to uninitialized memory.
         // - `raw_data` will not move until it is dropped.
         unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
-            // SAFETY: `__drm_dev_alloc()` was successful, hence `raw_drm` must be valid and the
+            // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
+            // successful.
+            let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
+
+            // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the
             // refcount must be non-zero.
-            unsafe { bindings::drm_dev_put(ptr::addr_of_mut!((*raw_drm.as_ptr()).dev).cast()) };
+            unsafe { bindings::drm_dev_put(drm_dev) };
         })?;
 
         // SAFETY: The reference count is one, and now we take ownership of that reference as a
@@ -143,6 +147,14 @@ unsafe fn from_drm_device(ptr: *const bindings::drm_device) -> *mut Self {
         unsafe { crate::container_of!(Opaque::cast_from(ptr), Self, dev) }.cast_mut()
     }
 
+    /// # Safety
+    ///
+    /// `ptr` must be a valid pointer to `Self`.
+    unsafe fn into_drm_device(ptr: NonNull<Self>) -> *mut bindings::drm_device {
+        // SAFETY: By the safety requirements of this function, `ptr` is a valid pointer to `Self`.
+        unsafe { &raw mut (*ptr.as_ptr()).dev }.cast()
+    }
+
     /// Not intended to be called externally, except via declare_drm_ioctls!()
     ///
     /// # Safety
@@ -192,8 +204,11 @@ fn inc_ref(&self) {
     }
 
     unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: `obj` is a valid pointer to `Self`.
+        let drm_dev = unsafe { Self::into_drm_device(obj) };
+
         // SAFETY: The safety requirements guarantee that the refcount is non-zero.
-        unsafe { bindings::drm_dev_put(obj.cast().as_ptr()) };
+        unsafe { bindings::drm_dev_put(drm_dev) };
     }
 }
 
-- 
2.50.0

Re: [PATCH 4/4] rust: drm: don't pass the address of drm::Device to drm_dev_put()

[Alice Ryhl]

On Thu, Jul 31, 2025 at 05:48:09PM +0200, Danilo Krummrich wrote:
> In drm_dev_put() call in AlwaysRefCounted::dec_ref() we rely on struct
> drm_device to be the first field in drm::Device, whereas everywhere
> else we correctly obtain the address of the actual struct drm_device.
> 
> Analogous to the from_drm_device() helper, provide the
> into_drm_device() helper in order to address this.
> 
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> +    /// # Safety
> +    ///
> +    /// `ptr` must be a valid pointer to `Self`.
> +    unsafe fn into_drm_device(ptr: NonNull<Self>) -> *mut bindings::drm_device {
> +        // SAFETY: By the safety requirements of this function, `ptr` is a valid pointer to `Self`.
> +        unsafe { &raw mut (*ptr.as_ptr()).dev }.cast()
> +    }

I think it would somewhat more consistent to use the naming raw_get() or
cast_into(), but I am ok with this naming too.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH 0/4] Alloc and drm::Device fixes

[Danilo Krummrich]

On Thu Jul 31, 2025 at 5:48 PM CEST, Danilo Krummrich wrote:
> This patch series replaces aligned_size() with Kmalloc::aligned_layout(), which
> is subsequently used to obtain a kmalloc() compatible Layout for
> __drm_dev_alloc() in drm::Device::new().
>
> It also contains two additional drm::Device fixes; the commits are based on
> next-20250731.

Applied to drm-misc-fixes, thanks!