Disambiguate a few things in comments in cmdq.rs.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index f139aad7af3f..09c28eeb6f12 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -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,
@@ -222,7 +224,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 = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];
- // 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`.
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) {
@@ -315,7 +317,7 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
//
// # 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_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();
--
2.52.0On Fri Jan 23, 2026 at 12:12 PM GMT, Eliot Courtney wrote:
> Disambiguate a few things in comments in cmdq.rs.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> drivers/gpu/nova-core/gsp/cmdq.rs | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index f139aad7af3f..09c28eeb6f12 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -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,
> @@ -222,7 +224,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 = &mut unsafe { self.0.as_slice_mut(0, 1) }.unwrap()[0];
> - // 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`?
> 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) {
> @@ -315,7 +317,7 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
> //
> // # Invariants
> //
> - // - 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?
Best,
Gary
> 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();
>
On Tue Jan 27, 2026 at 3:04 AM JST, Gary Guo wrote:
>> // - 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];
>> - // 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`?
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/
>> // # Invariants
>> //
>> - // - 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?
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.
On Wed Jan 28, 2026 at 1:35 PM JST, Eliot Courtney wrote:
> On Tue Jan 27, 2026 at 3:04 AM JST, Gary Guo wrote:
>>> // - 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];
>>> - // 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`?
>
> 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/
I agree it's a good idea to mention the source of the invariant, lest we
may lose track of why it is indeed true.
>
>>> // # Invariants
>>> //
>>> - // - 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?
>
> 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.
On Wed Jan 28, 2026 at 9:17 AM CET, Alexandre Courbot wrote: > On Wed Jan 28, 2026 at 1:35 PM JST, Eliot Courtney wrote: >> On Tue Jan 27, 2026 at 3:04 AM JST, Gary Guo wrote: >>> 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. While I really like the mathematical syntax, I think using the Rust syntax is superior, as it requires zero mental cycles to translate it to what is likely to be found in the code as well. (There also have been some considerations of using tools to validate safety comments or invariants to some extend eventually.)
© 2016 - 2026 Red Hat, Inc.