Skip to contentSkip to conversation

usb: gadget: Drop unnecessary NULL checks after container_of

RFC PATCH Superseded Guenter Roeck 5 participants 5 messages Apr 23, 2021 – Apr 24, 2021

Subsystems (2)
USB USB webcam gadget

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Guenter Roeck

author
#0 lore raw

RFC PATCH 1/1 usb: gadget: Drop unnecessary NULL checks after container_of

The parameters passed to allow_link and drop_link functions are never NULL. That means the result of container_of() on those parameters is also never NULL, even if the reference into the structure points to the first element of the structure. Remove the subsequent NULL checks.

The changes in this patch were made automatically using the following Coccinelle script.

@@
type t;
identifier v;
statement s;
@@

<+...
(
t v = container_of(...);
|
v = container_of(...);
)
...
when != v
- if (\( !v \| v == NULL \) ) s
...+>

Cc Laurent Pinchart <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>

---
After the recent discussion about a patch which tried to add a check against NULL after container_of(), I realized that there are a number of such checks in the kernel.

Now the big question: Are patches like this acceptable, or do they count as noise ?

Guenter

 drivers/usb/gadget/function/uvc_configfs.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/usb/gadget/function/uvc_configfs.c +0 -4
index 00fb58e50a15..b9d1bcb4f4ff 100644
@@ -914,8 +914,6 @@ static int uvcg_streaming_header_allow_link(struct config_item *src,
 
 	target_fmt = container_of(to_config_group(target), struct uvcg_format,
 				  group);
-	if (!target_fmt)
-		goto out;
 
 	uvcg_format_set_indices(to_config_group(target));
 

@@ -955,8 +953,6 @@ static void uvcg_streaming_header_drop_link(struct config_item *src,
 	mutex_lock(&opts->lock);
 	target_fmt = container_of(to_config_group(target), struct uvcg_format,
 				  group);
-	if (!target_fmt)
-		goto out;
 
 	list_for_each_entry_safe(format_ptr, tmp, &src_hdr->formats, entry)
 		if (format_ptr->fmt == target_fmt) {
Signature · 1 line
2.17.1
build bot
#1 lore raw
[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]

Hi Guenter,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on peter.chen-usb/for-usb-next v5.12-rc8 next-20210423] [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/Guenter-Roeck/usb-gadget-Drop-unnecessary-NULL-checks-after-container_of/20210423-230821
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: powerpc-randconfig-r013-20210423 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 06234f758e1945084582cf80450b396f75a9c06e)

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 powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/cbd5fdce089022e1d479b647580489e84d92ca5e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guenter-Roeck/usb-gadget-Drop-unnecessary-NULL-checks-after-container_of/20210423-230821 git checkout cbd5fdce089022e1d479b647580489e84d92ca5e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'

                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:186:1: note: expanded from here
   __do_insb
   ^
   arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/usb/gadget/function/uvc_configfs.c:15:
   In file included from drivers/usb/gadget/function/u_uvc.h:17:
   In file included from include/linux/usb/composite.h:41:
   In file included from include/linux/usb/gadget.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:188:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/usb/gadget/function/uvc_configfs.c:15:
   In file included from drivers/usb/gadget/function/u_uvc.h:17:
   In file included from include/linux/usb/composite.h:41:
   In file included from include/linux/usb/gadget.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:190:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/usb/gadget/function/uvc_configfs.c:15:
   In file included from drivers/usb/gadget/function/u_uvc.h:17:
   In file included from include/linux/usb/composite.h:41:
   In file included from include/linux/usb/gadget.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:192:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/usb/gadget/function/uvc_configfs.c:15:
   In file included from drivers/usb/gadget/function/u_uvc.h:17:
   In file included from include/linux/usb/composite.h:41:
   In file included from include/linux/usb/gadget.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:194:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/usb/gadget/function/uvc_configfs.c:15:
   In file included from drivers/usb/gadget/function/u_uvc.h:17:
   In file included from include/linux/usb/composite.h:41:
   In file included from include/linux/usb/gadget.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:196:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
>> drivers/usb/gadget/function/uvc_configfs.c:967:1: warning: unused label 'out' [-Wunused-label]

out:
^~~~
13 warnings generated.

vim +/out +967 drivers/usb/gadget/function/uvc_configfs.c

46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  936  
e16769d4bca672 Andrzej Pietrasiewicz 2016-11-28  937  static void uvcg_streaming_header_drop_link(struct config_item *src,
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  938  					   struct config_item *target)
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  939  {
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  940  	struct mutex *su_mutex = &src->ci_group->cg_subsys->su_mutex;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  941  	struct config_item *opts_item;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  942  	struct f_uvc_opts *opts;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  943  	struct uvcg_streaming_header *src_hdr;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  944  	struct uvcg_format *target_fmt = NULL;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  945  	struct uvcg_format_ptr *format_ptr, *tmp;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  946  
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  947  	src_hdr = to_uvcg_streaming_header(src);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  948  	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  949  
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  950  	opts_item = src->ci_parent->ci_parent->ci_parent;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  951  	opts = to_f_uvc_opts(opts_item);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  952  
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  953  	mutex_lock(&opts->lock);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  954  	target_fmt = container_of(to_config_group(target), struct uvcg_format,
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  955  				  group);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  956  
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  957  	list_for_each_entry_safe(format_ptr, tmp, &src_hdr->formats, entry)
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  958  		if (format_ptr->fmt == target_fmt) {
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  959  			list_del(&format_ptr->entry);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  960  			kfree(format_ptr);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  961  			--src_hdr->num_fmt;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  962  			break;
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  963  		}
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  964  
cb2200f7af8341 Joel Pepper           2018-05-29  965  	--target_fmt->linked;
cb2200f7af8341 Joel Pepper           2018-05-29  966  
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10 @967  out:
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  968  	mutex_unlock(&opts->lock);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  969  	mutex_unlock(su_mutex);
46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10  970  }

46919a23ee87bb Andrzej Pietrasiewicz 2014-12-10 971

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

maintainer
#2 lore raw
In reply to Guenter Roeck
Quoted 29 lines · Guenter Roeck

The parameters passed to allow_link and drop_link functions are never NULL. That means the result of container_of() on those parameters is also never NULL, even if the reference into the structure points to the first element of the structure. Remove the subsequent NULL checks.

The changes in this patch were made automatically using the following Coccinelle script.

@@
type t;
identifier v;
statement s;
@@

<+...
(
t v = container_of(...);
|
v = container_of(...);
)
...
when != v
- if (\( !v \| v == NULL \) ) s
...+>

Cc Laurent Pinchart <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
Quoted 7 lines

---
After the recent discussion about a patch which tried to add a check against NULL after container_of(), I realized that there are a number of such checks in the kernel.

Now the big question: Are patches like this acceptable, or do they count as noise ?

Yes they are acceptable, and no, they are not noise.

I will be glad to take this after -rc1 is out, thanks.

thanks,

greg k-h

Acked-by on v1 1/1
#3 lore raw
In reply to Guenter Roeck

Hi Guenter,

Quoted 29 lines · Guenter Roeck

The parameters passed to allow_link and drop_link functions are never NULL. That means the result of container_of() on those parameters is also never NULL, even if the reference into the structure points to the first element of the structure. Remove the subsequent NULL checks.

The changes in this patch were made automatically using the following Coccinelle script.

@@
type t;
identifier v;
statement s;
@@

<+...
(
t v = container_of(...);
|
v = container_of(...);
)
...
when != v
- if (\( !v \| v == NULL \) ) s
...+>

Cc Laurent Pinchart <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
Quoted 7 lines

---
After the recent discussion about a patch which tried to add a check against NULL after container_of(), I realized that there are a number of such checks in the kernel.

Now the big question: Are patches like this acceptable, or do they count as noise ?

Not noise in my book :-)

Acked-by Felipe Balbi <[email protected]>
Signature · 1 line
balbi
Reviewed-by
#4 lore raw

Hi Guenter,

Quoted 35 lines · Felipe Balbi
Quoted 34 lines · Guenter Roeck

The parameters passed to allow_link and drop_link functions are never NULL. That means the result of container_of() on those parameters is also never NULL, even if the reference into the structure points to the first element of the structure. Remove the subsequent NULL checks.

The changes in this patch were made automatically using the following Coccinelle script.

@@
type t;
identifier v;
statement s;
@@

<+...
(
t v = container_of(...);
|
v = container_of(...);
)
...
when != v
- if (\( !v \| v == NULL \) ) s
...+>

Cc Laurent Pinchart <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>

---
After the recent discussion about a patch which tried to add a check against NULL after container_of(), I realized that there are a number of such checks in the kernel.

Quoted 6 lines
Quoted 2 lines

Now the big question: Are patches like this acceptable, or do they count as noise ?

Not noise in my book :-)

Acked-by Felipe Balbi <[email protected]>

Likewise,

Reviewed-by Laurent Pinchart <[email protected]>

And thank you for the patch.

Signature · 3 lines
Regards,

Laurent Pinchart