| Message ID | 20240116111609.2258675-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] drm/nouveau/fifo/gk104: remove redundant variable ret | expand |
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
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 >
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
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
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++) {
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(-)