gpu: nova-core: gsp: fix command queue ring buffer bugs

Disambiguate a few things in comments in cmdq.rs.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +4 -2

index f139aad7af3f..09c28eeb6f12 100644
@@ -161,12 +161,14 @@ struct GspMem {
     /// Self-mapping page table entries.
     ptes: PteArray<{ GSP_PAGE_SIZE / size_of::<u64>() }>,
     /// CPU queue: the driver writes commands here, and the GSP reads them. It also contains the
-    /// write and read pointers that the CPU updates.
+    /// write and read pointers that the CPU updates. This means that the read pointer here is an
+    /// index into the GSP queue.
     ///
     /// This member is read-only for the GSP.
     cpuq: Msgq,
     /// GSP queue: the GSP writes messages here, and the driver reads them. It also contains the
-    /// write and read pointers that the GSP updates.
+    /// write and read pointers that the GSP updates. This means that the read pointer here is an
+    /// index into the CPU queue.
     ///
     /// This member is read-only for the driver.
     gspq: Msgq,

drivers/gpu/nova-core/gsp/cmdq.rs +1 -1

         let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
 
         if rx <= tx {

@@ -257,7 +259,7 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // - We will only access the driver-owned part of the shared memory.
         // - Per the safety statement of the function, no concurrent access will be performed.
         let gsp_mem = &unsafe { self.0.as_slice(0, 1) }.unwrap()[0];
-        // PANIC: per the invariant of `cpu_read_ptr`, `xx` is `<= MSGQ_NUM_PAGES`.
+        // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
         let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);
 
         match tx.cmp(&rx) {

drivers/gpu/nova-core/gsp/cmdq.rs +3 -3

     fn gsp_write_ptr(&self) -> u32 {
         let gsp_mem = self.0.start_ptr();
 

@@ -329,7 +331,7 @@ fn gsp_write_ptr(&self) -> u32 {
     //
     // # Invariants
     //
-    // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
+    // - The returned value is between `0` and `MSGQ_NUM_PAGES - 1`, inclusive.
     fn gsp_read_ptr(&self) -> u32 {
         let gsp_mem = self.0.start_ptr();
 

@@ -343,7 +345,7 @@ fn gsp_read_ptr(&self) -> u32 {
     //
     // # Invariants
     //
-    // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
+    // - The returned value is between `0` and `MSGQ_NUM_PAGES - 1`, inclusive.
     fn cpu_read_ptr(&self) -> u32 {
         let gsp_mem = self.0.start_ptr();
 

@@ -372,7 +374,7 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
     //
     // # Invariants
     //
-    // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
+    // - The returned value is between `0` and `MSGQ_NUM_PAGES - 1`, inclusive.
     fn cpu_write_ptr(&self) -> u32 {
         let gsp_mem = self.0.start_ptr();
 

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 227

Quoted 5 lines

         // - We will only access the driver-owned part of the shared memory.
         // - Per the safety statement of the function, no concurrent access will be performed.
         let gsp_mem = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];

drivers/gpu/nova-core/gsp/cmdq.rs +1 -1

-        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `<= MSGQ_NUM_PAGES`.
+        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.

Can this just be tx < MSGQ_NUM_PAGES?

Review

Quoted 4 lines

     // # Invariants
     //

(quoted diff) +1 -1

-    // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
+    // - The returned value is between `0` and `MSGQ_NUM_PAGES - 1`, inclusive.

I wonder if this can be is within 0..MSGQ_NUM_PAGES. What do others think?

Quoted 6 lines · Gary Guo

Quoted 5 lines

         // - We will only access the driver-owned part of the shared memory.
         // - Per the safety statement of the function, no concurrent access will be performed.
         let gsp_mem = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];

drivers/gpu/nova-core/gsp/cmdq.rs +1 -1

-        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `<= MSGQ_NUM_PAGES`.
+        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.

Quoted 1 line

Can this just be tx < MSGQ_NUM_PAGES?

In previous discussion[1] it's been noted that it's important to explain why the preconditions are satisfied, not just what they are, so that's the reason I kept this in. Happy to hear arguments either way though.

[1]: https://lore.kernel.org/all/CAH5fLgg0O=t2T2MQH2hvm4eZnCOa_NffzRw=XZPi9+7+=XsmRg@mail.gmail.com/

Quoted 6 lines

Quoted 5 lines

     // # Invariants
     //

(quoted diff) +1 -1

-    // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
+    // - The returned value is between `0` and `MSGQ_NUM_PAGES - 1`, inclusive.

Quoted 1 line

I wonder if this can be is within 0..MSGQ_NUM_PAGES. What do others think?

I think this is very reasonable, since this is part of the rust range syntax so it should be understandable. I also considered the mathematical syntax [0, MSGQ_NUM_PAGES), but not sure if this would be conventional - it does seem that this notation is used in a bunch of places though. Will apply your suggestion in the next version unless there is a definitive convention for this.

Quoted 9 lines · Eliot Courtney

Quoted 1 line · Gary Guo

I wonder if this can be is within 0..MSGQ_NUM_PAGES. What do others think?

I think this is very reasonable, since this is part of the rust range syntax so it should be understandable. I also considered the mathematical syntax [0, MSGQ_NUM_PAGES), but not sure if this would be conventional - it does seem that this notation is used in a bunch of places though. Will apply your suggestion in the next version unless there is a definitive convention for this.

Since this is Rust code, the Rust syntax to express ranges (within `` quotes) makes sense IMHO.

The current code hands out buffers that go all the way up to and including rx - 1, but we need to maintain an empty slot to prevent the ring buffer from wrapping around into having 'tx == rx', which means empty.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +0 -5

index 09c28eeb6f12..aa8758fc7723 100644
@@ -227,21 +227,26 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
         let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
 
-        if rx <= tx {
-            // The area from `tx` up to the end of the ring, and from the beginning of the ring up
-            // to `rx`, minus one unit, belongs to the driver.
-            if rx == 0 {
-                let last = after_tx.len() - 1;

drivers/gpu/nova-core/gsp/cmdq.rs +7 -1

+        } else if rx > tx {
+            // The area is contiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
+            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
+            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
+            (&mut after_tx[..(rx - tx - 1)], &mut before_tx[0..0])
         } else {
-            // The area from `tx` to `rx`, minus one unit, belongs to the driver.

drivers/gpu/nova-core/gsp/cmdq.rs +0 -0

         }
     }
 

Quoted 31 lines · Eliot Courtney

The current code hands out buffers that go all the way up to and including rx - 1, but we need to maintain an empty slot to prevent the ring buffer from wrapping around into having 'tx == rx', which means empty.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +1 -9

index 09c28eeb6f12..aa8758fc7723 100644
@@ -227,21 +227,26 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
         let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
 
-        if rx <= tx {
-            // The area from `tx` up to the end of the ring, and from the beginning of the ring up
-            // to `rx`, minus one unit, belongs to the driver.
-            if rx == 0 {
-                let last = after_tx.len() - 1;
-                (&mut after_tx[..last], &mut before_tx[0..0])
-            } else {
-                (after_tx, &mut before_tx[..rx])
-            }
+        // The area starting at `tx` and ending at `rx - 2` modulo MSGQ_NUM_PAGES, inclusive,

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 235

Quoted 5 lines

drivers/gpu/nova-core/gsp/cmdq.rs +5 -0

+        // belongs to the driver for writing.
+        if rx == 0 {
+            // Since `rx` is zero, leave an empty slot at end of the buffer.
+            let last = after_tx.len() - 1;
+            (&mut after_tx[..last], &mut before_tx[0..0])

Does the address actually matter? Otherwise I would find &mut [] easier to understand than an empty indexing.

Quoted 10 lines

drivers/gpu/nova-core/gsp/cmdq.rs +7 -1

+        } else if rx > tx {
+            // The area is contiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
+            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
+            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
+            (&mut after_tx[..(rx - tx - 1)], &mut before_tx[0..0])
         } else {
-            // The area from `tx` to `rx`, minus one unit, belongs to the driver.

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 249

Quoted 10 lines

drivers/gpu/nova-core/gsp/cmdq.rs +6 -4

-            //
-            // PANIC: per the invariants of `cpu_write_ptr` and `gsp_read_ptr`, `rx` and `tx` are
-            // `<= MSGQ_NUM_PAGES`, and the test above ensured that `rx > tx`.
-            (after_tx.split_at_mut(rx - tx).0, &mut before_tx[0..0])
+            // The area is discontiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - 1` is non-negative because `rx != 0` in this branch.
+            // - The index does not exceed `before_tx.len()` (which equals `tx`) because
+            //   `rx <= tx` in this branch.
+            (after_tx, &mut before_tx[..(rx - 1)])

If this is written with get_disjoint_mut, the indices would be so much easier to understand... To bad that that API is only available from 1.86 onwards.

The current code hands out buffers that go all the way up to and including rx - 1, but we need to maintain an empty slot to prevent the ring buffer from wrapping around into having 'tx == rx', which means empty.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +0 -0

index 09c28eeb6f12..aa8758fc7723 100644
@@ -227,21 +227,26 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.

drivers/gpu/nova-core/gsp/cmdq.rs +6 -1

+            // The area is contiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
+            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
+            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
+            (&mut after_tx[..(rx - tx - 1)], &mut before_tx[0..0])
         } else {
-            // The area from `tx` to `rx`, minus one unit, belongs to the driver.

The current code indexes into after_rx using tx which is an index for the whole buffer, not the split buffer after_rx.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +0 -1

index aa8758fc7723..c26396fda29c 100644
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use core::{
-    cmp,
     mem,
     sync::atomic::{
         fence,

drivers/gpu/nova-core/gsp/cmdq.rs +0 -3

-        match tx.cmp(&rx) {
-            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
-            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),

drivers/gpu/nova-core/gsp/cmdq.rs +0 -0

         }
     }
 

Quoted 24 lines · Eliot Courtney

The current code indexes into after_rx using tx which is an index for the whole buffer, not the split buffer after_rx.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +0 -1

index aa8758fc7723..c26396fda29c 100644
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use core::{
-    cmp,
     mem,
     sync::atomic::{
         fence,

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 267

Quoted 2 lines

drivers/gpu/nova-core/gsp/cmdq.rs +0 -0

@@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
         let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);

This code doesn't need splitting as it doesn't have the uniqueness issue that mutable references have. While you're at it, probably it's chance to simplify the code.

Quoted 4 lines

drivers/gpu/nova-core/gsp/cmdq.rs +0 -3

-        match tx.cmp(&rx) {
-            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
-            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 277

Quoted 10 lines

drivers/gpu/nova-core/gsp/cmdq.rs +9 -1

-            cmp::Ordering::Less => (after_rx, &before_rx[..tx]),
+        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for reading.
+        if rx <= tx {
+            // The area is contiguous.
+            // PANIC:
+            // - The index `tx - rx` is non-negative because `rx <= tx` in this branch.
+            // - The index does not exceed `after_rx.len()` (which is `MSGQ_NUM_PAGES - rx`)
+            //   because `tx < MSGQ_NUM_PAGES` by the `gsp_write_ptr` invariant.
+            (&after_rx[..(tx - rx)], &after_rx[0..0])

This can be just (&data[rx..tx], &[]) without the split.

drivers/gpu/nova-core/gsp/cmdq.rs fn new(dev: &device::Device<device::Bound>) -> Result<Self> { · line 282

Quoted 5 lines

drivers/gpu/nova-core/gsp/cmdq.rs +5 -0

+        } else {
+            // The area is discontiguous.
+            // PANIC: `tx` does not exceed `before_rx.len()` (which equals `rx`) because
+            //   `tx < rx` in this branch.
+            (after_rx, &before_rx[..tx])

This can be just (&data[rx..], &data[..tx]) without the split.

The current code indexes into after_rx using tx which is an index for the whole buffer, not the split buffer after_rx.

Also add more rigorous no-panic proofs.

Fixes 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/gsp/cmdq.rs | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/gpu/nova-core/gsp/cmdq.rs +0 -4

index aa8758fc7723..c26396fda29c 100644
@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
 use core::{
-    cmp,
     mem,
     sync::atomic::{
         fence,

@@ -267,10 +266,20 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
         // PANIC: per the invariant of `cpu_read_ptr`, `rx` is `< MSGQ_NUM_PAGES`.
         let (before_rx, after_rx) = gsp_mem.gspq.msgq.data.split_at(rx);
 
-        match tx.cmp(&rx) {
-            cmp::Ordering::Equal => (&after_rx[0..0], &after_rx[0..0]),
-            cmp::Ordering::Greater => (&after_rx[..tx], &before_rx[0..0]),