[PATCH v4 10/17] gpu: nova-core: stop re-parsing a bad GSP message

[John Hubbard]

A GSP message carries its length inside the checksummed region. Once the
framing or the checksum fails, there is no trustworthy length with which
to skip the message.

Two failures left a bad message at the queue head. A framing or checksum
failure returned without advancing the read pointer, so every later
receive parsed the same message again. A validly framed message whose
typed payload failed to decode returned early and did the same.

Poison the queue on a framing or checksum failure: log what was
inconsistent and fail every later receive, so the bad message is parsed
once and recovery takes a device reset. Advance the read pointer past a
validly framed message whether or not its payload decodes, and warn when
the payload is shorter than the type it decodes into.

Assisted-by: LLM
Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 93 +++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 340760e384d0..855c5a708525 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -2,7 +2,10 @@
 
 mod continuation;
 
-use core::mem;
+use core::{
+    cell::Cell,
+    mem, //
+};
 
 use kernel::{
     device,
@@ -11,6 +14,7 @@
         CoherentBox,
         DmaAddress, //
     },
+    fmt,
     io::{
         io_project,
         poll::read_poll_timeout,
@@ -532,6 +536,7 @@ pub(crate) fn new(
                     dev,
                     gsp_mem,
                     seq: 0,
+                    poisoned: Cell::new(false),
                 }),
             }))
         })
@@ -624,6 +629,12 @@ struct CmdqInner<'a> {
     dev: &'a device::Device,
     /// Current command sequence number.
     seq: u32,
+    /// Set once a message fails framing or checksum validation. Every later receive fails, since
+    /// the bad message cannot be skipped. See "Draining the GSP-to-CPU queue" in
+    /// `Documentation/gpu/nova/core/interrupts.rst`.
+    ///
+    /// A [`Cell`] because [`Self::wait_for_msg`] sets it through `&self`.
+    poisoned: Cell<bool>,
     /// Memory area shared with the GSP for communicating commands and messages.
     gsp_mem: DmaGspMem<'a>,
 }
@@ -732,6 +743,14 @@ fn send_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
         }
     }
 
+    /// Logs `reason`, poisons the queue, and returns `EIO` for the caller to propagate.
+    fn poison(&self, reason: fmt::Arguments<'_>) -> Error {
+        dev_err!(&self.dev, "GSP RPC: receive: queue poisoned: {}\n", reason);
+        self.poisoned.set(true);
+
+        EIO
+    }
+
     /// 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
@@ -746,11 +765,13 @@ fn send_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
     /// # Errors
     ///
     /// - `ETIMEDOUT` if `timeout` has elapsed before any message becomes available.
-    /// - `EIO` if there was some inconsistency (e.g. message shorter than advertised) on the
-    ///   message queue.
-    ///
-    /// Error codes returned by the message constructor are propagated as-is.
+    /// - `EIO` if the queue is already poisoned, or if the framing or the checksum is invalid,
+    ///   which poisons it (see [`Self::poisoned`]).
     fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
+        if self.poisoned.get() {
+            return Err(EIO);
+        }
+
         // Wait for a message to arrive from the GSP.
         let (slice_1, slice_2) = read_poll_timeout(
             || Ok(self.gsp_mem.driver_read_area()),
@@ -761,7 +782,12 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
         .map(|(slice_1, slice_2)| (slice_1.as_flattened(), slice_2.as_flattened()))?;
 
         // Extract the `GspMsgElement`.
-        let (header, slice_1) = GspMsgElement::from_bytes_prefix(slice_1).ok_or(EIO)?;
+        let Some((header, slice_1)) = GspMsgElement::from_bytes_prefix(slice_1) else {
+            return Err(self.poison(fmt!(
+                "read area of {} bytes is shorter than a message header",
+                slice_1.len()
+            )));
+        };
 
         dev_dbg!(
             &self.dev,
@@ -775,7 +801,11 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
 
         // Check that the driver read area is large enough for the message.
         if slice_1.len() + slice_2.len() < payload_length {
-            return Err(EIO);
+            return Err(self.poison(fmt!(
+                "message advertises {} payload bytes but only {} are readable",
+                payload_length,
+                slice_1.len() + slice_2.len()
+            )));
         }
 
         // Cut the message slices down to the actual length of the message.
@@ -798,12 +828,10 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
             slice_2,
         ])) != 0
         {
-            dev_err!(
-                &self.dev,
-                "GSP RPC: receive: Call {} - bad checksum\n",
+            return Err(self.poison(fmt!(
+                "message with sequence {} has a bad checksum",
                 header.sequence()
-            );
-            return Err(EIO);
+            )));
         }
 
         Ok(GspMessage {
@@ -817,13 +845,13 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
     /// A message whose function code is `M::FUNCTION` is decoded and returned. Any other message
     /// is logged as an event.
     ///
-    /// The read pointer is always advanced past the message, regardless of whether it matched.
+    /// The read pointer advances past the message in every case, including a decode failure.
     ///
     /// # Errors
     ///
     /// - `ETIMEDOUT` if `timeout` has elapsed before any message becomes available.
-    /// - `EIO` if there was some inconsistency (e.g. message shorter than advertised) on the
-    ///   message queue.
+    /// - `EIO` if the queue is poisoned or the message fails framing or checksum validation (see
+    ///   [`Self::wait_for_msg`]), or if the matched message is too short for `M::Message`.
     /// - `ERANGE` if the message was not the awaited reply.
     ///
     /// Error codes returned by [`MessageFromGsp::read`] are propagated as-is.
@@ -838,20 +866,27 @@ fn receive_msg<M: MessageFromGsp>(&mut self, timeout: Delta) -> Result<M>
 
         // An early return here would leave the read pointer on this message.
         let result = if matches!(function, Ok(f) if f == M::FUNCTION) {
-            let (cmd, contents_1) = M::Message::from_bytes_prefix(message.contents.0).ok_or(EIO)?;
-            let mut sbuffer = SBufferIter::new_reader([contents_1, message.contents.1]);
-
-            M::read(cmd, &mut sbuffer)
-                .map_err(|e| e.into())
-                .inspect(|_| {
-                    if !sbuffer.is_empty() {
-                        dev_warn!(
-                            &self.dev,
-                            "GSP message {:?} has unprocessed data\n",
-                            M::FUNCTION
-                        );
-                    }
-                })
+            match M::Message::from_bytes_prefix(message.contents.0) {
+                Some((cmd, contents_1)) => {
+                    let mut sbuffer = SBufferIter::new_reader([contents_1, message.contents.1]);
+
+                    M::read(cmd, &mut sbuffer)
+                        .map_err(|e| e.into())
+                        .inspect(|_| {
+                            if !sbuffer.is_empty() {
+                                dev_warn!(
+                                    &self.dev,
+                                    "GSP message {:?} has unprocessed data\n",
+                                    M::FUNCTION
+                                );
+                            }
+                        })
+                }
+                None => {
+                    dev_warn!(&self.dev, "GSP message {:?} too short\n", M::FUNCTION);
+                    Err(EIO)
+                }
+            }
         } else {
             self.log_event(function, seq);
 
-- 
2.55.0