lockdep: Move list.h inclusion into lockdep.h
diff mbox series

Message ID 20200624124212.GA17350@gondor.apana.org.au
State Accepted
Commit e885d5d94793ef342e49d55672baabbc16e32bb1
Headers show
Series
  • lockdep: Move list.h inclusion into lockdep.h
Related show

Commit Message

Herbert Xu June 24, 2020, 12:42 p.m. UTC
On Tue, Jun 23, 2020 at 04:28:58PM +0200, Petr Mladek wrote:
>
> My "allmodconfig" build has successfully finished with the following extra
>  fix on top of the two patches:
> 
> diff --git a/include/linux/list.h b/include/linux/list.h
> index aff44d34f4e4..6d606c4036ce 100644
> --- a/include/linux/list.h
> +++ b/include/linux/list.h
> @@ -6,7 +6,7 @@
>  #include <linux/stddef.h>
>  #include <linux/poison.h>
>  #include <linux/const.h>
> -#include <linux/kernel.h>
> +#include <linux/compiler.h>

Unfortunately this doesn't work because list.h actually does need
kernel.h for container_of.

However, we can easily fix the loop another way by removing list.h
from lockdep.h as it doesn't actually use any list macros/functions
but only the list type which is now in linux/types.h.

We could either fold this into the lockdep_types patch, or fold it
into the printk patch, or just leave it as a standalone patch.
What do you guys think?

---8<---
Currently lockdep_types.h includes list.h without actually using any
of its macros or functions.  All it needs are the type definitions
which were moved into types.h long ago.  This potentially causes
inclusion loops because both are included by many core header
files.

This patch moves the list.h inclusion into lockdep.h.  Note that
we could probably remove it completely but that could potentially
result in compile failures should any end users not include list.h
directly and also be unlucky enough to not get list.h via some other
header file.

Reported-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Andy Shevchenko June 24, 2020, 12:49 p.m. UTC | #1
On Wed, Jun 24, 2020 at 3:43 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jun 23, 2020 at 04:28:58PM +0200, Petr Mladek wrote:

...

> > -#include <linux/kernel.h>
> > +#include <linux/compiler.h>
>
> Unfortunately this doesn't work because list.h actually does need
> kernel.h for container_of.
>
> However, we can easily fix the loop another way by removing list.h
> from lockdep.h as it doesn't actually use any list macros/functions
> but only the list type which is now in linux/types.h.
>
> We could either fold this into the lockdep_types patch, or fold it
> into the printk patch, or just leave it as a standalone patch.
> What do you guys think?

Does lockdep_types include types? Then we are fine and it is the way to go.
Herbert Xu June 24, 2020, 12:50 p.m. UTC | #2
On Wed, Jun 24, 2020 at 03:49:23PM +0300, Andy Shevchenko wrote:
>
> Does lockdep_types include types? Then we are fine and it is the way to go.

Yes it already does.

Cheers,
Petr Mladek June 25, 2020, 10:11 a.m. UTC | #3
On Wed 2020-06-24 22:42:12, Herbert Xu wrote:
> On Tue, Jun 23, 2020 at 04:28:58PM +0200, Petr Mladek wrote:
> >
> > My "allmodconfig" build has successfully finished with the following extra
> >  fix on top of the two patches:
> > 
> > diff --git a/include/linux/list.h b/include/linux/list.h
> > index aff44d34f4e4..6d606c4036ce 100644
> > --- a/include/linux/list.h
> > +++ b/include/linux/list.h
> > @@ -6,7 +6,7 @@
> >  #include <linux/stddef.h>
> >  #include <linux/poison.h>
> >  #include <linux/const.h>
> > -#include <linux/kernel.h>
> > +#include <linux/compiler.h>
> 
> Unfortunately this doesn't work because list.h actually does need
> kernel.h for container_of.

Ah, I see.

> However, we can easily fix the loop another way by removing list.h
> from lockdep.h as it doesn't actually use any list macros/functions
> but only the list type which is now in linux/types.h.
> 
> We could either fold this into the lockdep_types patch, or fold it
> into the printk patch, or just leave it as a standalone patch.
> What do you guys think?

It logically belongs to the lockdep_types area.

I think that separate patch is the best solution so that Peter does
not need to rebase tip/locking/header.

> 
> ---8<---
> Currently lockdep_types.h includes list.h without actually using any
> of its macros or functions.  All it needs are the type definitions
> which were moved into types.h long ago.  This potentially causes
> inclusion loops because both are included by many core header
> files.
> 
> This patch moves the list.h inclusion into lockdep.h.  Note that
> we could probably remove it completely but that could potentially
> result in compile failures should any end users not include list.h
> directly and also be unlucky enough to not get list.h via some other
> header file.
> 
> Reported-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

It works with allmodconfig here, so feel free to use:

Tested-by: Petr Mladek <pmladek@suse.com>

Of course, it does not have much value. There might still be another
configuration or architecture that does not work but I would leave
this for test bots.

Best Regards,
Petr

> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 3b73cf84f77d..b1ad5c045353 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -21,6 +21,7 @@ extern int lock_stat;
>  #ifdef CONFIG_LOCKDEP
>  
>  #include <linux/linkage.h>
> +#include <linux/list.h>
>  #include <linux/debug_locks.h>
>  #include <linux/stacktrace.h>
>  
> diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
> index 7b9350624577..bb35b449f533 100644
> --- a/include/linux/lockdep_types.h
> +++ b/include/linux/lockdep_types.h
> @@ -32,8 +32,6 @@ enum lockdep_wait_type {
>  
>  #ifdef CONFIG_LOCKDEP
>  
> -#include <linux/list.h>
> -
>  /*
>   * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
>   * the total number of states... :-(
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu June 29, 2020, 7:32 a.m. UTC | #4
On Thu, Jun 25, 2020 at 12:11:19PM +0200, Petr Mladek wrote:
>
> It works with allmodconfig here, so feel free to use:
> 
> Tested-by: Petr Mladek <pmladek@suse.com>
> 
> Of course, it does not have much value. There might still be another
> configuration or architecture that does not work but I would leave
> this for test bots.

Thanks Petr!

Peter Z, could you please apply this patch on top of the existing
one in tip/locking/header?

Thanks,

Patch
diff mbox series

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 3b73cf84f77d..b1ad5c045353 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -21,6 +21,7 @@  extern int lock_stat;
 #ifdef CONFIG_LOCKDEP
 
 #include <linux/linkage.h>
+#include <linux/list.h>
 #include <linux/debug_locks.h>
 #include <linux/stacktrace.h>
 
diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
index 7b9350624577..bb35b449f533 100644
--- a/include/linux/lockdep_types.h
+++ b/include/linux/lockdep_types.h
@@ -32,8 +32,6 @@  enum lockdep_wait_type {
 
 #ifdef CONFIG_LOCKDEP
 
-#include <linux/list.h>
-
 /*
  * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
  * the total number of states... :-(