tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-linus
head: 3776c78559853fd151be7c41e369fd076fb679d5
commit: 3776c78559853fd151be7c41e369fd076fb679d5 [3/3] misc: rtsx_usb: use separate command and response buffers
config: arm-buildonly-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220701/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
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 arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=3776c78559853fd151be7c41e369fd076fb679d5
git remote add char-misc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git fetch --no-tags char-misc char-misc-linus
git checkout 3776c78559853fd151be7c41e369fd076fb679d5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/misc/cardreader/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!ucr->rsp_buf)
^~~~~~~~~~~~~
drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
return ret;
^~~
drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
if (!ucr->rsp_buf)
^~~~~~~~~~~~~~~~~~
drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
1 warning generated.
vim +639 drivers/misc/cardreader/rtsx_usb.c
616
617 static int rtsx_usb_probe(struct usb_interface *intf,
618 const struct usb_device_id *id)
619 {
620 struct usb_device *usb_dev = interface_to_usbdev(intf);
621 struct rtsx_ucr *ucr;
622 int ret;
623
624 dev_dbg(&intf->dev,
625 ": Realtek USB Card Reader found at bus %03d address %03d\n",
626 usb_dev->bus->busnum, usb_dev->devnum);
627
628 ucr = devm_kzalloc(&intf->dev, sizeof(*ucr), GFP_KERNEL);
629 if (!ucr)
630 return -ENOMEM;
631
632 ucr->pusb_dev = usb_dev;
633
634 ucr->cmd_buf = kmalloc(IOBUF_SIZE, GFP_KERNEL);
635 if (!ucr->cmd_buf)
636 return -ENOMEM;
637
638 ucr->rsp_buf = kmalloc(IOBUF_SIZE, GFP_KERNEL);
> 639 if (!ucr->rsp_buf)
640 goto out_free_cmd_buf;
641
642 usb_set_intfdata(intf, ucr);
643
644 ucr->vendor_id = id->idVendor;
645 ucr->product_id = id->idProduct;
646
647 mutex_init(&ucr->dev_mutex);
648
649 ucr->pusb_intf = intf;
650
651 /* initialize */
652 ret = rtsx_usb_init_chip(ucr);
653 if (ret)
654 goto out_init_fail;
655
656 /* initialize USB SG transfer timer */
657 timer_setup(&ucr->sg_timer, rtsx_usb_sg_timed_out, 0);
658
659 ret = mfd_add_hotplug_devices(&intf->dev, rtsx_usb_cells,
660 ARRAY_SIZE(rtsx_usb_cells));
661 if (ret)
662 goto out_init_fail;
663
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Fri, Jul 01, 2022 at 04:49:50PM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-linus
> head: 3776c78559853fd151be7c41e369fd076fb679d5
> commit: 3776c78559853fd151be7c41e369fd076fb679d5 [3/3] misc: rtsx_usb: use separate command and response buffers
> config: arm-buildonly-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220701/[email protected]/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
> 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 arm cross compiling tool for clang build
> # apt-get install binutils-arm-linux-gnueabi
> # https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=3776c78559853fd151be7c41e369fd076fb679d5
> git remote add char-misc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> git fetch --no-tags char-misc char-misc-linus
> git checkout 3776c78559853fd151be7c41e369fd076fb679d5
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/misc/cardreader/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> if (!ucr->rsp_buf)
> ^~~~~~~~~~~~~
> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
> return ret;
> ^~~
> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
> if (!ucr->rsp_buf)
> ^~~~~~~~~~~~~~~~~~
> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
> int ret;
> ^
> = 0
> 1 warning generated.
Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
patch to fix this? The warning does look correct.
thanks,
greg k-h
On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 04:49:50PM +0800, kernel test robot wrote:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-linus
>> head: 3776c78559853fd151be7c41e369fd076fb679d5
>> commit: 3776c78559853fd151be7c41e369fd076fb679d5 [3/3] misc: rtsx_usb: use separate command and response buffers
>> config: arm-buildonly-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220701/[email protected]/config)
>> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
>> 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 arm cross compiling tool for clang build
>> # apt-get install binutils-arm-linux-gnueabi
>> # https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=3776c78559853fd151be7c41e369fd076fb679d5
>> git remote add char-misc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
>> git fetch --no-tags char-misc char-misc-linus
>> git checkout 3776c78559853fd151be7c41e369fd076fb679d5
>> # save the config file
>> mkdir build_dir && cp config build_dir/.config
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/misc/cardreader/
>>
>> If you fix the issue, kindly add following tag where applicable
>> Reported-by: kernel test robot <[email protected]>
>>
>> All warnings (new ones prefixed by >>):
>>
>>>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>> if (!ucr->rsp_buf)
>> ^~~~~~~~~~~~~
>> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
>> return ret;
>> ^~~
>> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
>> if (!ucr->rsp_buf)
>> ^~~~~~~~~~~~~~~~~~
>> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
>> int ret;
>> ^
>> = 0
>> 1 warning generated.
>
> Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
> patch to fix this? The warning does look correct.
>
gcc didn't complain when I compiled either. I will send a follow-on patch.
thanks,
-- Shuah
On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote:
> On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 04:49:50PM +0800, kernel test robot wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-linus
> > > head: 3776c78559853fd151be7c41e369fd076fb679d5
> > > commit: 3776c78559853fd151be7c41e369fd076fb679d5 [3/3] misc: rtsx_usb: use separate command and response buffers
> > > config: arm-buildonly-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220701/[email protected]/config)
> > > compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
> > > 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 arm cross compiling tool for clang build
> > > # apt-get install binutils-arm-linux-gnueabi
> > > # https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=3776c78559853fd151be7c41e369fd076fb679d5
> > > git remote add char-misc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> > > git fetch --no-tags char-misc char-misc-linus
> > > git checkout 3776c78559853fd151be7c41e369fd076fb679d5
> > > # save the config file
> > > mkdir build_dir && cp config build_dir/.config
> > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/misc/cardreader/
> > >
> > > If you fix the issue, kindly add following tag where applicable
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > > > drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> > > if (!ucr->rsp_buf)
> > > ^~~~~~~~~~~~~
> > > drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
> > > return ret;
> > > ^~~
> > > drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
> > > if (!ucr->rsp_buf)
> > > ^~~~~~~~~~~~~~~~~~
> > > drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
> > > int ret;
> > > ^
> > > = 0
> > > 1 warning generated.
> >
> > Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
> > patch to fix this? The warning does look correct.
> >
>
> gcc didn't complain when I compiled either. I will send a follow-on patch.
Unfortunately, GCC won't warn for most uninitialized variables by
default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc
games with -Wno-maybe-initialized"). They will potentially show up at
W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this
case):
| drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’:
| drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
| 678 | return ret;
| | ^~~
| drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here
| 622 | int ret;
| | ^~~
| cc1: all warnings being treated as errors
Cheers,
Nathan
On 7/1/22 9:39 AM, Nathan Chancellor wrote:
> On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote:
>> On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
>>> On Fri, Jul 01, 2022 at 04:49:50PM +0800, kernel test robot wrote:
>>>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git char-misc-linus
>>>> head: 3776c78559853fd151be7c41e369fd076fb679d5
>>>> commit: 3776c78559853fd151be7c41e369fd076fb679d5 [3/3] misc: rtsx_usb: use separate command and response buffers
>>>> config: arm-buildonly-randconfig-r006-20220629 (https://download.01.org/0day-ci/archive/20220701/[email protected]/config)
>>>> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
>>>> 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 arm cross compiling tool for clang build
>>>> # apt-get install binutils-arm-linux-gnueabi
>>>> # https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=3776c78559853fd151be7c41e369fd076fb679d5
>>>> git remote add char-misc https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
>>>> git fetch --no-tags char-misc char-misc-linus
>>>> git checkout 3776c78559853fd151be7c41e369fd076fb679d5
>>>> # save the config file
>>>> mkdir build_dir && cp config build_dir/.config
>>>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/misc/cardreader/
>>>>
>>>> If you fix the issue, kindly add following tag where applicable
>>>> Reported-by: kernel test robot <[email protected]>
>>>>
>>>> All warnings (new ones prefixed by >>):
>>>>
>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>>> if (!ucr->rsp_buf)
>>>> ^~~~~~~~~~~~~
>>>> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
>>>> return ret;
>>>> ^~~
>>>> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
>>>> if (!ucr->rsp_buf)
>>>> ^~~~~~~~~~~~~~~~~~
>>>> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
>>>> int ret;
>>>> ^
>>>> = 0
>>>> 1 warning generated.
>>>
>>> Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
>>> patch to fix this? The warning does look correct.
>>>
>>
>> gcc didn't complain when I compiled either. I will send a follow-on patch.
>
> Unfortunately, GCC won't warn for most uninitialized variables by
> default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc
> games with -Wno-maybe-initialized"). They will potentially show up at
> W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this
> case):
>
Thank you.
> | drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’:
> | drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> | 678 | return ret;
> | | ^~~
> | drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here
> | 622 | int ret;
> | | ^~~
> | cc1: all warnings being treated as errors
>
This is a bug and a good find. ret should have been set
in the if (!ucr->rsp_buf) before going to error handling.
I wonder if it would have been flagged if ret were to be
initialized to 0. Something to experiment.
thanks,
-- Shuah
On Fri, Jul 01, 2022 at 10:09:45AM -0600, Shuah Khan wrote:
> On 7/1/22 9:52 AM, Shuah Khan wrote:
> > On 7/1/22 9:39 AM, Nathan Chancellor wrote:
> > > On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote:
> > > > On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
>
> > > > > > All warnings (new ones prefixed by >>):
> > > > > >
> > > > > > > > drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> > > > > > if (!ucr->rsp_buf)
> > > > > > ^~~~~~~~~~~~~
> > > > > > drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
> > > > > > return ret;
> > > > > > ^~~
> > > > > > drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
> > > > > > if (!ucr->rsp_buf)
> > > > > > ^~~~~~~~~~~~~~~~~~
> > > > > > drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
> > > > > > int ret;
> > > > > > ^
> > > > > > = 0
> > > > > > 1 warning generated.
> > > > >
> > > > > Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
> > > > > patch to fix this? The warning does look correct.
> > > > >
> > > >
> > > > gcc didn't complain when I compiled either. I will send a follow-on patch.
> > >
> > > Unfortunately, GCC won't warn for most uninitialized variables by
> > > default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc
> > > games with -Wno-maybe-initialized"). They will potentially show up at
> > > W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this
> > > case):
> > >
> >
> > Thank you.
> >
> > > | drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’:
> > > | drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
> > > | 678 | return ret;
> > > | | ^~~
> > > | drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here
> > > | 622 | int ret;
> > > | | ^~~
> > > | cc1: all warnings being treated as errors
> > >
> >
> > This is a bug and a good find. ret should have been set
> > in the if (!ucr->rsp_buf) before going to error handling.
> >
> > I wonder if it would have been flagged if ret were to be
> > initialized to 0. Something to experiment.
> >
>
> I had to try. As I suspected initializing ret will mask this bug.
>
> KCFLAGS=-Wmaybe-uninitialized will not flag it even though
> the bug still exists. It will return 0 w hen memory allocation
> fails.
Right, if the variable is unconditonally initialized at the top of the
function, it can never be flagged as uninitialized but that doesn't mean
the value is correct for every branch within the function.
> Initializing isn't always the right answer for these kinds of
> warnings.
I would say "unconditional initialization", as ret has to be initialized
somewhere to fix this warning, right? If you are referring to clang's
'ret = 0' suggestion, I agree that it can be misleading (especially in
the case of suggesting initializing dereferenced pointers to NULL...,
see [1]) but at the end of the day, it is still on the programmer to
analyze their code fully in light of the warning before implementing a
fix. Clang cannot know that the return code should be set in the if
statement rather than at the top of the function.
[1]: https://github.com/llvm/llvm-project/issues/51901
Cheers,
Nathan
On 7/1/22 9:52 AM, Shuah Khan wrote:
> On 7/1/22 9:39 AM, Nathan Chancellor wrote:
>> On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote:
>>> On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
>>>>> All warnings (new ones prefixed by >>):
>>>>>
>>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>>>> if (!ucr->rsp_buf)
>>>>> ^~~~~~~~~~~~~
>>>>> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
>>>>> return ret;
>>>>> ^~~
>>>>> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
>>>>> if (!ucr->rsp_buf)
>>>>> ^~~~~~~~~~~~~~~~~~
>>>>> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
>>>>> int ret;
>>>>> ^
>>>>> = 0
>>>>> 1 warning generated.
>>>>
>>>> Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
>>>> patch to fix this? The warning does look correct.
>>>>
>>>
>>> gcc didn't complain when I compiled either. I will send a follow-on patch.
>>
>> Unfortunately, GCC won't warn for most uninitialized variables by
>> default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc
>> games with -Wno-maybe-initialized"). They will potentially show up at
>> W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this
>> case):
>>
>
> Thank you.
>
>> | drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’:
>> | drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
>> | 678 | return ret;
>> | | ^~~
>> | drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here
>> | 622 | int ret;
>> | | ^~~
>> | cc1: all warnings being treated as errors
>>
>
> This is a bug and a good find. ret should have been set
> in the if (!ucr->rsp_buf) before going to error handling.
>
> I wonder if it would have been flagged if ret were to be
> initialized to 0. Something to experiment.
>
I had to try. As I suspected initializing ret will mask this bug.
KCFLAGS=-Wmaybe-uninitialized will not flag it even though
the bug still exists. It will return 0 when memory allocation
fails.
Initializing isn't always the right answer for these kinds of
warnings.
thanks,
-- Shuah
On 7/1/22 10:27 AM, Nathan Chancellor wrote:
> On Fri, Jul 01, 2022 at 10:09:45AM -0600, Shuah Khan wrote:
>> On 7/1/22 9:52 AM, Shuah Khan wrote:
>>> On 7/1/22 9:39 AM, Nathan Chancellor wrote:
>>>> On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote:
>>>>> On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote:
>>
>>>>>>> All warnings (new ones prefixed by >>):
>>>>>>>
>>>>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>>>>>> if (!ucr->rsp_buf)
>>>>>>> ^~~~~~~~~~~~~
>>>>>>> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here
>>>>>>> return ret;
>>>>>>> ^~~
>>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false
>>>>>>> if (!ucr->rsp_buf)
>>>>>>> ^~~~~~~~~~~~~~~~~~
>>>>>>> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning
>>>>>>> int ret;
>>>>>>> ^
>>>>>>> = 0
>>>>>>> 1 warning generated.
>>>>>>
>>>>>> Odd, gcc doesn't show this for me. Shuah, can you send a follow-on
>>>>>> patch to fix this? The warning does look correct.
>>>>>>
>>>>>
>>>>> gcc didn't complain when I compiled either. I will send a follow-on patch.
>>>>
>>>> Unfortunately, GCC won't warn for most uninitialized variables by
>>>> default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc
>>>> games with -Wno-maybe-initialized"). They will potentially show up at
>>>> W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this
>>>> case):
>>>>
>>>
>>> Thank you.
>>>
>>>> | drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’:
>>>> | drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>> | 678 | return ret;
>>>> | | ^~~
>>>> | drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here
>>>> | 622 | int ret;
>>>> | | ^~~
>>>> | cc1: all warnings being treated as errors
>>>>
>>>
>>> This is a bug and a good find. ret should have been set
>>> in the if (!ucr->rsp_buf) before going to error handling.
>>>
>>> I wonder if it would have been flagged if ret were to be
>>> initialized to 0. Something to experiment.
>>>
>>
>> I had to try. As I suspected initializing ret will mask this bug.
>>
>> KCFLAGS=-Wmaybe-uninitialized will not flag it even though
>> the bug still exists. It will return 0 w hen memory allocation
>> fails.
>
> Right, if the variable is unconditonally initialized at the top of the
> function, it can never be flagged as uninitialized but that doesn't mean
> the value is correct for every branch within the function.
>
+1
>> Initializing isn't always the right answer for these kinds of
>> warnings.
>
> I would say "unconditional initialization", as ret has to be initialized
> somewhere to fix this warning, right? If you are referring to clang's
> 'ret = 0' suggestion, I agree that it can be misleading (especially in
> the case of suggesting initializing dereferenced pointers to NULL...,
> see [1]) but at the end of the day, it is still on the programmer to
> analyze their code fully in light of the warning before implementing a
> fix. Clang cannot know that the return code should be set in the if
> statement rather than at the top of the function.
>
+1 on "programmer to analyze their code fully in light of the warning"
I am not necessarily referring to the clang warning. I agree that it could
be misleading and suggest or send us towards a wrong fix. It is on us to
be careful - this would be when fixing the problem and reviewing the fix.
thanks,
-- Shuah