| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2c381df5-de8c-4f2b-ad2d-9cce9569a956@nvidia.com>
Date: Thu, 6 Nov 2025 13:12:12 -0800
From: John Hubbard <jhubbard@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>,
Danilo Krummrich <dakr@...nel.org>
Cc: Joel Fernandes <joelagnelf@...dia.com>, Timur Tabi <ttabi@...dia.com>,
Alistair Popple <apopple@...dia.com>, Edwin Peer <epeer@...dia.com>,
Zhi Wang <zhiw@...dia.com>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Bjorn Helgaas <bhelgaas@...gle.com>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>,
Nouveau <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v5 1/3] gpu: nova-core: prepare Spec and Revision types
for boot0/boot42
On 11/5/25 11:45 PM, Alexandre Courbot wrote:
> On Thu Nov 6, 2025 at 10:27 AM JST, John Hubbard wrote:
>> 1) Implement Display for Spec. This simplifies the dev_info!() code for
>> printing banners such as:
>>
>> NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>
>> 2) Decouple Revision from boot0.
>>
>> 3) Enhance Revision, which in turn simplifies Spec::new().
>>
>> 4) Also, slightly enhance the comment about Spec, to be more precise.
>
> A bullet-list in a patch description is a sure sign you will be asked to
> split things up. :)
Yes. That is an eternal truth, which I foolishly ignored here. haha
>
> And in this case I think it makes all the more sense, since all these
> things taken separately ought to be simple, but having them in the same
> diff makes it confusing to review.
>
> Although it's mostly the `Display` implementation that at the very least
> should be its own patch, the rest can probably be kept together as it is
> related, and adding an intermediate patch would require temporary code
> to build Revision. The diff becomes much clearer once the impl blocks
> are moved where they should be (please see below).
>
> The comment update can be squashed together with the Revision/Spec
> patch.
>
Will do.
>>
>> Cc: Alexandre Courbot <acourbot@...dia.com>
>> Cc: Danilo Krummrich <dakr@...nel.org>
>> Cc: Timur Tabi <ttabi@...dia.com>
>> Signed-off-by: John Hubbard <jhubbard@...dia.com>
>> ---
>> drivers/gpu/nova-core/gpu.rs | 45 +++++++++++++++++++----------------
>> drivers/gpu/nova-core/regs.rs | 8 +++++++
>> 2 files changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 9d182bffe8b4..8173cdcd8378 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -130,16 +130,18 @@ fn try_from(value: u8) -> Result<Self> {
>> }
>>
>> pub(crate) struct Revision {
>> - major: u8,
>> - minor: u8,
>> + pub(crate) major: u8,
>> + pub(crate) minor: u8,
>> }
>>
>> -impl Revision {
>> - fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
>> - Self {
>> - major: boot0.major_revision(),
>> - minor: boot0.minor_revision(),
>> - }
>> +impl TryFrom<regs::NV_PMC_BOOT_0> for Spec {
>> + type Error = Error;
>> +
>> + fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> {
>> + Ok(Self {
>> + chipset: boot0.chipset()?,
>> + revision: boot0.revision(),
>> + })
>
> This impl block for `Revision` gets replaced by a block for `Spec`,
> which is only declared later. Can you move it (and the one for BOOT_42
> in the third patch) to the right place, next to the other impl blocks
> for `Spec`?
Yes.
>
>> }
>> }
>>
>> @@ -149,7 +151,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> }
>> }
>>
>> -/// Structure holding the metadata of the GPU.
>> +/// Structure holding a basic description of the GPU: Architecture, Chipset and Revision.
>> pub(crate) struct Spec {
>> chipset: Chipset,
>> /// The revision of the chipset.
>> @@ -160,10 +162,19 @@ impl Spec {
>> fn new(bar: &Bar0) -> Result<Spec> {
>> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>
>> - Ok(Self {
>> - chipset: boot0.chipset()?,
>> - revision: Revision::from_boot0(boot0),
>> - })
>> + Spec::try_from(boot0)
>> + }
>> +}
>> +
>> +impl fmt::Display for Spec {
>> + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> + write!(
>> + f,
>> + "Chipset: {}, Architecture: {:?}, Revision: {}",
>> + self.chipset,
>> + self.chipset.arch(),
>> + self.revision
>> + )
>> }
>> }
>>
>> @@ -193,13 +204,7 @@ pub(crate) fn new<'a>(
>> ) -> impl PinInit<Self, Error> + 'a {
>> try_pin_init!(Self {
>> spec: Spec::new(bar).inspect(|spec| {
>> - dev_info!(
>> - pdev.as_ref(),
>> - "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
>> - spec.chipset,
>> - spec.chipset.arch(),
>> - spec.revision
>> - );
>> + dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
>> })?,
>>
>> // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
>> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
>> index 206dab2e1335..207b865335af 100644
>> --- a/drivers/gpu/nova-core/regs.rs
>> +++ b/drivers/gpu/nova-core/regs.rs
>> @@ -41,6 +41,14 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
>> })
>> .and_then(Chipset::try_from)
>> }
>> +
>> + /// Returns the revision information of the chip.
>> + pub(crate) fn revision(self) -> crate::gpu::Revision {
>> + crate::gpu::Revision {
>
> nit: let's import `Revision`, or at least `gpu`, to align with what we
> do with the other structures.
Yes.
thanks,
--
John Hubbard
Powered by blists - more mailing lists