* [PATCH] Staging: nvec: Change container_of macro to an inline function.
@ 2023-03-18 17:05 Sumitra Sharma
2023-03-18 17:14 ` Julia Lawall
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sumitra Sharma @ 2023-03-18 17:05 UTC (permalink / raw)
To: Marc Dietrich, Greg Kroah-Hartman, ac100, linux-tegra,
linux-staging, linux-kernel, outreachy
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
2023-03-18 17:05 [PATCH] Staging: nvec: Change container_of macro to an inline function Sumitra Sharma
@ 2023-03-18 17:14 ` Julia Lawall
2023-03-18 17:55 ` Sumitra Sharma
2023-03-18 18:55 ` kernel test robot
2023-03-18 20:48 ` kernel test robot
2 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2023-03-18 17:14 UTC (permalink / raw)
To: Sumitra Sharma
Cc: Marc Dietrich, Greg Kroah-Hartman, ac100, linux-tegra,
linux-staging, linux-kernel, outreachy
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
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
2023-03-18 17:14 ` Julia Lawall
@ 2023-03-18 17:55 ` Sumitra Sharma
2023-03-20 21:10 ` Ira Weiny
0 siblings, 1 reply; 6+ messages in thread
From: Sumitra Sharma @ 2023-03-18 17:55 UTC (permalink / raw)
To: Julia Lawall
Cc: Marc Dietrich, Greg Kroah-Hartman, ac100, linux-tegra,
linux-staging, linux-kernel, outreachy
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
> >
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
2023-03-18 17:05 [PATCH] Staging: nvec: Change container_of macro to an inline function Sumitra Sharma
2023-03-18 17:14 ` Julia Lawall
@ 2023-03-18 18:55 ` kernel test robot
2023-03-18 20:48 ` kernel test robot
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2023-03-18 18:55 UTC (permalink / raw)
To: Sumitra Sharma, Marc Dietrich, Greg Kroah-Hartman, ac100,
linux-tegra, linux-staging, linux-kernel, outreachy
Cc: llvm, oe-kbuild-all
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
2023-03-18 17:05 [PATCH] Staging: nvec: Change container_of macro to an inline function Sumitra Sharma
2023-03-18 17:14 ` Julia Lawall
2023-03-18 18:55 ` kernel test robot
@ 2023-03-18 20:48 ` kernel test robot
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2023-03-18 20:48 UTC (permalink / raw)
To: Sumitra Sharma, Marc Dietrich, Greg Kroah-Hartman, ac100,
linux-tegra, linux-staging, linux-kernel, outreachy
Cc: oe-kbuild-all
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: nvec: Change container_of macro to an inline function.
2023-03-18 17:55 ` Sumitra Sharma
@ 2023-03-20 21:10 ` Ira Weiny
0 siblings, 0 replies; 6+ messages in thread
From: Ira Weiny @ 2023-03-20 21:10 UTC (permalink / raw)
To: Sumitra Sharma, Julia Lawall
Cc: Marc Dietrich, Greg Kroah-Hartman, ac100, linux-tegra,
linux-staging, linux-kernel, outreachy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-20 21:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-18 17:05 [PATCH] Staging: nvec: Change container_of macro to an inline function Sumitra Sharma
2023-03-18 17:14 ` Julia Lawall
2023-03-18 17:55 ` Sumitra Sharma
2023-03-20 21:10 ` Ira Weiny
2023-03-18 18:55 ` kernel test robot
2023-03-18 20:48 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).