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

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

[Colin Ian King]

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;
 
 	for (i = 0; i < device->runlists; i++) {
-- 
2.39.2

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Danilo Krummrich]

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?

- Danilo

> 
> regards,
> dan carpenter
> 

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

[Dan Carpenter]

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

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

[Dan Carpenter]

Let's CC Felix on this one because he might know the answer.

All day long I spend looking at code like this:

net/core/dev.c:724 dev_fill_forward_path() info: returning a literal zero is cleaner
net/core/dev.c:732 dev_fill_forward_path() info: returning a literal zero is cleaner

net/core/dev.c
   696  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
   697                            struct net_device_path_stack *stack)
   698  {
   699          const struct net_device *last_dev;
   700          struct net_device_path_ctx ctx = {
   701                  .dev    = dev,
   702          };
   703          struct net_device_path *path;
   704          int ret = 0;
   705  
   706          memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
   707          stack->num_paths = 0;
   708          while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
   709                  last_dev = ctx.dev;
   710                  path = dev_fwd_path(stack);
   711                  if (!path)
   712                          return -1;
   713  
   714                  memset(path, 0, sizeof(struct net_device_path));
   715                  ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
   716                  if (ret < 0)

This if condition might trick you into thinking that ->ndo_fill_forward_path()
can return non-zero positive numbers, but it can't.  It returns zero on
success or negative error codes on failure.  Smatch is doing cross
function analysis so we know this.

   717                          return -1;
   718  
   719                  if (WARN_ON_ONCE(last_dev == ctx.dev))
   720                          return -1;
   721          }
   722  
   723          if (!ctx.dev)
   724                  return ret;

Is this intentional or not?  Who knows?  If this were an obvious bug,
I could fix it right away but ambiguous stuff like this takes way more
time to deal with.

   725  
   726          path = dev_fwd_path(stack);
   727          if (!path)
   728                  return -1;
   729          path->type = DEV_PATH_ETHERNET;
   730          path->dev = ctx.dev;
   731  
   732          return ret;

Obviously this is intentional, but if you were tricked by the checking
earlier then you might assume that ret is some positive value from the
last iteration through the loop.  "return 0;" is so much clearer.

   733  }

regards,
dan carpetner