linux-kernel - Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DE3AAYKKI0HN.2QTWD76BN3LMO@nvidia.com>
Date: Sat, 08 Nov 2025 20:41:57 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "John Hubbard" <jhubbard@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "dakr@...nel.org" <dakr@...nel.org>
Cc: "Alexandre Courbot" <acourbot@...dia.com>, "lossin@...nel.org"
 <lossin@...nel.org>, "a.hindborg@...nel.org" <a.hindborg@...nel.org>,
 "boqun.feng@...il.com" <boqun.feng@...il.com>, "aliceryhl@...gle.com"
 <aliceryhl@...gle.com>, "Zhi Wang" <zhiw@...dia.com>, "simona@...ll.ch"
 <simona@...ll.ch>, "alex.gaynor@...il.com" <alex.gaynor@...il.com>,
 "ojeda@...nel.org" <ojeda@...nel.org>, "tmgross@...ch.edu"
 <tmgross@...ch.edu>, "nouveau@...ts.freedesktop.org"
 <nouveau@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "rust-for-linux@...r.kernel.org"
 <rust-for-linux@...r.kernel.org>, "bjorn3_gh@...tonmail.com"
 <bjorn3_gh@...tonmail.com>, "Edwin Peer" <epeer@...dia.com>,
 "airlied@...il.com" <airlied@...il.com>, "Joel Fernandes"
 <joelagnelf@...dia.com>, "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
 "gary@...yguo.net" <gary@...yguo.net>, "Alistair Popple"
 <apopple@...dia.com>, "Nouveau" <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

On Sat Nov 8, 2025 at 2:10 PM JST, John Hubbard wrote:
> On 11/7/25 9:02 PM, Timur Tabi wrote:
>> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>>> Implement Display for Spec. This simplifies the dev_info!() code for
>>> printing banners such as:
>>>
>>>     NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>>
>>> 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>
>> 
>> I'm okay with the entire patch set, but I do have a few questions.
>
> Great!
>
>> 
>>> +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
>>> +        )
>>> +    }
>>> +}
>>> +
>>>  /// Structure holding the resources required to operate the GPU.
>>>  #[pin_data]
>>>  pub(crate) struct Gpu {
>>> @@ -206,13 +218,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);
>> 
>> I believe that this is the only place where a `Spec` is actually printed.  Does it really make
>> sense to implement Display for a single usage?  Do we generally want to implement Display for
>> new types?
>> 
>
> I agree that it looks a little excessive, but I defer to reviewers
> who have a lot more Rust experience, and they requested this during
> a review of an earlier version.

I think this is the correct way to do; `Spec` should be the one to
decide how it is displayed, and from a maintainability perspective this
ensures that other sites that will want to print a `Spec` in the future
will reuse this implementation, instead of either rewriting one
themselves or having to figure out that there was already an existing
site and factor it out.

Iow, this code is proactively doing the right thing.