Message-ID: <1AE640813FDE7649BE1B193DEA596E880248D7E9@SHSMSX101.ccr.corp.intel.com>
Date:	Tue, 26 Nov 2013 00:29:14 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Lv Zheng <zetalog@...il.com>
CC:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Brown, Len" <len.brown@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: RE: [PATCH 2/4] ACPI: Cleanup <acpi/acpi_bus.h> and
 <acpi/acpi_drivers.h> inclusions.

Hi, Rafael

Thanks for commenting.

> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Tuesday, November 26, 2013 8:06 AM
> 
> On Saturday, November 23, 2013 07:29:08 AM Lv Zheng wrote:
> > From: Lv Zheng <lv.zheng@...el.com>
> >
> > This patch enfoces <linux/acpi.h> inclusion instead of direct
> > <acpi/acpi_drivers.h> and <acpi/acpi_bus.h> inclusions.
> 
> First off, please make the changelog shorter.  Here's my version:
> 
> "Replace direct inclusions of <acpi/acpi_drivers.h> and <acpi/acpi_bus.h>,
> which are incorrect, with <linux/acpi.h> inclusions.
> 
> First of all, <acpi/acpi_drivers.h> and <acpi/acpi_bus.h> should not
> be included directly from any files that are built for CONFIG_ACPI
> unset, because that generally leads to build warnings about undefined
> symbols in !CONFIG_ACPI builds.  For CONFIG_ACPI set <linux/acpi.h>
> includes those files and for CONFIG_ACPI it provides stub ACPI
> symbols to be used in that case.
> 
> Second, there are ordering dependencies between those files that
> always have to be met.  Namely, it is required that <acpi/acpi_bus.h>
> be included prior to <acpi/acpi_drivers.h> so that the acpi_pci_root
> declarations the latter depends on are always there.  That also is
> taken care of including <linux/acpi.h> as appropriate."

OK.

> 
> Second, please fold [3/4] into this one.  Keeping them separate is
> artificial and pointless in my opinion.  [Of course, the changelog will
> have to be modified then.]

OK.

> 
> [...]
> 
> Apart from the above ->
> 
> > Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> > ---
[...]

> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index ba5b56d..1bd3de4 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -40,8 +40,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/slab.h>
> >  #include <asm/io.h>
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> 
> -> add the new #include above the <asm/io.h> maybe?

The patch was generated for people who even don't have too many ACPI header knowledge can review and determine that the modifications will not trigger regressions.
So it doesn't cleanup other existing inclusion issues for the affected files, for example:
1. empty lines.
2. asm files
3. other acpi inclusions

Do you mean I need to cleanup whole file other than what the patch description claimed to clean?
If so, I'll follow this comment and all the following ones to refresh the patch.

> 
> >  #include <linux/dmi.h>
> >
> >  #include "internal.h"

[...]

> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 54a20ff..8507034 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -50,7 +50,6 @@
> >  #include <asm/uaccess.h>
> >
> >  #include <acpi/acpi.h>
> > -#include <acpi/acpi_bus.h>
> >  #include <acpi/processor.h>
> 
> -> I guess all of these <acpi/...> inclusions may go away from here?
> 
> >  #include "internal.h"
> >

[...]

> > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > index 2652a61..cc5feb9 100644
> > --- a/drivers/acpi/pci_link.c
> > +++ b/drivers/acpi/pci_link.c
> > @@ -40,8 +40,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >
> 
> -> Remove the empty line too.
> 
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> >
> >  #define PREFIX "ACPI: "
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 20360e4..dadd0f50 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -35,8 +35,6 @@
> >  #include <linux/pci-aspm.h>
> >  #include <linux/acpi.h>
> >  #include <linux/slab.h>
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> >  #include <acpi/apei.h>
> 
> Does <acpi/apei.h> have to be included directly here?  If not, please remove
> it too.
> 
> >
> >  #include "internal.h"

[...]

> > diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
> > index 6a5b152..56d30fe 100644
> > --- a/drivers/acpi/proc.c
> > +++ b/drivers/acpi/proc.c
> > @@ -5,8 +5,7 @@
> >  #include <linux/bcd.h>
> >  #include <asm/uaccess.h>
> >
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> 
> -> Put the new line above the <asm/uaccess.h>
> 
> >
> >  #include "sleep.h"
> >
> > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> > index b3171f3..ad88410 100644
> > --- a/drivers/acpi/processor_core.c
> > +++ b/drivers/acpi/processor_core.c
> > @@ -11,7 +11,7 @@
> >  #include <linux/dmi.h>
> >  #include <linux/slab.h>
> >
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> >  #include <acpi/processor.h>
> 
> -> Remove the useless empty line and processor.h inclusion too.
> 
> >
> >  #include "internal.h"
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index 644516d..a1087d8 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -46,7 +46,6 @@
> >  #include <asm/apic.h>
> >  #endif
> >
> > -#include <acpi/acpi_bus.h>
> >  #include <acpi/processor.h>
> 
> Is the processor.h inclusion necessary here?
> 
> >
> >  #define PREFIX "ACPI: "
> > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
> > index 60a7c28..df63370 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -36,8 +36,7 @@
> >  #include <asm/cpufeature.h>
> >  #endif
> >
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> >  #include <acpi/processor.h>
> 
> And here?  And it's better to add #include <linux/acpi.h> to the other #include
> <linux/...> things.
> 
> >  #define PREFIX "ACPI: "
> > diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> > index d1d2e7f..4af89f9 100644
> > --- a/drivers/acpi/processor_thermal.c
> > +++ b/drivers/acpi/processor_thermal.c
> > @@ -33,9 +33,8 @@
> >
> >  #include <asm/uaccess.h>
> >
> > -#include <acpi/acpi_bus.h>
> > +#include <linux/acpi.h>
> >  #include <acpi/processor.h>
> > -#include <acpi/acpi_drivers.h>
> 
> Same comments as above.
> 
> >  #define PREFIX "ACPI: "
> >
> > diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
> > index e7dd2c1..a9de919 100644
> > --- a/drivers/acpi/processor_throttling.c
> > +++ b/drivers/acpi/processor_throttling.c
> > @@ -36,8 +36,7 @@
> >  #include <asm/io.h>
> >  #include <asm/uaccess.h>
> >
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> > +#include <linux/acpi.h>
> >  #include <acpi/processor.h>
> 
> Same comments as above.
> 
> >  #define PREFIX "ACPI: "

[...]

> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index fd39459..8870277 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -12,7 +12,6 @@
> >  #include <linux/dmi.h>
> >  #include <linux/nls.h>
> >
> > -#include <acpi/acpi_drivers.h>
> >
> 
> Remove the empty line too.
> 
> >  #include "internal.h"
> >
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 721e949..281e5fd 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -21,8 +21,6 @@
> >
> >  #include <asm/io.h>
> >
> > -#include <acpi/acpi_bus.h>
> > -#include <acpi/acpi_drivers.h>
> >
> 
> Remove the empty line too.
> 
> OK, you should get the idea. Please keep things consistent and nice. :-)

Yes, I'll try to cover all above comments in 1 patch.
Thanks for the helping.

Best regards
-Lv

Powered by blists - more mailing lists