[PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[Srinivasan Shanmugam]

amdgpu_vm_flush() sends commands to the GPU to update the VM page tables
for a job.

When a job uses a GPU virtual address space, the GPU needs to refresh
its address translations after the driver updates the page tables.
A VM flush tells the GPU to forget old address translations and use the
updated page table mappings.

This flush command is not supported on all rings. Only rings that
implement the emit_vm_flush() callback know how to emit the correct
hardware command for this operation.

The function already gates vm_flush_needed on the presence of
ring->funcs->emit_vm_flush earlier in the logic. However, static
analysis tools such as Smatch may not track this relationship through
the vm_flush_needed boolean and warn that emit_vm_flush() could be NULL
when the VM flush command is emitted later.

Fixes the below:
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we previously assumed 'ring->funcs->emit_vm_flush' could be null (see line 788)

Fixes: b3cd285fa68d ("drm/amdgpu: update the PASID mapping only on demand")
Cc: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b89013a6aa0b..cc79cb7dd4e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -807,7 +807,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	if (cleaner_shader_needed)
 		ring->funcs->emit_cleaner_shader(ring);
 
-	if (vm_flush_needed) {
+	if (vm_flush_needed && ring->funcs->emit_vm_flush) {
 		trace_amdgpu_vm_flush(ring, job->vmid, job->vm_pd_addr);
 		amdgpu_ring_emit_vm_flush(ring, job->vmid, job->vm_pd_addr);
 	}
-- 
2.34.1

Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[Lazar, Lijo]

On 15-Mar-26 12:25 PM, Srinivasan Shanmugam wrote:
> amdgpu_vm_flush() sends commands to the GPU to update the VM page tables
> for a job.
> 
> When a job uses a GPU virtual address space, the GPU needs to refresh
> its address translations after the driver updates the page tables.
> A VM flush tells the GPU to forget old address translations and use the
> updated page table mappings.
> 
> This flush command is not supported on all rings. Only rings that
> implement the emit_vm_flush() callback know how to emit the correct
> hardware command for this operation.
> 
> The function already gates vm_flush_needed on the presence of
> ring->funcs->emit_vm_flush earlier in the logic. However, static
> analysis tools such as Smatch may not track this relationship through
> the vm_flush_needed boolean and warn that emit_vm_flush() could be NULL
> when the VM flush command is emitted later.
> 
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we previously assumed 'ring->funcs->emit_vm_flush' could be null (see line 788)
> 

I think the code logic is correct and the check is implicit in 
vm_flush_needed. We don't need to write code to eliminate 'false 
warnings'. Moreover, it is inappropriate to use a Fixes tag for this.

Thanks,
Lijo

> Fixes: b3cd285fa68d ("drm/amdgpu: update the PASID mapping only on demand")
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b89013a6aa0b..cc79cb7dd4e1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -807,7 +807,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	if (cleaner_shader_needed)
>   		ring->funcs->emit_cleaner_shader(ring);
>   
> -	if (vm_flush_needed) {
> +	if (vm_flush_needed && ring->funcs->emit_vm_flush) {
>   		trace_amdgpu_vm_flush(ring, job->vmid, job->vm_pd_addr);
>   		amdgpu_ring_emit_vm_flush(ring, job->vmid, job->vm_pd_addr);
>   	}

RE: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[SHANMUGAM, SRINIVASAN]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Monday, March 16, 2026 12:19 PM
> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>;
> Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Dan Carpenter <dan.carpenter@linaro.org>;
> Kuehling, Felix <Felix.Kuehling@amd.com>
> Subject: Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in
> amdgpu_vm_flush()
>
>
>
> On 15-Mar-26 12:25 PM, Srinivasan Shanmugam wrote:
> > amdgpu_vm_flush() sends commands to the GPU to update the VM page
> > tables for a job.
> >
> > When a job uses a GPU virtual address space, the GPU needs to refresh
> > its address translations after the driver updates the page tables.
> > A VM flush tells the GPU to forget old address translations and use
> > the updated page table mappings.
> >
> > This flush command is not supported on all rings. Only rings that
> > implement the emit_vm_flush() callback know how to emit the correct
> > hardware command for this operation.
> >
> > The function already gates vm_flush_needed on the presence of
> > ring->funcs->emit_vm_flush earlier in the logic. However, static
> > analysis tools such as Smatch may not track this relationship through
> > the vm_flush_needed boolean and warn that emit_vm_flush() could be
> > NULL when the VM flush command is emitted later.
> >
> > Fixes the below:
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we
> > previously assumed 'ring->funcs->emit_vm_flush' could be null (see
> > line 788)
> >
>
> I think the code logic is correct and the check is implicit in vm_flush_needed. We
> don't need to write code to eliminate 'false warnings'. Moreover, it is inappropriate to
> use a Fixes tag for this.


My initial thought was that the earlier gating of vm_flush_needed on
ring->funcs->emit_vm_flush would make the later call safe, but Smatch
still reports the following warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush()
error: we previously assumed 'ring->funcs->emit_vm_flush' could be null

Dan, could you please confirm whether this is expected to be a false
positive from Smatch due to the vm_flush_needed boolean, or if there
is a recommended pattern to help Smatch track this relationship?

Best regards,
Srini

>
> Thanks,
> Lijo

Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[Christian König]

On 3/16/26 08:04, SHANMUGAM, SRINIVASAN wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Monday, March 16, 2026 12:19 PM
>> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>;
>> Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org; Dan Carpenter <dan.carpenter@linaro.org>;
>> Kuehling, Felix <Felix.Kuehling@amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in
>> amdgpu_vm_flush()
>>
>>
>>
>> On 15-Mar-26 12:25 PM, Srinivasan Shanmugam wrote:
>>> amdgpu_vm_flush() sends commands to the GPU to update the VM page
>>> tables for a job.
>>>
>>> When a job uses a GPU virtual address space, the GPU needs to refresh
>>> its address translations after the driver updates the page tables.
>>> A VM flush tells the GPU to forget old address translations and use
>>> the updated page table mappings.
>>>
>>> This flush command is not supported on all rings. Only rings that
>>> implement the emit_vm_flush() callback know how to emit the correct
>>> hardware command for this operation.
>>>
>>> The function already gates vm_flush_needed on the presence of
>>> ring->funcs->emit_vm_flush earlier in the logic. However, static
>>> analysis tools such as Smatch may not track this relationship through
>>> the vm_flush_needed boolean and warn that emit_vm_flush() could be
>>> NULL when the VM flush command is emitted later.
>>>
>>> Fixes the below:
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we
>>> previously assumed 'ring->funcs->emit_vm_flush' could be null (see
>>> line 788)
>>>
>>
>> I think the code logic is correct and the check is implicit in vm_flush_needed. We
>> don't need to write code to eliminate 'false warnings'. Moreover, it is inappropriate to
>> use a Fixes tag for this.
> 
> 
> My initial thought was that the earlier gating of vm_flush_needed on
> ring->funcs->emit_vm_flush would make the later call safe, but Smatch
> still reports the following warning:
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush()
> error: we previously assumed 'ring->funcs->emit_vm_flush' could be null

Yeah that looks like smatch is either not analyzing the code correctly or we are missing something.

Regards,
Christian.

> 
> Dan, could you please confirm whether this is expected to be a false
> positive from Smatch due to the vm_flush_needed boolean, or if there
> is a recommended pattern to help Smatch track this relationship?
> 
> Best regards,
> Srini
> 
>>
>> Thanks,
>> Lijo

Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[Dan Carpenter]

On Mon, Mar 16, 2026 at 07:04:46AM +0000, SHANMUGAM, SRINIVASAN wrote:
> > > Fixes the below:
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we
> > > previously assumed 'ring->funcs->emit_vm_flush' could be null (see
> > > line 788)
> > >
> >
> > I think the code logic is correct and the check is implicit in vm_flush_needed. We
> > don't need to write code to eliminate 'false warnings'. Moreover, it is inappropriate to
> > use a Fixes tag for this.
> 
> 
> My initial thought was that the earlier gating of vm_flush_needed on
> ring->funcs->emit_vm_flush would make the later call safe, but Smatch
> still reports the following warning:
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush()
> error: we previously assumed 'ring->funcs->emit_vm_flush' could be null
> 
> Dan, could you please confirm whether this is expected to be a false
> positive from Smatch due to the vm_flush_needed boolean, or if there
> is a recommended pattern to help Smatch track this relationship?

I really would just encourage everyone to get used to ignoring old
warnings.  If we fix all the real bugs then the old warnings are all
false positives.

It's this assignment which trips Smatch up:

	vm_flush_needed &= !!ring->funcs->emit_vm_flush  &&
		           job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;

If we wrote it like:

	if (!ring->funcs->emit_vm_flush  || job->vm_pd_addr == AMDGPU_BO_INVALID_OFFSET)
		vm_flush_needed = false;

Then Smatch would parse it correctly.  Or if we wrote it like this:

        vm_flush_needed = vm_flush_needed &&
                          (ring->funcs->emit_vm_flush  &&
                           job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET);

In that case, Smatch sees the && and says this is a condition assignment
and re-writes it behind the scenes as:

	if (vm_flush_needed && (ring->funcs->emit_vm_flush  &&
				job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET))
		vm_flush_needed = true;
	else
		vm_flush_needed = false;

which is also parsed correctly.

So the difference here is that the &= type assignment is not handled.  The
simplest thing would be to make &= a special case.  This wouldn't really
help in many cases but handling |= would help since having a string of
"ret |= frob();" assignments is quite common.

regards,
dan carpenter

Re: [PATCH] drm/amdgpu: Make emit_vm_flush() check explicit in amdgpu_vm_flush()

[Christian König]

On 3/15/26 07:55, Srinivasan Shanmugam wrote:
> amdgpu_vm_flush() sends commands to the GPU to update the VM page tables
> for a job.
> 
> When a job uses a GPU virtual address space, the GPU needs to refresh
> its address translations after the driver updates the page tables.
> A VM flush tells the GPU to forget old address translations and use the
> updated page table mappings.
> 
> This flush command is not supported on all rings. Only rings that
> implement the emit_vm_flush() callback know how to emit the correct
> hardware command for this operation.
> 
> The function already gates vm_flush_needed on the presence of
> ring->funcs->emit_vm_flush earlier in the logic. However, static
> analysis tools such as Smatch may not track this relationship through
> the vm_flush_needed boolean and warn that emit_vm_flush() could be NULL
> when the VM flush command is emitted later.
> 
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:826 amdgpu_vm_flush() error: we previously assumed 'ring->funcs->emit_vm_flush' could be null (see line 788)

Absolutely clear NAK.


The test on line 788 is there to not set vm_flush_needed if ring->funcs->emit_vm_flush is NULL.

So testing that again here is completely nonsense.

Regards,
Christian.

> 
> Fixes: b3cd285fa68d ("drm/amdgpu: update the PASID mapping only on demand")
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b89013a6aa0b..cc79cb7dd4e1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -807,7 +807,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>  	if (cleaner_shader_needed)
>  		ring->funcs->emit_cleaner_shader(ring);
>  
> -	if (vm_flush_needed) {
> +	if (vm_flush_needed && ring->funcs->emit_vm_flush) {
>  		trace_amdgpu_vm_flush(ring, job->vmid, job->vm_pd_addr);
>  		amdgpu_ring_emit_vm_flush(ring, job->vmid, job->vm_pd_addr);
>  	}