Keep Gsp::new() from getting too cluttered, by factoring out the
selection of .fwsignature* items. This will continue to grow as we add
GPUs.

Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/firmware/gsp.rs | 31 +++++++++++++++------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
index 2bbea1db0238..60ea730d1bd5 100644
--- a/drivers/gpu/nova-core/firmware/gsp.rs
+++ b/drivers/gpu/nova-core/firmware/gsp.rs
@@ -146,6 +146,22 @@ pub(crate) struct GspFirmware {
 }
 
 impl GspFirmware {
+    fn find_gsp_sigs_section(chipset: Chipset) -> Option<&'static str> {
+        match chipset.arch() {
+            Architecture::Turing if matches!(chipset, Chipset::TU116 | Chipset::TU117) => {
+                Some(".fwsignature_tu11x")
+            }
+            Architecture::Turing => Some(".fwsignature_tu10x"),
+            // GA100 uses the same firmware as Turing
+            Architecture::Ampere if chipset == Chipset::GA100 => Some(".fwsignature_tu10x"),
+            Architecture::Ampere => Some(".fwsignature_ga10x"),
+            Architecture::Ada => Some(".fwsignature_ad10x"),
+            Architecture::Hopper => Some(".fwsignature_gh10x"),
+            Architecture::BlackwellGB10x => Some(".fwsignature_gb10x"),
+            Architecture::BlackwellGB20x => Some(".fwsignature_gb20x"),
+        }
+    }
+
     /// Loads the GSP firmware binaries, map them into `dev`'s address-space, and creates the page
     /// tables expected by the GSP bootloader to load it.
     pub(crate) fn new<'a>(
@@ -211,20 +227,7 @@ pub(crate) fn new<'a>(
                 },
                 size,
                 signatures: {
-                    let sigs_section = match chipset.arch() {
-                        Architecture::Turing
-                            if matches!(chipset, Chipset::TU116 | Chipset::TU117) => {
-                            ".fwsignature_tu11x"
-                        }
-                        Architecture::Turing => ".fwsignature_tu10x",
-                        // GA100 uses the same firmware as Turing
-                        Architecture::Ampere if chipset == Chipset::GA100 => ".fwsignature_tu10x",
-                        Architecture::Ampere => ".fwsignature_ga10x",
-                        Architecture::Ada => ".fwsignature_ad10x",
-                        Architecture::Hopper => ".fwsignature_gh10x",
-                        Architecture::BlackwellGB10x => ".fwsignature_gb10x",
-                        Architecture::BlackwellGB20x => ".fwsignature_gb20x",
-                    };
+                    let sigs_section = Self::find_gsp_sigs_section(chipset).ok_or(ENOTSUPP)?;
 
                     elf::elf64_section(firmware.data(), sigs_section)
                         .ok_or(EINVAL)
-- 
2.53.0

On Thu Mar 26, 2026 at 10:38 AM JST, John Hubbard wrote:
> Keep Gsp::new() from getting too cluttered, by factoring out the
> selection of .fwsignature* items. This will continue to grow as we add
> GPUs.
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware/gsp.rs | 31 +++++++++++++++------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
> index 2bbea1db0238..60ea730d1bd5 100644
> --- a/drivers/gpu/nova-core/firmware/gsp.rs
> +++ b/drivers/gpu/nova-core/firmware/gsp.rs
> @@ -146,6 +146,22 @@ pub(crate) struct GspFirmware {
>  }
>  
>  impl GspFirmware {
> +    fn find_gsp_sigs_section(chipset: Chipset) -> Option<&'static str> {
> +        match chipset.arch() {
> +            Architecture::Turing if matches!(chipset, Chipset::TU116 | Chipset::TU117) => {
> +                Some(".fwsignature_tu11x")
> +            }
> +            Architecture::Turing => Some(".fwsignature_tu10x"),
> +            // GA100 uses the same firmware as Turing
> +            Architecture::Ampere if chipset == Chipset::GA100 => Some(".fwsignature_tu10x"),
> +            Architecture::Ampere => Some(".fwsignature_ga10x"),
> +            Architecture::Ada => Some(".fwsignature_ad10x"),
> +            Architecture::Hopper => Some(".fwsignature_gh10x"),
> +            Architecture::BlackwellGB10x => Some(".fwsignature_gb10x"),
> +            Architecture::BlackwellGB20x => Some(".fwsignature_gb20x"),
> +        }
> +    }
> +

So in v8 I pointed out, on this very patch, that this method doesn't
need to return an `Option` [1]. Which you agreed to [2]. And yet this is
unchanged?

Oh, actually this is fixed in the next patch, for whatever reason. Why?
The original match statement was even exhaustive to begin with, so I
don't see why that `Option` was even introduced in the first place.

Please apply the feedback on the patch it was given on.

[1] https://lore.kernel.org/all/177443554510.105362.9416236198599659187.b4-review@b4/
[2] https://lore.kernel.org/all/5db75d43-830d-460e-b377-9a1f72c21fc7@nvidia.com/

On 3/30/26 7:29 AM, Alexandre Courbot wrote:
> On Thu Mar 26, 2026 at 10:38 AM JST, John Hubbard wrote:
>> Keep Gsp::new() from getting too cluttered, by factoring out the
>> selection of .fwsignature* items. This will continue to grow as we add
>> GPUs.
>>
>> Reviewed-by: Gary Guo <gary@garyguo.net>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/firmware/gsp.rs | 31 +++++++++++++++------------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
>> index 2bbea1db0238..60ea730d1bd5 100644
>> --- a/drivers/gpu/nova-core/firmware/gsp.rs
>> +++ b/drivers/gpu/nova-core/firmware/gsp.rs
>> @@ -146,6 +146,22 @@ pub(crate) struct GspFirmware {
>>  }
>>  
>>  impl GspFirmware {
>> +    fn find_gsp_sigs_section(chipset: Chipset) -> Option<&'static str> {
>> +        match chipset.arch() {
>> +            Architecture::Turing if matches!(chipset, Chipset::TU116 | Chipset::TU117) => {
>> +                Some(".fwsignature_tu11x")
>> +            }
>> +            Architecture::Turing => Some(".fwsignature_tu10x"),
>> +            // GA100 uses the same firmware as Turing
>> +            Architecture::Ampere if chipset == Chipset::GA100 => Some(".fwsignature_tu10x"),
>> +            Architecture::Ampere => Some(".fwsignature_ga10x"),
>> +            Architecture::Ada => Some(".fwsignature_ad10x"),
>> +            Architecture::Hopper => Some(".fwsignature_gh10x"),
>> +            Architecture::BlackwellGB10x => Some(".fwsignature_gb10x"),
>> +            Architecture::BlackwellGB20x => Some(".fwsignature_gb20x"),
>> +        }
>> +    }
>> +
> 
> So in v8 I pointed out, on this very patch, that this method doesn't
> need to return an `Option` [1]. Which you agreed to [2]. And yet this is
> unchanged?
> 
> Oh, actually this is fixed in the next patch, for whatever reason. Why?

It's an off-by-one error in chosing which commit to edit during git
rebase.

> The original match statement was even exhaustive to begin with, so I
> don't see why that `Option` was even introduced in the first place.

Because at various points during the many months that this patchset
has existed, there were unsupported Arches. Turing, for example.


thanks,
-- 
John Hubbard