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

[Dan Carpenter <dan.carpenter@linaro.org>]

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