Re: [PATCH] gpu: nova-core: fix packed registry table size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing memory barrier before write pointer update allows GSP to read stale command data.
- [High] Unrelated asynchronous messages are silently dropped while waiting for a command reply or initialization event.
--

--- Patch [1]: [PATCH] gpu: nova-core: fix packed registry table size ---
commit 7a27da8622da09265a549e78737e3707931209c2
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: fix packed registry table size

PACKED_REGISTRY_TABLE::size describes the entire table, including its
fixed-size header. SetRegistry currently initializes it with only the
variable payload length, omitting the 8 bytes header.

Fix this by using CommandToGsp::size to obtain the actual command
size, including its header.

Fixes: 19b0a6e7c2be ("gpu: nova-core: gsp: Add SetRegistry command")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/20260722075253.B6DDB1F00A3D@smtp.kernel.org
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 08380de39048a..0f8bd77acddf7 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -115,10 +115,7 @@ impl CommandToGsp for SetRegistry {
>      type InitError = Infallible;
>  
>      fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> -        Self::Command::init(
> -            self.entries.len() as u32,
> -            self.variable_payload_len() as u32,
> -        )
> +        Self::Command::init(self.entries.len() as u32, self.size() as u32)
>      }
>  
>      fn variable_payload_len(&self) -> usize {

[Severity: High]
This isn't a bug introduced by this patch, but looking at advance_cpu_write_ptr()
in drivers/gpu/nova-core/gsp/cmdq.rs:

drivers/gpu/nova-core/gsp/cmdq.rs:advance_cpu_write_ptr() {
        MsgqTxHeader::set_write_ptr(tx, wptr);

        // Ensure all command data is visible before triggering the GSP read.
        fence(Ordering::SeqCst);
}

Does the fence(Ordering::SeqCst) need to be placed before set_write_ptr()?

Since it is executed after the write pointer update, could a weakly ordered
architecture reorder the payload stores to become globally visible after the
pointer update, allowing the GSP to read stale or zeroed command data?

[Severity: High]
This is a pre-existing issue, but in drivers/gpu/nova-core/gsp/cmdq.rs,
receive_msg() unconditionally advances the read pointer even when returning
ERANGE for an unmatched message:

drivers/gpu/nova-core/gsp/cmdq.rs:receive_msg() {
        ...
        } else {
            Err(ERANGE)
        };

        // Advance the read pointer past this message.
        self.gsp_mem.advance_cpu_read_ptr(u32::try_from(
            message.header.length().div_ceil(GSP_PAGE_SIZE),
        )?);

        result
}

When this ERANGE is caught in send_command() or wait_gsp_init_done(), the
loop simply continues:

drivers/gpu/nova-core/gsp/cmdq.rs:send_command() {
        loop {
            match inner.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) {
                Ok(reply) => break Ok(reply),
                Err(ERANGE) => continue,
                Err(e) => break Err(e),
            }
        }
}

Will this permanently discard unrelated asynchronous messages (like GPU
thermal alerts or hardware faults) arriving from the GSP during this wait,
rather than processing or queuing them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-nova-registry-size-fix-v1-1-8f471ba00ab4@nvidia.com?part=1