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

[Zhi Wang]

On Wed, 25 Feb 2026 22:41:51 +0900
Eliot Courtney <ecourtney@xxxxxxxxxx> wrote:

Looks good to me.

Reviewed-by: Zhi Wang <zhiw@xxxxxxxxxx>

> 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.
> 
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/gsp/boot.rs      |   8 +-
>  drivers/gpu/nova-core/gsp/cmdq.rs      | 260
> ++++++++++++++++++---------------
> drivers/gpu/nova-core/gsp/commands.rs  |   4 +-
> drivers/gpu/nova-core/gsp/sequencer.rs |   2 +- 4 files changed, 152
> insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs
> b/drivers/gpu/nova-core/gsp/boot.rs index 1cb21da855b9..cb583f57666a
> 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,
> @@ -232,13 +232,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.as_ref(), "GPU name: {}\n",
> name), Err(e) => dev_warn!(pdev.as_ref(), "GPU name unavailable:
> {:?}\n", e), diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs
> b/drivers/gpu/nova-core/gsp/cmdq.rs index 44c3e960c965..faf1e9d5072b
> 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 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,
> @@ -610,73 +563,27 @@ 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<M>(&mut self, bar: &Bar0, command: M) -> Result
> +    fn send_command<M>(&mut self, dev: &device::Device, bar: &Bar0,
> command: M) -> Result where
>          M: CommandToGsp,
>          Error: From<M::InitError>,
>      {
>          let mut state = SplitState::new(&command)?;
> -
> -        self.send_single_command(bar, state.command(command))?;
> +        self.send_single_command(dev, bar, state.command(command))?;
>  
>          while let Some(continuation) =
> state.next_continuation_record() { dev_dbg!(
> -                &self.dev,
> +                dev,
>                  "GSP RPC: send continuation: size=0x{:x}\n",
>                  command_size(&continuation),
>              );
>              // Turbofish needed because the compiler cannot infer M
> here.
> -            self.send_single_command::<ContinuationRecord<'_>>(bar,
> continuation)?;
> +            self.send_single_command::<ContinuationRecord<'_>>(dev,
> bar, continuation)?; }
>  
>          Ok(())
>      }
>  
> -    /// Sends `command` to the GSP and waits for the reply.
> -    ///
> -    /// # 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_sync_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(bar, command)?;
> -
> -        loop {
> -            match self.receive_msg::<M::Reply>(Delta::from_secs(10))
> {
> -                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_async_command<M>(&mut self, bar: &Bar0,
> command: M) -> Result
> -    where
> -        M: CommandToGsp<Reply = NoReply>,
> -        Error: From<M::InitError>,
> -    {
> -        self.send_command(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 @@ -695,7 +602,7 @@ pub(crate) fn
> send_async_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> ///   message queue. ///
>      /// Error codes returned by the message constructor are
> propagated as-is.
> -    fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>>
> {
> +    fn wait_for_msg(&self, dev: &device::Device, timeout: Delta) ->
> Result<GspMessage<'_>> { // Wait for a message to arrive from the GSP.
>          let (slice_1, slice_2) = read_poll_timeout(
>              || Ok(self.gsp_mem.driver_read_area()),
> @@ -712,7 +619,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,
> +            dev,
>              "GSP RPC: receive: seq# {}, function={:?},
> length=0x{:x}\n", header.sequence(),
>              header.function(),
> @@ -747,7 +654,7 @@ fn wait_for_msg(&self, timeout: Delta) ->
> Result<GspMessage<'_>> { ])) != 0
>          {
>              dev_err!(
> -                self.dev,
> +                dev,
>                  "GSP RPC: receive: Call {} - bad checksum\n",
>                  header.sequence()
>              );
> @@ -776,12 +683,12 @@ 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, dev:
> &device::Device, timeout: Delta) -> Result<M> where
>          // This allows all error types, including `Infallible`, to
> be used for `M::InitError`. Error: From<M::InitError>,
>      {
> -        let message = self.wait_for_msg(timeout)?;
> +        let message = self.wait_for_msg(dev, timeout)?;
>          let function = message.header.function().map_err(|_|
> EINVAL)?; 
>          // Extract the message. Store the result as we want to
> advance the read pointer even in @@ -802,9 +709,132 @@ pub(crate) fn
> receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Resul 
>          result
>      }
> +}
> +
> +/// 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.
> +    #[pin]
> +    inner: Mutex<CmdqInner>,
> +}
> +
> +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 {
> +            inner <- new_mutex!(CmdqInner {
> +                gsp_mem: DmaGspMem::new(dev)?,
> +                seq: 0,
> +                }),
> +            dev: dev.into(),
> +        })
> +    }
> +
> +    /// 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);
> +    }
> +
> +    /// Sends `command` to the GSP and waits for the reply.
> +    ///
> +    /// The mutex is held for the entire send+receive cycle to
> ensure that no other command can
> +    /// be interleaved. 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_sync_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(&self.dev, bar, command)?;
> +
> +        loop {
> +            match inner.receive_msg::<M::Reply>(&self.dev,
> Delta::from_secs(10)) {
> +                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_async_command<M>(&self, bar: &Bar0, command:
> M) -> Result
> +    where
> +        M: CommandToGsp<Reply = NoReply>,
> +        Error: From<M::InitError>,
> +    {
> +        self.inner.lock().send_command(&self.dev, 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(&self.dev, timeout)
> +    }
>  
>      /// 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()
> +        self.inner.lock().gsp_mem.0.dma_handle()
>      }
>  }
> diff --git a/drivers/gpu/nova-core/gsp/commands.rs
> b/drivers/gpu/nova-core/gsp/commands.rs index
> b42e32dcc55c..7ceff2e6bd63 100644 ---
> a/drivers/gpu/nova-core/gsp/commands.rs +++
> b/drivers/gpu/nova-core/gsp/commands.rs @@ -170,7 +170,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>(Delta::from_secs(10)) {
>              Ok(_) => break Ok(()),
> @@ -239,7 +239,7 @@ 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_sync_command(bar, GetGspStaticInfo) }
>  
> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs
> b/drivers/gpu/nova-core/gsp/sequencer.rs index
> 2fa2a0792cec..bc94b8567c6a 100644 ---
> a/drivers/gpu/nova-core/gsp/sequencer.rs +++
> b/drivers/gpu/nova-core/gsp/sequencer.rs @@ -366,7 +366,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>(Delta::from_secs(10)) { Ok(seq_info)
> => break seq_info,
>