Wrap `Cmdq`'s mutable state in a new struct `CmdqInner` and wrap that in
a Mutex. This lets `Cmdq` methods take &self instead of &mut self, which
lets required commands be sent e.g. while unloading the driver.
The mutex is held over both send and receive in `send_command` to make
sure that it doesn't get the reply of some other command that could have
been sent just beforehand.
Reviewed-by: Zhi Wang <zhiw@nvidia.com>
Tested-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/boot.rs | 8 +-
drivers/gpu/nova-core/gsp/cmdq.rs | 170 +++++++++++++++++++--------------
drivers/gpu/nova-core/gsp/commands.rs | 4 +-
drivers/gpu/nova-core/gsp/sequencer.rs | 2 +-
4 files changed, 107 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index ffc478b33640..5e73bd769dcc 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -137,7 +137,7 @@ fn run_fwsec_frts(
///
/// Upon return, the GSP is up and running, and its runtime object given as return value.
pub(crate) fn boot(
- mut self: Pin<&mut Self>,
+ self: Pin<&mut Self>,
pdev: &pci::Device<device::Bound>,
bar: &Bar0,
chipset: Chipset,
@@ -223,13 +223,13 @@ pub(crate) fn boot(
dev: pdev.as_ref().into(),
bar,
};
- GspSequencer::run(&mut self.cmdq, seq_params)?;
+ GspSequencer::run(&self.cmdq, seq_params)?;
// Wait until GSP is fully initialized.
- commands::wait_gsp_init_done(&mut self.cmdq)?;
+ commands::wait_gsp_init_done(&self.cmdq)?;
// Obtain and display basic GPU information.
- let info = commands::get_gsp_info(&mut self.cmdq, bar)?;
+ let info = commands::get_gsp_info(&self.cmdq, bar)?;
match info.gpu_name() {
Ok(name) => dev_info!(pdev, "GPU name: {}\n", name),
Err(e) => dev_warn!(pdev, "GPU name unavailable: {:?}\n", e),
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 86ff9a3d1732..d36a62ba1c60 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -12,8 +12,12 @@
},
dma_write,
io::poll::read_poll_timeout,
+ new_mutex,
prelude::*,
- sync::aref::ARef,
+ sync::{
+ aref::ARef,
+ Mutex, //
+ },
time::Delta,
transmute::{
AsBytes,
@@ -448,12 +452,9 @@ struct GspMessage<'a> {
/// area.
#[pin_data]
pub(crate) struct Cmdq {
- /// Device this command queue belongs to.
- dev: ARef<device::Device>,
- /// Current command sequence number.
- seq: u32,
- /// Memory area shared with the GSP for communicating commands and messages.
- gsp_mem: DmaGspMem,
+ /// Inner mutex-protected state.
+ #[pin]
+ inner: Mutex<CmdqInner>,
}
impl Cmdq {
@@ -473,18 +474,17 @@ impl Cmdq {
/// Number of page table entries for the GSP shared region.
pub(crate) const NUM_PTES: usize = size_of::<GspMem>() >> GSP_PAGE_SHIFT;
- /// Timeout for waiting for space on the command queue.
- const ALLOCATE_TIMEOUT: Delta = Delta::from_secs(1);
-
/// Default timeout for receiving a message from the GSP.
pub(super) const RECEIVE_TIMEOUT: Delta = Delta::from_secs(5);
/// Creates a new command queue for `dev`.
pub(crate) fn new(dev: &device::Device<device::Bound>) -> impl PinInit<Self, Error> + '_ {
try_pin_init!(Self {
- gsp_mem: DmaGspMem::new(dev)?,
- dev: dev.into(),
- seq: 0,
+ inner <- new_mutex!(CmdqInner {
+ dev: dev.into(),
+ gsp_mem: DmaGspMem::new(dev)?,
+ seq: 0,
+ }),
})
}
@@ -508,6 +508,89 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
+ /// Sends `command` to the GSP and waits for the reply.
+ ///
+ /// Messages with non-matching function codes are silently consumed until the expected reply
+ /// arrives.
+ ///
+ /// The queue is locked for the entire send+receive cycle to ensure that no other command can
+ /// be interleaved.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available to send the command, or if the reply is
+ /// not received within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command and reply initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&self, bar: &Bar0, command: M) -> Result<M::Reply>
+ where
+ M: CommandToGsp,
+ M::Reply: MessageFromGsp,
+ Error: From<M::InitError>,
+ Error: From<<M::Reply as MessageFromGsp>::InitError>,
+ {
+ let mut inner = self.inner.lock();
+ inner.send_command(bar, command)?;
+
+ loop {
+ match inner.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) {
+ Ok(reply) => break Ok(reply),
+ Err(ERANGE) => continue,
+ Err(e) => break Err(e),
+ }
+ }
+ }
+
+ /// Sends `command` to the GSP without waiting for a reply.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command_no_wait<M>(&self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp<Reply = NoReply>,
+ Error: From<M::InitError>,
+ {
+ self.inner.lock().send_command(bar, command)
+ }
+
+ /// Receive a message from the GSP.
+ ///
+ /// See [`CmdqInner::receive_msg`] for details.
+ pub(crate) fn receive_msg<M: MessageFromGsp>(&self, timeout: Delta) -> Result<M>
+ where
+ // This allows all error types, including `Infallible`, to be used for `M::InitError`.
+ Error: From<M::InitError>,
+ {
+ 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`].
+struct CmdqInner {
+ /// Device this command queue belongs to.
+ dev: ARef<device::Device>,
+ /// Current command sequence number.
+ seq: u32,
+ /// Memory area shared with the GSP for communicating commands and messages.
+ gsp_mem: DmaGspMem,
+}
+
+impl CmdqInner {
+ /// Timeout for waiting for space on the command queue.
+ const ALLOCATE_TIMEOUT: Delta = Delta::from_secs(1);
+
/// Sends `command` to the GSP, without splitting it.
///
/// # Errors
@@ -588,7 +671,7 @@ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- fn send_command_internal<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
where
M: CommandToGsp,
Error: From<M::InitError>,
@@ -608,54 +691,6 @@ fn send_command_internal<M>(&mut self, bar: &Bar0, command: M) -> Result
}
}
- /// Sends `command` to the GSP and waits for the reply.
- ///
- /// Messages with non-matching function codes are silently consumed until the expected reply
- /// arrives.
- ///
- /// # Errors
- ///
- /// - `ETIMEDOUT` if space does not become available to send the command, or if the reply is
- /// not received within the timeout.
- /// - `EIO` if the variable payload requested by the command has not been entirely
- /// written to by its [`CommandToGsp::init_variable_payload`] method.
- ///
- /// Error codes returned by the command and reply initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result<M::Reply>
- where
- M: CommandToGsp,
- M::Reply: MessageFromGsp,
- Error: From<M::InitError>,
- Error: From<<M::Reply as MessageFromGsp>::InitError>,
- {
- self.send_command_internal(bar, command)?;
-
- loop {
- match self.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) {
- Ok(reply) => break Ok(reply),
- Err(ERANGE) => continue,
- Err(e) => break Err(e),
- }
- }
- }
-
- /// Sends `command` to the GSP without waiting for a reply.
- ///
- /// # Errors
- ///
- /// - `ETIMEDOUT` if space does not become available within the timeout.
- /// - `EIO` if the variable payload requested by the command has not been entirely
- /// written to by its [`CommandToGsp::init_variable_payload`] method.
- ///
- /// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command_no_wait<M>(&mut self, bar: &Bar0, command: M) -> Result
- where
- M: CommandToGsp<Reply = NoReply>,
- Error: From<M::InitError>,
- {
- self.send_command_internal(bar, command)
- }
-
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
@@ -691,7 +726,7 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
let (header, slice_1) = GspMsgElement::from_bytes_prefix(slice_1).ok_or(EIO)?;
dev_dbg!(
- self.dev,
+ &self.dev,
"GSP RPC: receive: seq# {}, function={:?}, length=0x{:x}\n",
header.sequence(),
header.function(),
@@ -726,7 +761,7 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
])) != 0
{
dev_err!(
- self.dev,
+ &self.dev,
"GSP RPC: receive: Call {} - bad checksum\n",
header.sequence()
);
@@ -755,7 +790,7 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
/// - `ERANGE` if the message had a recognized but non-matching function code.
///
/// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
- pub(crate) fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
+ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
where
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
Error: From<M::InitError>,
@@ -791,9 +826,4 @@ pub(crate) fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Resul
result
}
-
- /// Returns the DMA handle of the command queue's shared memory region.
- pub(crate) fn dma_handle(&self) -> DmaAddress {
- self.gsp_mem.0.dma_handle()
- }
}
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 77054c92fcc2..c89c7b57a751 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -165,7 +165,7 @@ fn read(
}
/// Waits for GSP initialization to complete.
-pub(crate) fn wait_gsp_init_done(cmdq: &mut Cmdq) -> Result {
+pub(crate) fn wait_gsp_init_done(cmdq: &Cmdq) -> Result {
loop {
match cmdq.receive_msg::<GspInitDone>(Cmdq::RECEIVE_TIMEOUT) {
Ok(_) => break Ok(()),
@@ -234,6 +234,6 @@ pub(crate) fn gpu_name(&self) -> core::result::Result<&str, GpuNameError> {
}
/// Send the [`GetGspInfo`] command and awaits for its reply.
-pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticInfoReply> {
+pub(crate) fn get_gsp_info(cmdq: &Cmdq, bar: &Bar0) -> Result<GetGspStaticInfoReply> {
cmdq.send_command(bar, GetGspStaticInfo)
}
diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
index ce2b3bb05d22..474e4c8021db 100644
--- a/drivers/gpu/nova-core/gsp/sequencer.rs
+++ b/drivers/gpu/nova-core/gsp/sequencer.rs
@@ -356,7 +356,7 @@ pub(crate) struct GspSequencerParams<'a> {
}
impl<'a> GspSequencer<'a> {
- pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
+ pub(crate) fn run(cmdq: &Cmdq, params: GspSequencerParams<'a>) -> Result {
let seq_info = loop {
match cmdq.receive_msg::<GspSequence>(Cmdq::RECEIVE_TIMEOUT) {
Ok(seq_info) => break seq_info,
--
2.53.0On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
<snip>
> + /// 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()
> + }
Just noticed that we now need to lock to get the DMA handle. It's
inconsequential in practice but a bit inelegant.
Since the DMA handle never changes, and is only ever needed during
initialization, I think I will just insert a patch before this one that
adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
to obtain the handle at construction time and can get rid of this
method, which keeps the public API focused on message handling.
No need to resend, I will apply this patch on top of mine and merge.
On Thu Mar 19, 2026 at 12:27 AM JST, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
> <snip>
>> + /// 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()
>> + }
>
> Just noticed that we now need to lock to get the DMA handle. It's
> inconsequential in practice but a bit inelegant.
>
> Since the DMA handle never changes, and is only ever needed during
> initialization, I think I will just insert a patch before this one that
> adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
> to obtain the handle at construction time and can get rid of this
> method, which keeps the public API focused on message handling.
>
> No need to resend, I will apply this patch on top of mine and merge.
Yerp it's not ideal, but we only call this method in one location, once,
when creating the command queue. The locking in this series is very
basic and we probably want to improve it later to be more performant, so
I think it's simpler and better to just leave this as is for now.
On Thu Mar 19, 2026 at 12:27 AM JST, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
> <snip>
>> + /// 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()
>> + }
>
> Just noticed that we now need to lock to get the DMA handle. It's
> inconsequential in practice but a bit inelegant.
>
> Since the DMA handle never changes, and is only ever needed during
> initialization, I think I will just insert a patch before this one that
> adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
> to obtain the handle at construction time and can get rid of this
> method, which keeps the public API focused on message handling.
>
> No need to resend, I will apply this patch on top of mine and merge.
Actually let's fix that afterwards, what I proposed above would
introduce code that hasn't undergone review, and doesn't reduce the
number of patches in the series so doesn't bring any benefit. I'll merge
the series as-is for now.
On 3/18/26 4:21 PM, Alexandre Courbot wrote:
> On Thu Mar 19, 2026 at 12:27 AM JST, Alexandre Courbot wrote:
>> On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
>> <snip>
>>> + /// 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()
>>> + }
>>
>> Just noticed that we now need to lock to get the DMA handle. It's
>> inconsequential in practice but a bit inelegant.
>>
>> Since the DMA handle never changes, and is only ever needed during
>> initialization, I think I will just insert a patch before this one that
>> adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
>> to obtain the handle at construction time and can get rid of this
>> method, which keeps the public API focused on message handling.
>>
>> No need to resend, I will apply this patch on top of mine and merge.
>
> Actually let's fix that afterwards, what I proposed above would
> introduce code that hasn't undergone review, and doesn't reduce the
> number of patches in the series so doesn't bring any benefit. I'll merge
> the series as-is for now.
For what it's worth, this avoided an email from me, to the effect of
"there is no such thing as a quick locking fixup, don't do this".
So, good. :)
thanks,
--
John Hubbard
On Wed Mar 18, 2026 at 4:27 PM CET, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
> <snip>
>> + /// 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()
>> + }
>
> Just noticed that we now need to lock to get the DMA handle. It's
> inconsequential in practice but a bit inelegant.
>
> Since the DMA handle never changes, and is only ever needed during
> initialization, I think I will just insert a patch before this one that
> adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
> to obtain the handle at construction time and can get rid of this
> method, which keeps the public API focused on message handling.
>
> No need to resend, I will apply this patch on top of mine and merge.
It is a minor inconvinience, but it may indicate that dma::Coherent should
probaly support locking, i.e. you want to protect the data within the
dma::Coherent allocation, not the dma::Coherent object itself.
On Thu Mar 19, 2026 at 12:40 AM JST, Danilo Krummrich wrote:
> On Wed Mar 18, 2026 at 4:27 PM CET, Alexandre Courbot wrote:
>> On Wed Mar 18, 2026 at 1:07 PM JST, Eliot Courtney wrote:
>> <snip>
>>> + /// 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()
>>> + }
>>
>> Just noticed that we now need to lock to get the DMA handle. It's
>> inconsequential in practice but a bit inelegant.
>>
>> Since the DMA handle never changes, and is only ever needed during
>> initialization, I think I will just insert a patch before this one that
>> adds a `pub(super) dma_handle` member to `Cmdq`. That way we only need
>> to obtain the handle at construction time and can get rid of this
>> method, which keeps the public API focused on message handling.
>>
>> No need to resend, I will apply this patch on top of mine and merge.
>
> It is a minor inconvinience, but it may indicate that dma::Coherent should
> probaly support locking, i.e. you want to protect the data within the
> dma::Coherent allocation, not the dma::Coherent object itself.
Not quite sure I understand what you mean here - can you elaborate? This
sounds to me like you want to add a `Mutex` to every `dma::Coherent`, so
I am likely misunderstanding. :)
© 2016 - 2026 Red Hat, Inc.