Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret

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

On Tue, Jan 23, 2024 at 12:04:23AM +0100, Danilo Krummrich wrote:
> On 1/16/24 13:31, Dan Carpenter wrote:
> > On Tue, Jan 16, 2024 at 11:16:09AM +0000, Colin Ian King wrote:
> > > The variable ret is being assigned a value but it isn't being
> > > read afterwards. The assignment is redundant and so ret can be
> > > removed.
> > > 
> > > Cleans up clang scan build warning:
> > > warning: Although the value stored to 'ret' is used in the enclosing
> > > expression, the value is never actually read from 'ret'
> > > [deadcode.DeadStores]
> > > 
> > > Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
> > > ---
> > >   drivers/gpu/drm/nouveau/nvif/fifo.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nvif/fifo.c b/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > index a463289962b2..e96de14ce87e 100644
> > > --- a/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > +++ b/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > @@ -73,9 +73,9 @@ u64
> > >   nvif_fifo_runlist(struct nvif_device *device, u64 engine)
> > >   {
> > >   	u64 runm = 0;
> > > -	int ret, i;
> > > +	int i;
> > > -	if ((ret = nvif_fifo_runlists(device)))
> > > +	if (nvif_fifo_runlists(device))
> > >   		return runm;
> > 
> > Could we return a literal zero here?  Otherwise, I'm surprised this
> > doesn't trigger a static checker warning.
> 
> Why do you think so? Conditionally, runm is used later on as well. I don't
> think the checker should complain about keeping the value single source.
> 
> If you agree, want to offer your RB?

If you look at v6.7 then probably 300 patches were from static
analysis.  The syzbot gets credit for 63 bugs and those bugs are more
important because those are real life bugs.  But static analysis is a
huge in terms of just quantity.

One of the most common bugs that static checkers complain about is
missing error codes.  It's a super common bug.  Returning success
instead of failure almost always results in NULL dereference or a use
after free or some kind of crash.  Fortunately, error paths seldom
affect real life users.

My published Smatch checks only complain about:

	if (ret)
		return ret;

	if (failure)
		return ret;

I have a different check that I haven't published but I wish that I
could which looks like:

	if (!ret)
		return ret;

Here is a bug that check found recently.
https://lore.kernel.org/all/9c81251b-bc87-4ca3-bb86-843dc85e5145@moroto.mountain/

I have a different unpublished check for every time ret is zero and we
do:
	return ret;

But I only review those warnings for specific code.  Perhaps, I could
make a warning for:

	if (failure)
		return ret;

I'm sure I tried this in the past and it had more false positives than
when we have an "if (ret) return ret;" like in the first example, but I
can't remember.  I could experiment with that a bit...

To me, if "return ret;" and "return 0;" are the same, then "return 0;"
is obviously more clear and looks more intentional.  When I was looking
at the code here, I had to consider the context.  Especially when the
patch was dealing with the "ret" variable it seemed suspicous.  But
"return 0;" is unamibuous.

I don't have a problem with this patch, it's correct.  But I really do
think that "return 0;" is clearer than "return ret;"

regards,
dan carpenter