From: "Eliot Courtney" <ecourtney@nvidia.com>
To: "Gary Guo" <gary@garyguo.net>,
"Eliot Courtney" <ecourtney@nvidia.com>,
"Danilo Krummrich" <dakr@kernel.org>,
"Alexandre Courbot" <acourbot@nvidia.com>,
"Alice Ryhl" <aliceryhl@google.com>,
"David Airlie" <airlied@gmail.com>,
"Simona Vetter" <simona@ffwll.ch>,
"Alistair Popple" <apopple@nvidia.com>
Cc: nouveau@lists.freedesktop.org, rust-for-linux@vger.kernel.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
dri-devel <dri-devel-bounces@lists.freedesktop.org>
Subject: Re: [PATCH v2 2/4] gpu: nova-core: gsp: clarify comments about invariants and pointer roles
Date: Wed, 28 Jan 2026 13:35:35 +0900 [thread overview]
Message-ID: <DFZXYMSS9OOI.5NUPB8GCHRKD@nvidia.com> (raw)
In-Reply-To: <DFYPX3TNI3Y2.38MQUIWHHR9Z1@garyguo.net>
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.
WARNING: multiple messages have this Message-ID (diff)
From: "Eliot Courtney" <ecourtney@nvidia.com>
To: "Gary Guo" <gary@garyguo.net>,
"Eliot Courtney" <ecourtney@nvidia.com>,
"Danilo Krummrich" <dakr@kernel.org>,
"Alexandre Courbot" <acourbot@nvidia.com>,
"Alice Ryhl" <aliceryhl@google.com>,
"David Airlie" <airlied@gmail.com>,
"Simona Vetter" <simona@ffwll.ch>,
"Alistair Popple" <apopple@nvidia.com>
Cc: <nouveau@lists.freedesktop.org>, <rust-for-linux@vger.kernel.org>,
<dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
"dri-devel" <dri-devel-bounces@lists.freedesktop.org>
Subject: Re: [PATCH v2 2/4] gpu: nova-core: gsp: clarify comments about invariants and pointer roles
Date: Wed, 28 Jan 2026 13:35:35 +0900 [thread overview]
Message-ID: <DFZXYMSS9OOI.5NUPB8GCHRKD@nvidia.com> (raw)
In-Reply-To: <DFYPX3TNI3Y2.38MQUIWHHR9Z1@garyguo.net>
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.
next prev parent reply other threads:[~2026-01-28 4:35 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-23 12:12 [PATCH v2 0/4] gpu: nova-core: gsp: fix command queue ring buffer bugs Eliot Courtney
2026-01-23 12:12 ` [PATCH v2 1/4] gpu: nova-core: gsp: fix incorrect advancing of write pointer Eliot Courtney
2026-01-23 12:12 ` [PATCH v2 2/4] gpu: nova-core: gsp: clarify comments about invariants and pointer roles Eliot Courtney
2026-01-26 18:04 ` Gary Guo
2026-01-26 18:04 ` Gary Guo
2026-01-28 4:35 ` Eliot Courtney [this message]
2026-01-28 4:35 ` Eliot Courtney
2026-01-28 8:17 ` Alexandre Courbot
2026-01-28 8:17 ` Alexandre Courbot
2026-01-28 10:46 ` Danilo Krummrich
2026-01-28 10:46 ` Danilo Krummrich
2026-01-23 12:12 ` [PATCH v2 3/4] gpu: nova-core: gsp: fix improper handling of empty slot in cmdq Eliot Courtney
2026-01-26 18:26 ` Gary Guo
2026-01-26 18:26 ` Gary Guo
2026-01-28 11:42 ` Alexandre Courbot
2026-01-28 11:42 ` Alexandre Courbot
2026-01-28 11:39 ` Alexandre Courbot
2026-01-28 11:39 ` Alexandre Courbot
2026-01-23 12:12 ` [PATCH v2 4/4] gpu: nova-core: gsp: fix improper indexing in driver_read_area Eliot Courtney
2026-01-26 18:30 ` Gary Guo
2026-01-26 18:30 ` Gary Guo
2026-01-28 12:18 ` Alexandre Courbot
2026-01-28 12:18 ` Alexandre Courbot
2026-01-28 11:57 ` Alexandre Courbot
2026-01-28 11:57 ` Alexandre Courbot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DFZXYMSS9OOI.5NUPB8GCHRKD@nvidia.com \
--to=ecourtney@nvidia.com \
--cc=acourbot@nvidia.com \
--cc=airlied@gmail.com \
--cc=aliceryhl@google.com \
--cc=apopple@nvidia.com \
--cc=dakr@kernel.org \
--cc=dri-devel-bounces@lists.freedesktop.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gary@garyguo.net \
--cc=linux-kernel@vger.kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=simona@ffwll.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.