Re: [PATCH v4 1/2] media: rc: new driver for USB-UIRT device

[Nathan Chancellor <nathan@kernel.org>]

On 6/18/2021 1:44 AM, Sean Young wrote:
> On Fri, Jun 18, 2021 at 06:18:06AM +0800, kernel test robot wrote:
>> Hi Sean,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on linuxtv-media/master]
>> [also build test WARNING on usb-serial/usb-next usb/usb-testing peter.chen-usb/for-usb-next v5.13-rc6 next-20210617]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    https://github.com/0day-ci/linux/commits/Sean-Young/IR-driver-for-USB-UIRT-device/20210616-182135
>> base:   git://linuxtv.org/media_tree.git master
>> config: powerpc64-randconfig-r012-20210617 (attached as .config)
>> compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
>> 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 powerpc64 cross compiling tool for clang build
>>          # apt-get install binutils-powerpc64-linux-gnu
>>          # https://github.com/0day-ci/linux/commit/17d3a0332baecb0359e05e8ae755478c7a1a4468
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review Sean-Young/IR-driver-for-USB-UIRT-device/20210616-182135
>>          git checkout 17d3a0332baecb0359e05e8ae755478c7a1a4468
>>          # save the attached .config to linux build tree
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All warnings (new ones prefixed by >>):
>>
>>     In file included from drivers/media/rc/uirt.c:11:
>>     In file included from include/linux/completion.h:12:
>>     In file included from include/linux/swait.h:5:
>>     In file included from include/linux/list.h:9:
>>     In file included from include/linux/kernel.h:12:
>>     In file included from include/linux/bitops.h:32:
>>     In file included from arch/powerpc/include/asm/bitops.h:62:
>>     arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
>>     #define __lwsync()      __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
>>             ^
>>     <built-in>:310:9: note: previous definition is here
>>     #define __lwsync __builtin_ppc_lwsync
>>             ^
>>>> drivers/media/rc/uirt.c:639:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>             if (!urb)
>>                 ^~~~
>>     drivers/media/rc/uirt.c:705:9: note: uninitialized use occurs here
>>             return err;
> 
> This is interesting. clang is right here, there are error paths where err is
> not initialized. gcc-11.1 does not pick this up for some reason. The error path
> should be an immediate dominator so it shouldn't be complicated to detect.

The reason GCC does not warn about this is due to commit 78a5255ffb6a 
("Stop the ad-hoc games with -Wno-maybe-initialized"), which disables 
the GCC version of this warning except with W=2, which very few people 
use. You could use 'KCFLAGS=-Wmaybe-uninitialized' to try and see the 
same warning.

Cheers,
Nathan

> I'll send out a v5 with this issue fixed.
> 
> Sean
> 
>>                    ^~~
>>     drivers/media/rc/uirt.c:639:2: note: remove the 'if' if its condition is always false
>>             if (!urb)
>>             ^~~~~~~~~
>>     drivers/media/rc/uirt.c:630:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>             if (!urb)
>>                 ^~~~
>>     drivers/media/rc/uirt.c:705:9: note: uninitialized use occurs here
>>             return err;
>>                    ^~~
>>     drivers/media/rc/uirt.c:630:2: note: remove the 'if' if its condition is always false
>>             if (!urb)
>>             ^~~~~~~~~
>>     drivers/media/rc/uirt.c:626:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>             if (!rc)
>>                 ^~~
>>     drivers/media/rc/uirt.c:705:9: note: uninitialized use occurs here
>>             return err;
>>                    ^~~
>>     drivers/media/rc/uirt.c:626:2: note: remove the 'if' if its condition is always false
>>             if (!rc)
>>             ^~~~~~~~
>>     drivers/media/rc/uirt.c:622:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>             if (!uirt->out)
>>                 ^~~~~~~~~~
>>     drivers/media/rc/uirt.c:705:9: note: uninitialized use occurs here
>>             return err;
>>                    ^~~
>>     drivers/media/rc/uirt.c:622:2: note: remove the 'if' if its condition is always false
>>             if (!uirt->out)
>>             ^~~~~~~~~~~~~~~
>>     drivers/media/rc/uirt.c:618:6: warning: variable 'err' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>>             if (!uirt->in)
>>                 ^~~~~~~~~
>>     drivers/media/rc/uirt.c:705:9: note: uninitialized use occurs here
>>             return err;
>>                    ^~~
>>     drivers/media/rc/uirt.c:618:2: note: remove the 'if' if its condition is always false
>>             if (!uirt->in)
>>             ^~~~~~~~~~~~~~
>>     drivers/media/rc/uirt.c:604:15: note: initialize the variable 'err' to silence this warning
>>             int pipe, err;
>>                          ^
>>                           = 0
>>     6 warnings generated.
>>
>>
>> vim +639 drivers/media/rc/uirt.c
>>
>>     594	
>>     595	static int uirt_probe(struct usb_interface *intf,
>>     596			      const struct usb_device_id *id)
>>     597	{
>>     598		struct usb_device *usbdev = interface_to_usbdev(intf);
>>     599		struct usb_endpoint_descriptor *ep_in;
>>     600		struct usb_endpoint_descriptor *ep_out;
>>     601		struct uirt *uirt;
>>     602		struct rc_dev *rc;
>>     603		struct urb *urb;
>>     604		int pipe, err;
>>     605	
>>     606		if (usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out, NULL, NULL) ||
>>     607		    usb_endpoint_maxp(ep_in) != MAX_PACKET ||
>>     608		    usb_endpoint_maxp(ep_out) != MAX_PACKET) {
>>     609			dev_err(&intf->dev, "required endpoints not found\n");
>>     610			return -ENODEV;
>>     611		}
>>     612	
>>     613		uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
>>     614		if (!uirt)
>>     615			return -ENOMEM;
>>     616	
>>     617		uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
>>     618		if (!uirt->in)
>>     619			goto free_uirt;
>>     620	
>>     621		uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
>>     622		if (!uirt->out)
>>     623			goto free_uirt;
>>     624	
>>     625		rc = rc_allocate_device(RC_DRIVER_IR_RAW);
>>     626		if (!rc)
>>     627			goto free_uirt;
>>     628	
>>     629		urb = usb_alloc_urb(0, GFP_KERNEL);
>>     630		if (!urb)
>>     631			goto free_rcdev;
>>     632	
>>     633		pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
>>     634		usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
>>     635				  uirt_in_callback, uirt);
>>     636		uirt->urb_in = urb;
>>     637	
>>     638		urb = usb_alloc_urb(0, GFP_KERNEL);
>>   > 639		if (!urb)
>>     640			goto free_rcdev;
>>     641	
>>     642		pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
>>     643		usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
>>     644				  uirt_out_callback, uirt);
>>     645	
>>     646		uirt->dev = &intf->dev;
>>     647		uirt->usbdev = usbdev;
>>     648		uirt->rc = rc;
>>     649		uirt->urb_out = urb;
>>     650		uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
>>     651	
>>     652		err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
>>     653		if (err != 0) {
>>     654			dev_err(uirt->dev, "failed to submit read urb: %d\n", err);
>>     655			goto free_rcdev;
>>     656		}
>>     657	
>>     658		err = init_ftdi(usbdev);
>>     659		if (err) {
>>     660			dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
>>     661			goto kill_urbs;
>>     662		}
>>     663	
>>     664		err = uirt_setup(uirt);
>>     665		if (err)
>>     666			goto kill_urbs;
>>     667	
>>     668		usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
>>     669	
>>     670		rc->device_name = "USB-UIRT";
>>     671		rc->driver_name = KBUILD_MODNAME;
>>     672		rc->input_phys = uirt->phys;
>>     673		usb_to_input_id(usbdev, &rc->input_id);
>>     674		rc->dev.parent = &intf->dev;
>>     675		rc->priv = uirt;
>>     676		rc->tx_ir = uirt_tx;
>>     677		rc->s_tx_carrier = uirt_set_tx_carrier;
>>     678		rc->s_learning_mode = uirt_set_rx_wideband;
>>     679		rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>>     680		rc->map_name = RC_MAP_RC6_MCE;
>>     681		rc->rx_resolution = UNIT_US;
>>     682		rc->timeout = IR_TIMEOUT;
>>     683	
>>     684		uirt_set_tx_carrier(rc, 38000);
>>     685	
>>     686		err = rc_register_device(rc);
>>     687		if (err)
>>     688			goto kill_urbs;
>>     689	
>>     690		usb_set_intfdata(intf, uirt);
>>     691	
>>     692		return 0;
>>     693	
>>     694	kill_urbs:
>>     695		usb_kill_urb(uirt->urb_in);
>>     696		usb_kill_urb(uirt->urb_out);
>>     697	free_rcdev:
>>     698		usb_free_urb(uirt->urb_in);
>>     699		usb_free_urb(uirt->urb_out);
>>     700		rc_free_device(rc);
>>     701	free_uirt:
>>     702		kfree(uirt->in);
>>     703		kfree(uirt->out);
>>     704		kfree(uirt);
>>     705		return err;
>>     706	}
>>     707	
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 
>