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

["Alexandre Courbot" <acourbot@nvidia.com>]

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@nvidia.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Timur Tabi <ttabi@nvidia.com>
>>> Signed-off-by: John Hubbard <jhubbard@nvidia.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.