| Message ID | 20230318170514.GA49181@sumitra.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Staging: nvec: Change container_of macro to an inline function. | expand |
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 > > >
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 > > > > > >
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
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
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
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'}
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(-)