From: Pavel Skripkin <paskripkin@gmail.com>
To: kernel test robot <rong.a.chen@intel.com>,
clang-built-linux@googlegroups.com, kbuild-all@lists.01.org,
linux-kernel@vger.kernel.org
Subject: Re: drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
Date: Mon, 9 Aug 2021 23:37:08 +0300 [thread overview]
Message-ID: <538445a9-3f96-5099-fb83-517e756a93fa@gmail.com> (raw)
In-Reply-To: <YRF1t5kx6hTrv5LC@carbon>
On 8/9/21 9:36 PM, Petko Manolov wrote:
> On 21-08-09 14:06:11, Pavel Skripkin wrote:
>> On 8/9/21 1:37 PM, kernel test robot wrote:
>> >
>> > tree:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> > head: 85a90500f9a1717c4e142ce92e6c1cb1a339ec78
>> > commit: 8a160e2e9aeb8318159b48701ad8a6e22274372d net: usb: pegasus:
>> > Check the return value of get_geristers() and friends;
>> > date: 4 days ago
>> > :::::: branch date: 8 hours ago
>> > :::::: commit date: 4 days ago
>> > config: x86_64-randconfig-c001-20210808 (attached as .config)
>> > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
>> > 41a6b50c25961addc04438b567ee1f4ef9e40f98)
>> > reproduce (this is a W=1 build):
>> > wget
>> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
>> > -O ~/bin/make.cross
>> > chmod +x ~/bin/make.cross
>> > # install x86_64 cross compiling tool for clang build
>> > # apt-get install binutils-x86-64-linux-gnu
>> > #
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a160e2e9aeb8318159b48701ad8a6e22274372d
>> > git remote add linus
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> > git fetch --no-tags linus master
>> > git checkout 8a160e2e9aeb8318159b48701ad8a6e22274372d
>> > # save the attached .config to linux build tree
>> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
>> > ARCH=x86_64 clang-analyzer
>> > If you fix the issue, kindly add following tag as appropriate
>> > Reported-by: kernel test robot <lkp@intel.com>
>>
>> Hi, @Petko!
>>
>> For you not to scan all these warnings:
>>
>> > > > drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
>> > ret = set_registers(pegasus, EthCtrl0, 3, data);
>> > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > drivers/net/usb/pegasus.c:461:2: note: Value stored to 'ret' is
>> > never read
>> > ret = set_registers(pegasus, EthCtrl0, 3, data);
>> > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> This is the real bug, I think. Can be fixed like this:
>>
>> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
>> index 22353bab76c8..f2b8891c7b36 100644
>> --- a/drivers/net/usb/pegasus.c
>> +++ b/drivers/net/usb/pegasus.c
>> @@ -459,6 +459,8 @@ static int enable_net_traffic(struct net_device *dev,
>> struct usb_device *usb)
>>
>> memcpy(pegasus->eth_regs, data, sizeof(data));
>> ret = set_registers(pegasus, EthCtrl0, 3, data);
>> + if (ret < 0)
>> + goto fail;
>>
>> if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
>> usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
>>
>>
>> It was caused by our last refactoring: enable_net_traffic() now returns 0 on
>> success and this ret is never checked.
>
> I'd rather remove the 'ret = ' part and leave set_registers() alone. If this
> particular write operation fail, it doesn't mean the adapter won't work at all.
> Perhaps it won't be the most optimal mode, but it will work. There are some
> legal checks after set_registers() that also make sense to pass. So the patch i
> suggest looks like:
>
>
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 652e9fcf0b77..49cfc720d78f 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -433,7 +433,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
> data[2] = loopback ? 0x09 : 0x01;
>
> memcpy(pegasus->eth_regs, data, sizeof(data));
> - ret = set_registers(pegasus, EthCtrl0, 3, data);
> + set_registers(pegasus, EthCtrl0, 3, data);
>
> if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS ||
> usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
>
It works. I am not aware of device specifics, so I decided to handle the
error instead of ignoring.
Will you take care of posting this patch, or I can do it with
Suggested-by tag?
With regards,
Pavel Skripkin
prev parent reply other threads:[~2021-08-09 20:37 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <202108080902.ZhmxmJZr-lkp@intel.com>
2021-08-09 10:37 ` drivers/net/usb/pegasus.c:461:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores] kernel test robot
2021-08-09 11:00 ` Pavel Skripkin
2021-08-09 11:06 ` Pavel Skripkin
2021-08-09 18:36 ` Petko Manolov
2021-08-09 20:37 ` Pavel Skripkin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=538445a9-3f96-5099-fb83-517e756a93fa@gmail.com \
--to=paskripkin@gmail.com \
--cc=clang-built-linux@googlegroups.com \
--cc=kbuild-all@lists.01.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rong.a.chen@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).