[PATCH] Staging: nvec: Change container_of macro to an inline function.

Sumitra Sharma posted 1 patch 1 year ago
drivers/staging/nvec/nvec_paz00.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by Sumitra Sharma 1 year ago
The macro has the drawback that one cannot determine
what type it applies to by looking at the definition.
Hence this macro definition is not type-safe.

The inline function gives the same benefits as the
macro and only accepts the specific type of arguments.
Use static because the definition only requires it to be
visible in the current file.

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---
 drivers/staging/nvec/nvec_paz00.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/nvec/nvec_paz00.c b/drivers/staging/nvec/nvec_paz00.c
index 8b4da95081c8..9573ba762cdd 100644
--- a/drivers/staging/nvec/nvec_paz00.c
+++ b/drivers/staging/nvec/nvec_paz00.c
@@ -14,8 +14,10 @@
 #include <linux/platform_device.h>
 #include "nvec.h"
 
-#define to_nvec_led(led_cdev) \
-	container_of(led_cdev, struct nvec_led, cdev)
+static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
+{
+	return container_of(led_cdev, struct nvec_led, cdev);
+}
 
 #define NVEC_LED_REQ {'\x0d', '\x10', '\x45', '\x10', '\x00'}
 
-- 
2.25.1
Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by kernel test robot 1 year ago
Hi Sumitra,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Sumitra-Sharma/Staging-nvec-Change-container_of-macro-to-an-inline-function/20230319-010628
patch link:    https://lore.kernel.org/r/20230318170514.GA49181%40sumitra.com
patch subject: [PATCH] Staging: nvec: Change container_of macro to an inline function.
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230319/202303190432.kRoDGmiE-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
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
        # https://github.com/intel-lab-lkp/linux/commit/e58807d27f0ba705144bce72751f5cb0a75b9682
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sumitra-Sharma/Staging-nvec-Change-container_of-macro-to-an-inline-function/20230319-010628
        git checkout e58807d27f0ba705144bce72751f5cb0a75b9682
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/staging/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303190432.kRoDGmiE-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/container_of.h:5,
                    from include/linux/list.h:5,
                    from include/linux/module.h:12,
                    from drivers/staging/nvec/nvec_paz00.c:10:
   drivers/staging/nvec/nvec_paz00.c: In function 'to_nvec_led':
>> include/linux/container_of.h:20:54: error: invalid use of undefined type 'struct nvec_led'
      20 |         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
         |                                                      ^~
   include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
      78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
         |                                                        ^~~~
   include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
      20 |         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
         |         ^~~~~~~~~~~~~
   include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
      20 |         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
         |                       ^~~~~~~~~~~
   drivers/staging/nvec/nvec_paz00.c:19:16: note: in expansion of macro 'container_of'
      19 |         return container_of(led_cdev, struct nvec_led, cdev);
         |                ^~~~~~~~~~~~
   include/linux/compiler_types.h:338:27: error: expression in static assertion is not an integer
     338 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
      78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
         |                                                        ^~~~
   include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
      20 |         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
         |         ^~~~~~~~~~~~~
   include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
      20 |         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
         |                       ^~~~~~~~~~~
   drivers/staging/nvec/nvec_paz00.c:19:16: note: in expansion of macro 'container_of'
      19 |         return container_of(led_cdev, struct nvec_led, cdev);
         |                ^~~~~~~~~~~~
   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/types.h:6,
                    from include/linux/kasan-checks.h:5,
                    from include/asm-generic/rwonce.h:26,
                    from ./arch/arm/include/generated/asm/rwonce.h:1,
                    from include/linux/compiler.h:247,
                    from include/linux/build_bug.h:5:
>> include/linux/stddef.h:16:33: error: invalid use of undefined type 'struct nvec_led'
      16 | #define offsetof(TYPE, MEMBER)  __builtin_offsetof(TYPE, MEMBER)
         |                                 ^~~~~~~~~~~~~~~~~~
   include/linux/container_of.h:23:28: note: in expansion of macro 'offsetof'
      23 |         ((type *)(__mptr - offsetof(type, member))); })
         |                            ^~~~~~~~
   drivers/staging/nvec/nvec_paz00.c:19:16: note: in expansion of macro 'container_of'
      19 |         return container_of(led_cdev, struct nvec_led, cdev);
         |                ^~~~~~~~~~~~
   drivers/staging/nvec/nvec_paz00.c:20:1: error: control reaches end of non-void function [-Werror=return-type]
      20 | }
         | ^
   cc1: some warnings being treated as errors


vim +20 include/linux/container_of.h

d2a8ebbf8192b8 Andy Shevchenko  2021-11-08   9  
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  10  /**
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  11   * container_of - cast a member of a structure out to the containing structure
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  12   * @ptr:	the pointer to the member.
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  13   * @type:	the type of the container struct this is embedded in.
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  14   * @member:	the name of the member within the struct.
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  15   *
7376e561fd2e01 Sakari Ailus     2022-10-24  16   * WARNING: any const qualifier of @ptr is lost.
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  17   */
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  18  #define container_of(ptr, type, member) ({				\
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  19  	void *__mptr = (void *)(ptr);					\
e1edc277e6f6df Rasmus Villemoes 2021-11-08 @20  	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
e1edc277e6f6df Rasmus Villemoes 2021-11-08  21  		      __same_type(*(ptr), void),			\
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  22  		      "pointer type mismatch in container_of()");	\
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  23  	((type *)(__mptr - offsetof(type, member))); })
d2a8ebbf8192b8 Andy Shevchenko  2021-11-08  24  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by kernel test robot 1 year ago
Hi Sumitra,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Sumitra-Sharma/Staging-nvec-Change-container_of-macro-to-an-inline-function/20230319-010628
patch link:    https://lore.kernel.org/r/20230318170514.GA49181%40sumitra.com
patch subject: [PATCH] Staging: nvec: Change container_of macro to an inline function.
config: arm64-randconfig-r014-20230319 (https://download.01.org/0day-ci/archive/20230319/202303190233.v2KJFmIG-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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 arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/e58807d27f0ba705144bce72751f5cb0a75b9682
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sumitra-Sharma/Staging-nvec-Change-container_of-macro-to-an-inline-function/20230319-010628
        git checkout e58807d27f0ba705144bce72751f5cb0a75b9682
        # 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=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/staging/nvec/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303190233.v2KJFmIG-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/staging/nvec/nvec_paz00.c:19:9: error: incomplete definition of type 'struct nvec_led'
           return container_of(led_cdev, struct nvec_led, cdev);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/container_of.h:20:47: note: expanded from macro 'container_of'
           static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
   include/linux/compiler_types.h:338:74: note: expanded from macro '__same_type'
   #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
                                                                            ^
   include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
   #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
                                    ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
   #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
                                                          ^~~~
   drivers/staging/nvec/nvec_paz00.c:17:22: note: forward declaration of 'struct nvec_led'
   static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
                        ^
>> drivers/staging/nvec/nvec_paz00.c:19:9: error: offsetof of incomplete type 'struct nvec_led'
           return container_of(led_cdev, struct nvec_led, cdev);
                  ^                      ~~~~~~
   include/linux/container_of.h:23:21: note: expanded from macro 'container_of'
           ((type *)(__mptr - offsetof(type, member))); })
                              ^        ~~~~
   include/linux/stddef.h:16:32: note: expanded from macro 'offsetof'
   #define offsetof(TYPE, MEMBER)  __builtin_offsetof(TYPE, MEMBER)
                                   ^                  ~~~~
   drivers/staging/nvec/nvec_paz00.c:17:22: note: forward declaration of 'struct nvec_led'
   static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
                        ^
>> drivers/staging/nvec/nvec_paz00.c:19:9: error: returning 'void' from a function with incompatible result type 'struct nvec_led *'
           return container_of(led_cdev, struct nvec_led, cdev);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/container_of.h:18:41: note: expanded from macro 'container_of'
   #define container_of(ptr, type, member) ({                              \
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   3 errors generated.


vim +19 drivers/staging/nvec/nvec_paz00.c

    16	
    17	static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
    18	{
  > 19		return container_of(led_cdev, struct nvec_led, cdev);
    20	}
    21	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by Julia Lawall 1 year ago

On Sat, 18 Mar 2023, Sumitra Sharma wrote:

> The macro has the drawback that one cannot determine
> what type it applies to by looking at the definition.
> Hence this macro definition is not type-safe.
>
> The inline function gives the same benefits as the
> macro and only accepts the specific type of arguments.
> Use static because the definition only requires it to be
> visible in the current file.

Sumitra,

The subject line and log message could be a little less generic.  For the
subject line, one has the impression that you are changing the definition
of container_of itself.

The log message is also a bit wordy.  Something like the following would
be more concise and still present the issue:

Convert to_nvec_led from a macro to an inline function, to make the
relevant types apparent in the definition and to benefit from the type
checking performed by the compiler at call sites.

julia


>
> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
>  drivers/staging/nvec/nvec_paz00.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec_paz00.c b/drivers/staging/nvec/nvec_paz00.c
> index 8b4da95081c8..9573ba762cdd 100644
> --- a/drivers/staging/nvec/nvec_paz00.c
> +++ b/drivers/staging/nvec/nvec_paz00.c
> @@ -14,8 +14,10 @@
>  #include <linux/platform_device.h>
>  #include "nvec.h"
>
> -#define to_nvec_led(led_cdev) \
> -	container_of(led_cdev, struct nvec_led, cdev)
> +static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
> +{
> +	return container_of(led_cdev, struct nvec_led, cdev);
> +}
>
>  #define NVEC_LED_REQ {'\x0d', '\x10', '\x45', '\x10', '\x00'}
>
> --
> 2.25.1
>
>
>
Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by Sumitra Sharma 1 year ago
On Sat, Mar 18, 2023 at 06:14:50PM +0100, Julia Lawall wrote:
> 
> 
> On Sat, 18 Mar 2023, Sumitra Sharma wrote:
> 
> > The macro has the drawback that one cannot determine
> > what type it applies to by looking at the definition.
> > Hence this macro definition is not type-safe.
> >
> > The inline function gives the same benefits as the
> > macro and only accepts the specific type of arguments.
> > Use static because the definition only requires it to be
> > visible in the current file.
> 
> Sumitra,
> 
> The subject line and log message could be a little less generic.  For the
> subject line, one has the impression that you are changing the definition
> of container_of itself.
> 
> The log message is also a bit wordy.  Something like the following would
> be more concise and still present the issue:
>

Okay. I will focus more on writing better patch subject and description.

Thanks.

Regards,

Sumitra

> Convert to_nvec_led from a macro to an inline function, to make the
> relevant types apparent in the definition and to benefit from the type
> checking performed by the compiler at call sites.
> 
> julia
> 
> 
> >
> > Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> > ---
> >  drivers/staging/nvec/nvec_paz00.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/nvec/nvec_paz00.c b/drivers/staging/nvec/nvec_paz00.c
> > index 8b4da95081c8..9573ba762cdd 100644
> > --- a/drivers/staging/nvec/nvec_paz00.c
> > +++ b/drivers/staging/nvec/nvec_paz00.c
> > @@ -14,8 +14,10 @@
> >  #include <linux/platform_device.h>
> >  #include "nvec.h"
> >
> > -#define to_nvec_led(led_cdev) \
> > -	container_of(led_cdev, struct nvec_led, cdev)
> > +static inline struct nvec_led *to_nvec_led(struct led_classdev *led_cdev)
> > +{
> > +	return container_of(led_cdev, struct nvec_led, cdev);
> > +}
> >
> >  #define NVEC_LED_REQ {'\x0d', '\x10', '\x45', '\x10', '\x00'}
> >
> > --
> > 2.25.1
> >
> >
> >
Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
Posted by Ira Weiny 1 year ago
Sumitra Sharma wrote:
> On Sat, Mar 18, 2023 at 06:14:50PM +0100, Julia Lawall wrote:
> > 
> > 
> > On Sat, 18 Mar 2023, Sumitra Sharma wrote:
> > 
> > > The macro has the drawback that one cannot determine
> > > what type it applies to by looking at the definition.
> > > Hence this macro definition is not type-safe.
> > >
> > > The inline function gives the same benefits as the
> > > macro and only accepts the specific type of arguments.
> > > Use static because the definition only requires it to be
> > > visible in the current file.
> > 
> > Sumitra,
> > 
> > The subject line and log message could be a little less generic.  For the
> > subject line, one has the impression that you are changing the definition
> > of container_of itself.
> > 
> > The log message is also a bit wordy.  Something like the following would
> > be more concise and still present the issue:
> >
> 
> Okay. I will focus more on writing better patch subject and description.

Sumitra,

I can't tell for sure via email if you are getting discouraged.  But if you are
don't feel bad.  Writing good commit messages is hard.

That said, I see a couple of build errors from 0day on this patch.  Do use the
tools to correct things like that before submitting.

Ira