Re: [PATCH v2 2/4] kernel.h: Split out container_of() and typeof_member() macros

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Oct 07, 2021 at 12:51:27PM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt cleaning it up by splitting out container_of() and
> typeof_member() macros.
> 
> At the same time convert users in the header and other folders to use it.
> Though for time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
> 
> Note, there are _a lot_ of headers and modules that include kernel.h solely
> for one of these macros and this allows to unburden compiler for the twisted
> inclusion paths and to make new code cleaner in the future.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/kunit/test.h         |  2 ++
>  include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++
>  include/linux/kernel.h       | 31 +-----------------------------
>  include/linux/kobject.h      |  1 +
>  include/linux/list.h         |  6 ++++--
>  include/linux/llist.h        |  4 +++-
>  include/linux/plist.h        |  5 ++++-
>  include/media/media-entity.h |  3 ++-
>  lib/radix-tree.c             |  6 +++++-
>  lib/rhashtable.c             |  1 +
>  10 files changed, 60 insertions(+), 36 deletions(-)
>  create mode 100644 include/linux/container_of.h
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 24b40e5c160b..4d498f496790 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -11,6 +11,8 @@
>  
>  #include <kunit/assert.h>
>  #include <kunit/try-catch.h>
> +
> +#include <linux/container_of.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
> new file mode 100644
> index 000000000000..f6ee1be0e784
> --- /dev/null
> +++ b/include/linux/container_of.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_CONTAINER_OF_H
> +#define _LINUX_CONTAINER_OF_H
> +
> +#define typeof_member(T, m)	typeof(((T*)0)->m)
> +
> +/**
> + * container_of - cast a member of a structure out to the containing structure
> + * @ptr:	the pointer to the member.
> + * @type:	the type of the container struct this is embedded in.
> + * @member:	the name of the member within the struct.
> + *
> + */
> +#define container_of(ptr, type, member) ({				\
> +	void *__mptr = (void *)(ptr);					\
> +	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
> +			 !__same_type(*(ptr), void),			\
> +			 "pointer type mismatch in container_of()");	\
> +	((type *)(__mptr - offsetof(type, member))); })
> +
> +/**
> + * container_of_safe - cast a member of a structure out to the containing structure
> + * @ptr:	the pointer to the member.
> + * @type:	the type of the container struct this is embedded in.
> + * @member:	the name of the member within the struct.
> + *
> + * If IS_ERR_OR_NULL(ptr), ptr is returned unchanged.
> + */
> +#define container_of_safe(ptr, type, member) ({				\
> +	void *__mptr = (void *)(ptr);					\
> +	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
> +			 !__same_type(*(ptr), void),			\
> +			 "pointer type mismatch in container_of()");	\
> +	IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) :			\
> +		((type *)(__mptr - offsetof(type, member))); })
> +
> +#endif	/* _LINUX_CONTAINER_OF_H */
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d416fe3165cb..ad9fdcce9dcf 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -9,6 +9,7 @@
>  #include <linux/stddef.h>
>  #include <linux/types.h>
>  #include <linux/compiler.h>
> +#include <linux/container_of.h>
>  #include <linux/bitops.h>
>  #include <linux/kstrtox.h>
>  #include <linux/log2.h>
> @@ -482,36 +483,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  #define __CONCAT(a, b) a ## b
>  #define CONCATENATE(a, b) __CONCAT(a, b)
>  
> -/**
> - * container_of - cast a member of a structure out to the containing structure
> - * @ptr:	the pointer to the member.
> - * @type:	the type of the container struct this is embedded in.
> - * @member:	the name of the member within the struct.
> - *
> - */
> -#define container_of(ptr, type, member) ({				\
> -	void *__mptr = (void *)(ptr);					\
> -	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
> -			 !__same_type(*(ptr), void),			\
> -			 "pointer type mismatch in container_of()");	\
> -	((type *)(__mptr - offsetof(type, member))); })
> -
> -/**
> - * container_of_safe - cast a member of a structure out to the containing structure
> - * @ptr:	the pointer to the member.
> - * @type:	the type of the container struct this is embedded in.
> - * @member:	the name of the member within the struct.
> - *
> - * If IS_ERR_OR_NULL(ptr), ptr is returned unchanged.
> - */
> -#define container_of_safe(ptr, type, member) ({				\
> -	void *__mptr = (void *)(ptr);					\
> -	BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&	\
> -			 !__same_type(*(ptr), void),			\
> -			 "pointer type mismatch in container_of()");	\
> -	IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) :			\
> -		((type *)(__mptr - offsetof(type, member))); })
> -
>  /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>  # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index efd56f990a46..bf8371e58b17 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -15,6 +15,7 @@
>  #ifndef _KOBJECT_H_
>  #define _KOBJECT_H_
>  
> +#include <linux/container_of.h>
>  #include <linux/types.h>
>  #include <linux/list.h>
>  #include <linux/sysfs.h>
> diff --git a/include/linux/list.h b/include/linux/list.h
> index f2af4b4aa4e9..5dc679b373da 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -2,11 +2,13 @@
>  #ifndef _LINUX_LIST_H
>  #define _LINUX_LIST_H
>  
> +#include <linux/container_of.h>
> +#include <linux/const.h>
>  #include <linux/types.h>
>  #include <linux/stddef.h>
>  #include <linux/poison.h>
> -#include <linux/const.h>
> -#include <linux/kernel.h>
> +
> +#include <asm/barrier.h>
>  
>  /*
>   * Circular doubly linked list implementation.


This change looks odd.

You already have kernel.h including container_of.h, so why not have a
series that does:
	- create container_of.h and have kernel.h include it
	- multiple patches that remove kernel.h and use container_of.h
	  instead only.
	- multiple patches that remove kernel.h and use container_of.h
	  and other .h files (like list.h seems to need here.)
	- remove container_of.h from kernel.h

Mushing them all together here makes this really hard to understand why
this change is needed here.

thanks,

greg k-h