[PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Alexandre Courbot]

The command-queue structure has a `dma_handle` method that returns the
DMA handle to the memory segment shared with the GSP. This works, but is
not ideal for the following reasons:

- That method is effectively only ever called once, and is technically
  an accessor method since the handle doesn't change over time,
- It feels a bit out-of-place with the other methods of `Cmdq` which
  only deal with the sending or receiving of messages,
- The method has `pub(crate)` visibility, allowing other driver code to
  access this highly-sensitive handle.

Address all these issues by turning `dma_handle` into a struct member
with `pub(super)` visibility. This keeps the method space focused, and
also ensures the member is not visible outside of the modules that need
it.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 26 +++++++++++++++-----------
 drivers/gpu/nova-core/gsp/fw.rs   |  2 +-
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index d36a62ba1c60..03c7c2d0ea35 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -30,6 +30,8 @@
     SplitState, //
 };
 
+use pin_init::pin_init_scope;
+
 use crate::{
     driver::Bar0,
     gsp::{
@@ -455,6 +457,8 @@ pub(crate) struct Cmdq {
     /// Inner mutex-protected state.
     #[pin]
     inner: Mutex<CmdqInner>,
+    /// DMA handle of the command queue's shared memory region.
+    pub(super) dma_handle: DmaAddress,
 }
 
 impl Cmdq {
@@ -479,12 +483,17 @@ impl Cmdq {
 
     /// Creates a new command queue for `dev`.
     pub(crate) fn new(dev: &device::Device<device::Bound>) -> impl PinInit<Self, Error> + '_ {
-        try_pin_init!(Self {
-            inner <- new_mutex!(CmdqInner {
-                dev: dev.into(),
-                gsp_mem: DmaGspMem::new(dev)?,
-                seq: 0,
-            }),
+        pin_init_scope(move || {
+            let gsp_mem = DmaGspMem::new(dev)?;
+
+            Ok(try_pin_init!(Self {
+                dma_handle: gsp_mem.0.dma_handle(),
+                inner <- new_mutex!(CmdqInner {
+                    dev: dev.into(),
+                    gsp_mem,
+                    seq: 0,
+                }),
+            }))
         })
     }
 
@@ -570,11 +579,6 @@ pub(crate) fn receive_msg<M: MessageFromGsp>(&self, timeout: Delta) -> Result<M>
     {
         self.inner.lock().receive_msg(timeout)
     }
-
-    /// Returns the DMA handle of the command queue's shared memory region.
-    pub(crate) fn dma_handle(&self) -> DmaAddress {
-        self.inner.lock().gsp_mem.0.dma_handle()
-    }
 }
 
 /// Inner mutex protected state of [`Cmdq`].
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index a061131b5412..0506c2293e7c 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -892,7 +892,7 @@ impl MessageQueueInitArguments {
     /// Creates a new init arguments structure for `cmdq`.
     fn new(cmdq: &Cmdq) -> Self {
         Self(bindings::MESSAGE_QUEUE_INIT_ARGUMENTS {
-            sharedMemPhysAddr: cmdq.dma_handle(),
+            sharedMemPhysAddr: cmdq.dma_handle,
             pageTableEntryCount: num::usize_into_u32::<{ Cmdq::NUM_PTES }>(),
             cmdQueueOffset: num::usize_as_u64(Cmdq::CMDQ_OFFSET),
             statQueueOffset: num::usize_as_u64(Cmdq::STATQ_OFFSET),

---
base-commit: a19457958c3018783881c4416f272cd594f13049
change-id: 20260319-b4-cmdq-dma-handle-c56ae9006104

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Eliot Courtney]

On Thu Mar 19, 2026 at 3:00 PM JST, Alexandre Courbot wrote:
> The command-queue structure has a `dma_handle` method that returns the
> DMA handle to the memory segment shared with the GSP. This works, but is
> not ideal for the following reasons:
>
> - That method is effectively only ever called once, and is technically
>   an accessor method since the handle doesn't change over time,
> - It feels a bit out-of-place with the other methods of `Cmdq` which
>   only deal with the sending or receiving of messages,
> - The method has `pub(crate)` visibility, allowing other driver code to
>   access this highly-sensitive handle.
>
> Address all these issues by turning `dma_handle` into a struct member
> with `pub(super)` visibility. This keeps the method space focused, and
> also ensures the member is not visible outside of the modules that need
> it.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>

Re: [PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Danilo Krummrich]

On Thu Mar 19, 2026 at 7:00 AM CET, Alexandre Courbot wrote:
> The command-queue structure has a `dma_handle` method that returns the
> DMA handle to the memory segment shared with the GSP. This works, but is
> not ideal for the following reasons:
>
> - That method is effectively only ever called once, and is technically
>   an accessor method since the handle doesn't change over time,
> - It feels a bit out-of-place with the other methods of `Cmdq` which
>   only deal with the sending or receiving of messages,
> - The method has `pub(crate)` visibility, allowing other driver code to
>   access this highly-sensitive handle.
>
> Address all these issues by turning `dma_handle` into a struct member
> with `pub(super)` visibility. This keeps the method space focused, and
> also ensures the member is not visible outside of the modules that need
> it.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Linking my note from the original patch series [1].

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

[1] https://lore.kernel.org/all/DH60S91O053Z.1DDUJSL0D6NQQ@kernel.org/

Re: [PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Alexandre Courbot]

On Fri Mar 20, 2026 at 9:45 PM JST, Danilo Krummrich wrote:
> On Thu Mar 19, 2026 at 7:00 AM CET, Alexandre Courbot wrote:
>> The command-queue structure has a `dma_handle` method that returns the
>> DMA handle to the memory segment shared with the GSP. This works, but is
>> not ideal for the following reasons:
>>
>> - That method is effectively only ever called once, and is technically
>>   an accessor method since the handle doesn't change over time,
>> - It feels a bit out-of-place with the other methods of `Cmdq` which
>>   only deal with the sending or receiving of messages,
>> - The method has `pub(crate)` visibility, allowing other driver code to
>>   access this highly-sensitive handle.
>>
>> Address all these issues by turning `dma_handle` into a struct member
>> with `pub(super)` visibility. This keeps the method space focused, and
>> also ensures the member is not visible outside of the modules that need
>> it.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Linking my note from the original patch series [1].
>
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>
> [1] https://lore.kernel.org/all/DH60S91O053Z.1DDUJSL0D6NQQ@kernel.org/

Do you want me to add a `Link:` tag to the commit?

Re: [PATCH] gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Danilo Krummrich]

On 3/21/26 2:41 PM, Alexandre Courbot wrote:
> On Fri Mar 20, 2026 at 9:45 PM JST, Danilo Krummrich wrote:
>> On Thu Mar 19, 2026 at 7:00 AM CET, Alexandre Courbot wrote:
>>> The command-queue structure has a `dma_handle` method that returns the
>>> DMA handle to the memory segment shared with the GSP. This works, but is
>>> not ideal for the following reasons:
>>>
>>> - That method is effectively only ever called once, and is technically
>>>   an accessor method since the handle doesn't change over time,
>>> - It feels a bit out-of-place with the other methods of `Cmdq` which
>>>   only deal with the sending or receiving of messages,
>>> - The method has `pub(crate)` visibility, allowing other driver code to
>>>   access this highly-sensitive handle.
>>>
>>> Address all these issues by turning `dma_handle` into a struct member
>>> with `pub(super)` visibility. This keeps the method space focused, and
>>> also ensures the member is not visible outside of the modules that need
>>> it.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> Linking my note from the original patch series [1].
>>
>> Reviewed-by: Danilo Krummrich <dakr@kernel.org>
>>
>> [1] https://lore.kernel.org/all/DH60S91O053Z.1DDUJSL0D6NQQ@kernel.org/
> 
> Do you want me to add a `Link:` tag to the commit?

I don't think that's necessary.

Claude review: gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Claude Code Review Bot]

Overall Series Review

Subject: gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member
Author: Alexandre Courbot <acourbot@nvidia.com>
Patches: 5
Reviewed: 2026-03-22T04:46:10.923513

---

This is a single, clean refactoring patch for the `nova-core` GPU driver (Rust). It converts a method (`Cmdq::dma_handle()`) that locked a mutex just to return an immutable DMA address into a plain struct field, computed once at construction time. The change is well-motivated, straightforward, and correct.

**Verdict: Looks good.**

---
Generated by Claude Code Patch Reviewer

Claude review: gpu: nova-core: gsp: move Cmdq's DMA handle to a struct member

[Claude Code Review Bot]

Patch Review

**Motivation (sound):** The commit message clearly explains three valid reasons:
1. The accessor is only called once and the value never changes — no need for a method.
2. The method was out of place among message send/receive methods.
3. `pub(crate)` visibility was overly broad; `pub(super)` is tighter.

**Construction change (correct):** The `new()` initializer is refactored from a simple `try_pin_init!` to use `pin_init_scope`, which allows pre-computing `gsp_mem` before consuming it:

```rust
pin_init_scope(move || {
    let gsp_mem = DmaGspMem::new(dev)?;

    Ok(try_pin_init!(Self {
        dma_handle: gsp_mem.0.dma_handle(),
        inner <- new_mutex!(CmdqInner {
            dev: dev.into(),
            gsp_mem,
            seq: 0,
        }),
    }))
})
```

This is the correct pattern — `pin_init_scope` exists exactly for this "compute something, then use it in the initializer" case, as documented in `rust/pin-init/src/lib.rs:1278`. The DMA handle is extracted from `gsp_mem` before `gsp_mem` is moved into `CmdqInner`, which is necessary since you can't borrow after move.

**Caller update (trivial, correct):** In `fw.rs`, the method call `cmdq.dma_handle()` becomes field access `cmdq.dma_handle`:

```rust
sharedMemPhysAddr: cmdq.dma_handle,
```

This is the only call site, confirming the commit message's claim that the method is "effectively only ever called once."

**Visibility narrowing:** Changing from `pub(crate)` to `pub(super)` is a good practice — `fw.rs` is in the same `gsp` module (super), so it retains access, while code outside `gsp` no longer can reach this sensitive DMA address.

**One minor observation:** The `DmaAddress` type is `Copy` (it's used by value in both old and new code without `.clone()`), so storing it as a separate field has no semantic issue — it's just a copied integer/address value, not a second reference to the DMA mapping.

**No issues found.** The patch is clean, well-scoped, and the `pin_init_scope` usage matches the documented pattern.

Reviewed-by assessment: **No objections.**

---
Generated by Claude Code Patch Reviewer