2021-10-07 16:17:56

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Thu, 7 Oct 2021 18:44:04 +0300
Andy Shevchenko <[email protected]> wrote:

> When kernel.h is used in the headers it adds a lot into dependency hell,
> especially when there are circular dependencies are involved.
>
> Replace kernel.h inclusion with the list of what is really being used.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> include/linux/list.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> 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>

Is there a reason you didn't quite sort this into alphabetical order?

> -#include <linux/const.h>
> -#include <linux/kernel.h>
> +
> +#include <asm/barrier.h>
>
> /*
> * Circular doubly linked list implementation.


2021-10-07 16:23:25

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> On Thu, 7 Oct 2021 18:44:04 +0300
> Andy Shevchenko <[email protected]> wrote:

...

> Is there a reason you didn't quite sort this into alphabetical order?

Glad you asked! Yes. Greg and possibly others will come and tell me that
I mustn't do two things in one change.

--
With Best Regards,
Andy Shevchenko


2021-10-07 16:28:58

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Thu, Oct 07, 2021 at 07:21:18PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > On Thu, 7 Oct 2021 18:44:04 +0300
> > Andy Shevchenko <[email protected]> wrote:
>
> ...
>
> > Is there a reason you didn't quite sort this into alphabetical order?
>
> Glad you asked! Yes. Greg and possibly others will come and tell me that
> I mustn't do two things in one change.

Actually I have to avoid moving const.h. I will change this in v5.

--
With Best Regards,
Andy Shevchenko


2021-10-07 22:01:38

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> On Thu, 7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
>
> > When kernel.h is used in the headers it adds a lot into dependency hell,
> > especially when there are circular dependencies are involved.
> >
> > Replace kernel.h inclusion with the list of what is really being used.
> >
> > Signed-off-by: Andy Shevchenko <[email protected]>
> > ---
> > include/linux/list.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > 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>
>
> Is there a reason you didn't quite sort this into alphabetical order?

On a side note, if someone with perle knowledge could add a checkpatch
warning for this, I think it would be very nice. I'm a bit tired of
asking for alphabetical order in reviews :-)

> > -#include <linux/const.h>
> > -#include <linux/kernel.h>
> > +
> > +#include <asm/barrier.h>
> >
> > /*
> > * Circular doubly linked list implementation.

--
Regards,

Laurent Pinchart

2021-10-09 01:04:43

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Thu, 2021-10-07 at 20:29 +0300, Laurent Pinchart wrote:
> On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > On Thu, 7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
> > > When kernel.h is used in the headers it adds a lot into dependency hell,
> > > especially when there are circular dependencies are involved.
> > >
> > > Replace kernel.h inclusion with the list of what is really being used.
[]
> > > diff --git 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>
> >
> > Is there a reason you didn't quite sort this into alphabetical order?
>
> On a side note, if someone with perle knowledge could add a checkpatch
> warning for this, I think it would be very nice. I'm a bit tired of
> asking for alphabetical order in reviews :-)

As are people that want reverse christmas tree.
Neither of which I will do as I think both are poor form at best.

If you want, this was a checkpatch reverse christmas tree attempt,
as that was more common to some.

https://lore.kernel.org/lkml/[email protected]/


2021-10-09 01:23:57

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

On Fri, Oct 08, 2021 at 05:59:18PM -0700, Joe Perches wrote:
> On Thu, 2021-10-07 at 20:29 +0300, Laurent Pinchart wrote:
> > On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > > On Thu, 7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
> > > > When kernel.h is used in the headers it adds a lot into dependency hell,
> > > > especially when there are circular dependencies are involved.
> > > >
> > > > Replace kernel.h inclusion with the list of what is really being used.
> []
> > > > diff --git 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>
> > >
> > > Is there a reason you didn't quite sort this into alphabetical order?
> >
> > On a side note, if someone with perle knowledge could add a checkpatch
> > warning for this, I think it would be very nice. I'm a bit tired of
> > asking for alphabetical order in reviews :-)
>
> As are people that want reverse christmas tree.
> Neither of which I will do as I think both are poor form at best.

Reverse xmas tree order is just a matter of style, while alphabetical
ordering of headers helps catching duplicate, including when merging
branches that both add the same header in different locations. I thus
think there's a technical value to it.

> If you want, this was a checkpatch reverse christmas tree attempt,
> as that was more common to some.
>
> https://lore.kernel.org/lkml/[email protected]/

--
Regards,

Laurent Pinchart