Re: [PATCH v2 4/4] gpu: nova-core: gsp: add mutex locking to Cmdq

["Gary Guo" <gary@garyguo.net>]

On Thu Feb 26, 2026 at 2:50 PM GMT, Eliot Courtney wrote:
> 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_sync_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>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/boot.rs      |   8 +-
>  drivers/gpu/nova-core/gsp/cmdq.rs      | 266 ++++++++++++++++++---------------
>  drivers/gpu/nova-core/gsp/commands.rs  |   4 +-
>  drivers/gpu/nova-core/gsp/sequencer.rs |   2 +-
>  4 files changed, 153 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index 55899eba75db..d12ad1bd2cd8 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -128,7 +128,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,
> @@ -214,13 +214,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 6bb1decd2af5..5010587c96f9 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -16,8 +16,12 @@
>      },
>      dma_write,
>      io::poll::read_poll_timeout,
> +    new_mutex,
>      prelude::*,
> -    sync::aref::ARef,
> +    sync::{
> +        aref::ARef,
> +        Mutex, //
> +    },
>      time::Delta,
>      transmute::{
>          AsBytes,
> @@ -54,8 +58,8 @@
>  
>  /// Trait implemented by types representing a command to send to the GSP.
>  ///
> -/// The main purpose of this trait is to provide [`Cmdq::send_command`] with the information it
> -/// needs to send a given command.
> +/// The main purpose of this trait is to provide [`Cmdq`] with the information it needs to send
> +/// a given command.
>  ///
>  /// [`CommandToGsp::init`] in particular is responsible for initializing the command directly
>  /// into the space reserved for it in the command queue buffer.
> @@ -470,66 +474,15 @@ pub(crate) fn command_size<M>(command: &M) -> usize
>      size_of::<M::Command>() + command.variable_payload_len()
>  }
>  
> -/// GSP command queue.
> -///
> -/// Provides the ability to send commands and receive messages from the GSP using a shared memory
> -/// area.
> -#[pin_data]
> -pub(crate) struct Cmdq {
> -    /// Device this command queue belongs to.
> -    dev: ARef<device::Device>,
> +/// Inner mutex protected state of [`Cmdq`].
> +struct CmdqInner {
>      /// Current command sequence number.
>      seq: u32,
>      /// Memory area shared with the GSP for communicating commands and messages.
>      gsp_mem: DmaGspMem,
>  }
>  
> -impl Cmdq {
> -    /// Offset of the data after the PTEs.
> -    const POST_PTE_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq);
> -
> -    /// Offset of command queue ring buffer.
> -    pub(crate) const CMDQ_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq)
> -        + core::mem::offset_of!(Msgq, msgq)
> -        - Self::POST_PTE_OFFSET;
> -
> -    /// Offset of message queue ring buffer.
> -    pub(crate) const STATQ_OFFSET: usize = core::mem::offset_of!(GspMem, gspq)
> -        + core::mem::offset_of!(Msgq, msgq)
> -        - Self::POST_PTE_OFFSET;
> -
> -    /// Number of page table entries for the GSP shared region.
> -    pub(crate) const NUM_PTES: usize = size_of::<GspMem>() >> GSP_PAGE_SHIFT;
> -
> -    /// 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,
> -        })
> -    }
> -
> -    /// Computes the checksum for the message pointed to by `it`.
> -    ///
> -    /// A message is made of several parts, so `it` is an iterator over byte slices representing
> -    /// these parts.
> -    fn calculate_checksum<T: Iterator<Item = u8>>(it: T) -> u32 {
> -        let sum64 = it
> -            .enumerate()
> -            .map(|(idx, byte)| (((idx % 8) * 8) as u32, byte))
> -            .fold(0, |acc, (rol, byte)| acc ^ u64::from(byte).rotate_left(rol));
> -
> -        ((sum64 >> 32) as u32) ^ (sum64 as u32)
> -    }
> -
> -    /// Notifies the GSP that we have updated the command queue pointers.
> -    fn notify_gsp(bar: &Bar0) {
> -        regs::NV_PGSP_QUEUE_HEAD::default()
> -            .set_address(0)
> -            .write(bar);
> -    }
> -
> +impl CmdqInner {
>      /// Sends `command` to the GSP, without splitting it.
>      ///
>      /// # Errors
> @@ -540,7 +493,7 @@ fn notify_gsp(bar: &Bar0) {
>      ///   written to by its [`CommandToGsp::init_variable_payload`] method.
>      ///
>      /// Error codes returned by the command initializers are propagated as-is.
> -    fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> +    fn send_single_command<M>(&mut self, dev: &device::Device, bar: &Bar0, command: M) -> Result

Any reason that the `dev` is passed in everything instead of just have it be
part of `CmdqInner`?

It appears that the `Cmdq` methods don't actually use it apart from passing it
to `CmdqInner`.

Best,
Gary

>      where
>          M: CommandToGsp,
>          // This allows all error types, including `Infallible`, to be used for `M::InitError`.
> @@ -583,7 +536,7 @@ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>              ])));
>  
>          dev_dbg!(
> -            &self.dev,
> +            dev,
>              "GSP RPC: send: seq# {}, function={:?}, length=0x{:x}\n",
>              self.seq,
>              M::FUNCTION,