[PATCH 0/3] Various minor fixes to Nouveau

[PATCH 0/3] Various minor fixes to Nouveau

[Timur Tabi]

Three minor fixes to Nouveau, discovered while working on Nova.

Timur Tabi (3):
  drm/nouveau: fix error path in nvkm_gsp_fwsec_v2
  drm/nouveau: remove unused increment in gm200_flcn_pio_imem_wr
  drm/nouveau: remove unused memory target test

 drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c     | 15 ++++-----------
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c |  5 +++--
 2 files changed, 7 insertions(+), 13 deletions(-)


base-commit: 6531a2cf07ef156956840853692755cc7e1621b7
-- 
2.43.0

[PATCH v2 1/3] drm/nouveau: fix error path in nvkm_gsp_fwsec_v2

[Timur Tabi]

Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call fails, but
it never uses or returns 'ret' after that point.  We always need to release
the firmware regardless, so do that and then check for error.

Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
index 52412965fac1..5b721bd9d799 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
@@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const char *name,
 	fw->boot_addr = bld->start_tag << 8;
 	fw->boot_size = bld->code_size;
 	fw->boot = kmemdup(bl->data + hdr->data_offset + bld->code_off, fw->boot_size, GFP_KERNEL);
-	if (!fw->boot)
-		ret = -ENOMEM;
 
 	nvkm_firmware_put(bl);
 
+	if (!fw->boot)
+		return -ENOMEM;
+
 	/* Patch in interface data. */
 	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset, init_cmd);
 }
-- 
2.43.0

[PATCH v2 2/3] drm/nouveau: remove unused increment in gm200_flcn_pio_imem_wr

[Timur Tabi]

The 'tag' parameter is passed by value and is not actually used after
being incremented, so remove the increment.  It's the function that calls
gm200_flcn_pio_imem_wr that is supposed to (and does) increment 'tag'.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
index b7da3ab44c27..6a004c6e6742 100644
--- a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
+++ b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
@@ -103,7 +103,7 @@ gm200_flcn_pio_imem_wr_init(struct nvkm_falcon *falcon, u8 port, bool sec, u32 i
 static void
 gm200_flcn_pio_imem_wr(struct nvkm_falcon *falcon, u8 port, const u8 *img, int len, u16 tag)
 {
-	nvkm_falcon_wr32(falcon, 0x188 + (port * 0x10), tag++);
+	nvkm_falcon_wr32(falcon, 0x188 + (port * 0x10), tag);
 	while (len >= 4) {
 		nvkm_falcon_wr32(falcon, 0x184 + (port * 0x10), *(u32 *)img);
 		img += 4;
-- 
2.43.0

[PATCH v2 3/3] drm/nouveau: remove unused memory target test

[Timur Tabi]

The memory target check is a hold-over from a refactor.  It's harmless
but distracting, so just remove it.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
index 6a004c6e6742..7c43397c19e6 100644
--- a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
+++ b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
@@ -249,9 +249,11 @@ int
 gm200_flcn_fw_load(struct nvkm_falcon_fw *fw)
 {
 	struct nvkm_falcon *falcon = fw->falcon;
-	int target, ret;
+	int ret;
 
 	if (fw->inst) {
+		int target;
+
 		nvkm_falcon_mask(falcon, 0x048, 0x00000001, 0x00000001);
 
 		switch (nvkm_memory_target(fw->inst)) {
@@ -285,15 +287,6 @@ gm200_flcn_fw_load(struct nvkm_falcon_fw *fw)
 	}
 
 	if (fw->boot) {
-		switch (nvkm_memory_target(&fw->fw.mem.memory)) {
-		case NVKM_MEM_TARGET_VRAM: target = 4; break;
-		case NVKM_MEM_TARGET_HOST: target = 5; break;
-		case NVKM_MEM_TARGET_NCOH: target = 6; break;
-		default:
-			WARN_ON(1);
-			return -EINVAL;
-		}
-
 		ret = nvkm_falcon_pio_wr(falcon, fw->boot, 0, 0,
 					 IMEM, falcon->code.limit - fw->boot_size, fw->boot_size,
 					 fw->boot_addr >> 8, false);
-- 
2.43.0

Re: [PATCH v2 1/3] drm/nouveau: fix error path in nvkm_gsp_fwsec_v2

[Philipp Stanner]

On Mon, 2025-08-04 at 14:25 -0500, Timur Tabi wrote:
> Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call fails,
> but
> it never uses or returns 'ret' after that point.  We always need to
> release
> the firmware regardless, so do that and then check for error.
> 
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>

That looks like a bug (leak?) to me.

+Cc stable?

P.

> ---
>  drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> index 52412965fac1..5b721bd9d799 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> @@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const
> char *name,
>  	fw->boot_addr = bld->start_tag << 8;
>  	fw->boot_size = bld->code_size;
>  	fw->boot = kmemdup(bl->data + hdr->data_offset + bld-
> >code_off, fw->boot_size, GFP_KERNEL);
> -	if (!fw->boot)
> -		ret = -ENOMEM;
>  
>  	nvkm_firmware_put(bl);
>  
> +	if (!fw->boot)
> +		return -ENOMEM;
> +
>  	/* Patch in interface data. */
>  	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset,
> init_cmd);
>  }

Re: [PATCH v2 2/3] drm/nouveau: remove unused increment in gm200_flcn_pio_imem_wr

[Philipp Stanner]

On Mon, 2025-08-04 at 14:25 -0500, Timur Tabi wrote:
> The 'tag' parameter is passed by value and is not actually used after
> being incremented, so remove the increment.  It's the function that calls
> gm200_flcn_pio_imem_wr that is supposed to (and does) increment 'tag'.
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>

Reviewed-by: Philipp Stanner <phasta@kernel.org>

> ---
>  drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
> index b7da3ab44c27..6a004c6e6742 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c
> @@ -103,7 +103,7 @@ gm200_flcn_pio_imem_wr_init(struct nvkm_falcon *falcon, u8 port, bool sec, u32 i
>  static void
>  gm200_flcn_pio_imem_wr(struct nvkm_falcon *falcon, u8 port, const u8 *img, int len, u16 tag)
>  {
> -	nvkm_falcon_wr32(falcon, 0x188 + (port * 0x10), tag++);
> +	nvkm_falcon_wr32(falcon, 0x188 + (port * 0x10), tag);
>  	while (len >= 4) {
>  		nvkm_falcon_wr32(falcon, 0x184 + (port * 0x10), *(u32 *)img);
>  		img += 4;

Re: [PATCH v2 1/3] drm/nouveau: fix error path in nvkm_gsp_fwsec_v2

[Danilo Krummrich]

On 8/4/25 9:25 PM, Timur Tabi wrote:
> Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call fails, but
> it never uses or returns 'ret' after that point.  We always need to release
> the firmware regardless, so do that and then check for error.
> 
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")

As mentioned by Philipp, we should Cc stable for this one.

> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>   drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> index 52412965fac1..5b721bd9d799 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> @@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const char *name,
>   	fw->boot_addr = bld->start_tag << 8;
>   	fw->boot_size = bld->code_size;
>   	fw->boot = kmemdup(bl->data + hdr->data_offset + bld->code_off, fw->boot_size, GFP_KERNEL);
> -	if (!fw->boot)
> -		ret = -ENOMEM;
>   
>   	nvkm_firmware_put(bl);
>   
> +	if (!fw->boot)
> +		return -ENOMEM;

Good catch! It's also good that you moved the return below the
nvkm_firmware_put() call.

But don't we also need to revert the preceding call to nvkm_falcon_fw_ctor()?

> +
>   	/* Patch in interface data. */
>   	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset, init_cmd);
>   }

Re: [PATCH 0/3] Various minor fixes to Nouveau

[Danilo Krummrich]

On 8/4/25 9:25 PM, Timur Tabi wrote:
> Three minor fixes to Nouveau, discovered while working on Nova.

Thanks for this series!

> Timur Tabi (3):
>    drm/nouveau: fix error path in nvkm_gsp_fwsec_v2
>    drm/nouveau: remove unused increment in gm200_flcn_pio_imem_wr
>    drm/nouveau: remove unused memory target test

Please also add a Fixes tag for the last two patches.

Re: [PATCH v2 1/3] drm/nouveau: fix error path in nvkm_gsp_fwsec_v2

[Timur Tabi]

On Sat, 2025-08-09 at 13:26 +0200, Danilo Krummrich wrote:
> > +	if (!fw->boot)
> > +		return -ENOMEM;
> 
> Good catch! It's also good that you moved the return below the
> nvkm_firmware_put() call.
> 
> But don't we also need to revert the preceding call to nvkm_falcon_fw_ctor()?

I don't know.  I cannot decipher Nouveau's cleanup code.

If it does need to be reverted, then we have that problem in a lot of places, in both
nvkm_gsp_fwsec_v2() and nvkm_gsp_fwsec_v3().

At one point Ben told me that Nouveau cleans itself up on failure, eliminating the need to clean up
on every "return ret" exit.  You can see this through fwsec.c -- none of the return-error calls do
any cleanup.

I tried to test this, but I didn't get far.  I modified nvkm_gsp_fwsec_v2 to return failure and did
a trace of nvkm_falcon_fw_ctor() and nvkm_falcon_fw_ctor(), and this is what I found:

[ 1281.361048] nvkm_falcon_fw_ctor::204 fw=ffff8e898022f540 (null)                                 
[ 1281.367036] nvkm_falcon_fw_ctor::204 fw=ffff8e898022f640 (null)                                 
[ 1281.379502] nvkm_falcon_fw_ctor::204 fw=ffffcfc3408e7908 (null)                                 
[ 1281.422846] nvkm_falcon_fw_dtor::157 fw=ffff8e89812480d0 (null)                                 
[ 1281.429326] nvkm_falcon_fw_dtor::157 fw=ffff8e898022f640 booter-unload                          
[ 1281.435884] nvkm_falcon_fw_dtor::157 fw=ffff8e898022f540 booter-load  

What's throwing me off is:

[ 1281.379502] nvkm_falcon_fw_ctor::204 fw=ffffcfc3408e7908 (null)                                 
[ 1281.422846] nvkm_falcon_fw_dtor::157 fw=ffff8e89812480d0 (null)                                 

These two should match.  For some reason, there is no dtor call for 7908, but more disturbingly,
there is no ctor call for 80d0.  This doesn't happen if nvkm_gsp_fwsec_v2 succeeds.

Re: [PATCH 0/3] Various minor fixes to Nouveau

[Timur Tabi]

On Sat, 2025-08-09 at 13:28 +0200, Danilo Krummrich wrote:
> On 8/4/25 9:25 PM, Timur Tabi wrote:
> > Three minor fixes to Nouveau, discovered while working on Nova.
> 
> Thanks for this series!
> 
> > Timur Tabi (3):
> >    drm/nouveau: fix error path in nvkm_gsp_fwsec_v2
> >    drm/nouveau: remove unused increment in gm200_flcn_pio_imem_wr
> >    drm/nouveau: remove unused memory target test
> 
> Please also add a Fixes tag for the last two patches.

Done.  Please see v3, posted on the 12th.