Message ID | 20240116111609.2258675-1-colin.i.king@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:42cf:b0:101:a8e8:374 with SMTP id q15csp182605dye; Tue, 16 Jan 2024 03:16:32 -0800 (PST) X-Google-Smtp-Source: AGHT+IHf17PFvfDchsLt9m1liVoaPXzJpYIPzawrDpXIWZF0qPKjX+mb/6DAT2EAx0LpkEShky5Z X-Received: by 2002:a17:90b:104:b0:28e:8845:356a with SMTP id p4-20020a17090b010400b0028e8845356amr82247pjz.56.1705403792473; Tue, 16 Jan 2024 03:16:32 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1705403792; cv=none; d=google.com; s=arc-20160816; b=0r0gmu1L5cofXxmtrpqyrs8DdJpcWE5pQWmBts/Wwnv/1N7Vnls0KL57qXOHZNcAoV gFLUgR0Th118oUM/Rmc3sZZfeLkZNNJX1DM7CxqAUPIGt1NsPzSFt1isCJkjGTTDiUHS SDcUF6TkoZOAiJr2BtHQux2t2klygrkiSxNz4JMzQtVltOU2ZQjhSzoxGxn6Fs8mxbZ7 JyhbcHn8ysZigiLqlPPZDrwskWAlHM0/W9JnI/9LpE3/pvYjqM64+jmA7X99DBRrhD6S 9gMqiRLxYDhcUa5UqNW8y2YlSTkzF2q33azI8hmEzSoRkR4QNRGC/E9OZGA0knUVALak 6epw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=bV4EzhRkZw5eNG7ZovPTr2GfxWsv5MIBKsQhvD/ibb8=; fh=hbCOnVczf6vBPtc/YvmKZwV9bd3qG02Snja7BSK34ig=; b=AHT6Gf1pMWnPwSiu3EYf8HLebUSVuluni7wsMsxrp7xWatuUNuV1NY+JkFmvIOqcuQ M8H2d9Zr2hYkHP/uIU59aT+uDBR7k9RVXW4/CaIXrvAQXgddGA9xYVpOjbOTSANKIlQb 3MqXAqn2IMcHA5LrgQTycbsSKIj2VgLsoRh1Y6v8eH2W2k+lXKmNcUBUBC8qdAwuNY1Z irPruYxfBqbrpyAmYjFUYbZBPUTpBYu1CKrWdkprvFleFoNEBU/fmTT8qQcD2xlCBnlY ExC/zMhdFJpC+LqQBYHLtpQePbHtYuJPKDH86pb5tPiPJV+QB8y1JXMV0K1v2Y7IjLTs z4Kg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b="CpH/qViK"; spf=pass (google.com: domain of linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id gj6-20020a17090b108600b0028cbf77f0cesi13578940pjb.102.2024.01.16.03.16.32 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jan 2024 03:16:32 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b="CpH/qViK"; spf=pass (google.com: domain of linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-27302-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 9AB10285907 for <ouuuleilei@gmail.com>; Tue, 16 Jan 2024 11:16:31 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6D8771B955; Tue, 16 Jan 2024 11:16:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="CpH/qViK" Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 62A101B941; Tue, 16 Jan 2024 11:16:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-40e800461baso11416915e9.3; Tue, 16 Jan 2024 03:16:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1705403770; x=1706008570; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=bV4EzhRkZw5eNG7ZovPTr2GfxWsv5MIBKsQhvD/ibb8=; b=CpH/qViKI2WF0U05Ch5X5Je5Wd0ltw8HvN/j8jxJ1lFd9RHGiNHXGEyzt/JeXFheLd 6UHHXpd5mOIeZTH3iAWSPcXkT5SsSbKKecpxDgMjOXz8ikjh9M9nazadeCYBcfGK4Lw3 ImdkLXk//ZWbuulAMbU7bza5Q233BD2yzoL3R5Nftqx4xQ0q9DQnm9VRyXA6wnuJvGGC mGFIROezKGQulHH/6027z8RxEvojGjqYBrf55xZ4skQpBC0va1xZXRznSfAI20JeXI1S 2baRgoakt696H8R9QpgK/3t5EX5i1h2/hSLphDkDQNfm3iNWl4KwBPCfAmlcthz1GZqV TKXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705403770; x=1706008570; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bV4EzhRkZw5eNG7ZovPTr2GfxWsv5MIBKsQhvD/ibb8=; b=TDcY0yd/7C48oPIGaGd0xdx7P/VsDdZhz1kO2GBNMSTv35JRzHZxtXCm+YtCZdqclI x7ZhsmLXS30V1wLf9QqTTUYgCP2im8grgpsAesfSO+A3toh/EnXLb7K2K27eLvWEgIEP h6+N+Hm8y2ioPw+Dc0bmQyYqXsfc1qU10ducVHptn+XAid3vgApn7AQ91L3wkjw6UWw0 v763EumJG8HR5E71hk5mtmoXGy+WeoxZTGqKsicQqaAOmWsuMGPP3uVPoHSDSS9izJR+ 9Y1D6pTNx4yDgwNs6zAcfhqGeAjKVX5y9N1pGxvCTMGYfzp/V+9/K/DFfICUWelG7C1L inhQ== X-Gm-Message-State: AOJu0YyTv2r3U09FzTSRvuoa52Oe3DUN3CUF6QwVSWqHbOcfuz/uZ+fp MO6jKyIAE16KImaoj4KrXfE= X-Received: by 2002:a05:600c:3093:b0:40e:4303:df0 with SMTP id g19-20020a05600c309300b0040e43030df0mr2089782wmn.258.1705403770493; Tue, 16 Jan 2024 03:16:10 -0800 (PST) Received: from localhost (cpc154979-craw9-2-0-cust193.16-3.cable.virginm.net. [80.193.200.194]) by smtp.gmail.com with ESMTPSA id n18-20020a05600c501200b0040e77ce8768sm7102765wmr.16.2024.01.16.03.16.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jan 2024 03:16:10 -0800 (PST) From: Colin Ian King <colin.i.king@gmail.com> To: Karol Herbst <kherbst@redhat.com>, Lyude Paul <lyude@redhat.com>, Danilo Krummrich <dakr@redhat.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret Date: Tue, 16 Jan 2024 11:16:09 +0000 Message-Id: <20240116111609.2258675-1-colin.i.king@gmail.com> X-Mailer: git-send-email 2.39.2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788245487477193423 X-GMAIL-MSGID: 1788245487477193423 |
Series |
[next] drm/nouveau/fifo/gk104: remove redundant variable ret
|
|
Commit Message
Colin Ian King
Jan. 16, 2024, 11:16 a.m. UTC
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(-)
Comments
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++) {