diff mbox

[v9,01/21] ACPI / table: Use pr_debug() instead of pr_info() for MADT table scanning

Message ID 1424853601-6675-2-git-send-email-hanjun.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo Feb. 25, 2015, 8:39 a.m. UTC
For a normal 8 cpu sockets system, it will up to 240 cpu threads (Xeon E7
v2 family for now), and we need 240 entries for local apic or local x2apic
in MADT table, so it will be much verbose information printed with a slow
uart console when system booted, this will be even worse with large system
with 16/32 cpu sockets.

This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
local apic/x2apic/sapic structures when scanning the MADT table to remove
those verbose information, but leave other structures unchanged.

CC: Rafael J Wysocki <rjw@rjwysocki.net>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/tables.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

Comments

Rafael J. Wysocki March 4, 2015, 10:33 p.m. UTC | #1
On Wednesday, February 25, 2015 04:39:41 PM Hanjun Guo wrote:
> For a normal 8 cpu sockets system, it will up to 240 cpu threads (Xeon E7
> v2 family for now), and we need 240 entries for local apic or local x2apic
> in MADT table, so it will be much verbose information printed with a slow
> uart console when system booted, this will be even worse with large system
> with 16/32 cpu sockets.
> 
> This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
> local apic/x2apic/sapic structures when scanning the MADT table to remove
> those verbose information, but leave other structures unchanged.
> 
> CC: Rafael J Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/tables.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 93b8152..f4e5b88 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -23,6 +23,8 @@
>   *
>   */
>  
> +/* Uncomment next line to get verbose printout */
> +/* #define DEBUG */
>  #define pr_fmt(fmt) "ACPI: " fmt
>  
>  #include <linux/init.h>
> @@ -61,9 +63,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_apic *p =
>  			    (struct acpi_madt_local_apic *)header;
> -			pr_info("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> -				p->processor_id, p->id,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> +				 p->processor_id, p->id,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>  		}
>  		break;
>  
> @@ -71,9 +73,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_x2apic *p =
>  			    (struct acpi_madt_local_x2apic *)header;
> -			pr_info("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
> -				p->local_apic_id, p->uid,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
> +				 p->local_apic_id, p->uid,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>  		}
>  		break;
>  
> @@ -81,8 +83,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_io_apic *p =
>  			    (struct acpi_madt_io_apic *)header;
> -			pr_info("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
> -				p->id, p->address, p->global_irq_base);
> +			pr_debug("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
> +				 p->id, p->address, p->global_irq_base);
>  		}
>  		break;
>  
> @@ -155,9 +157,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_io_sapic *p =
>  			    (struct acpi_madt_io_sapic *)header;
> -			pr_info("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
> -				p->id, (void *)(unsigned long)p->address,
> -				p->global_irq_base);
> +			pr_debug("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
> +				 p->id, (void *)(unsigned long)p->address,
> +				 p->global_irq_base);
>  		}
>  		break;
>  
> @@ -165,9 +167,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_sapic *p =
>  			    (struct acpi_madt_local_sapic *)header;
> -			pr_info("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
> -				p->processor_id, p->id, p->eid,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
> +				 p->processor_id, p->id, p->eid,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>  		}
>  		break;
>  
>
Olof Johansson March 5, 2015, 5:55 p.m. UTC | #2
On Wed, Feb 25, 2015 at 04:39:41PM +0800, Hanjun Guo wrote:
> For a normal 8 cpu sockets system, it will up to 240 cpu threads (Xeon E7
> v2 family for now), and we need 240 entries for local apic or local x2apic
> in MADT table, so it will be much verbose information printed with a slow
> uart console when system booted, this will be even worse with large system
> with 16/32 cpu sockets.
> 
> This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
> local apic/x2apic/sapic structures when scanning the MADT table to remove
> those verbose information, but leave other structures unchanged.
> 
> CC: Rafael J Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Acked-by: Olof Johansson <olof@lixom.net>

(This can surely go in now and not be pending on the rest of the patch set)


-Olof
Grant Likely March 6, 2015, 8:17 p.m. UTC | #3
On Wed, 25 Feb 2015 16:39:41 +0800
, Hanjun Guo <hanjun.guo@linaro.org>
 wrote:
> For a normal 8 cpu sockets system, it will up to 240 cpu threads (Xeon E7
> v2 family for now), and we need 240 entries for local apic or local x2apic
> in MADT table, so it will be much verbose information printed with a slow
> uart console when system booted, this will be even worse with large system
> with 16/32 cpu sockets.
> 
> This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
> local apic/x2apic/sapic structures when scanning the MADT table to remove
> those verbose information, but leave other structures unchanged.
> 
> CC: Rafael J Wysocki <rjw@rjwysocki.net>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

One nitpick below, but don't respin over this, and don't do a fixup.

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/acpi/tables.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 93b8152..f4e5b88 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -23,6 +23,8 @@
>   *
>   */
>  
> +/* Uncomment next line to get verbose printout */
> +/* #define DEBUG */
>  #define pr_fmt(fmt) "ACPI: " fmt
>  
>  #include <linux/init.h>
> @@ -61,9 +63,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_apic *p =
>  			    (struct acpi_madt_local_apic *)header;
> -			pr_info("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> -				p->processor_id, p->id,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> +				 p->processor_id, p->id,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");

The whitespace changes makes each 1 line change into 3 line changes. In
these situations, I would chose to leave the whitespace alone to keep
the diffstat as small as possible. It makes it less likely to conflict
with other patches and easier to find context.

>  		}
>  		break;
>  
> @@ -71,9 +73,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_x2apic *p =
>  			    (struct acpi_madt_local_x2apic *)header;
> -			pr_info("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
> -				p->local_apic_id, p->uid,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
> +				 p->local_apic_id, p->uid,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>  		}
>  		break;
>  
> @@ -81,8 +83,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_io_apic *p =
>  			    (struct acpi_madt_io_apic *)header;
> -			pr_info("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
> -				p->id, p->address, p->global_irq_base);
> +			pr_debug("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
> +				 p->id, p->address, p->global_irq_base);
>  		}
>  		break;
>  
> @@ -155,9 +157,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_io_sapic *p =
>  			    (struct acpi_madt_io_sapic *)header;
> -			pr_info("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
> -				p->id, (void *)(unsigned long)p->address,
> -				p->global_irq_base);
> +			pr_debug("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
> +				 p->id, (void *)(unsigned long)p->address,
> +				 p->global_irq_base);
>  		}
>  		break;
>  
> @@ -165,9 +167,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  		{
>  			struct acpi_madt_local_sapic *p =
>  			    (struct acpi_madt_local_sapic *)header;
> -			pr_info("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
> -				p->processor_id, p->id, p->eid,
> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> +			pr_debug("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
> +				 p->processor_id, p->id, p->eid,
> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>  		}
>  		break;
>  
> -- 
> 1.9.1
>
Joe Perches March 6, 2015, 8:31 p.m. UTC | #4
On Fri, 2015-03-06 at 20:17 +0000, Grant Likely wrote:
> On Wed, 25 Feb 2015 16:39:41 +0800 Hanjun Guo <hanjun.guo@linaro.org> wrote:
> > This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
> > local apic/x2apic/sapic structures when scanning the MADT table to remove
> > those verbose information, but leave other structures unchanged.
[]
> One nitpick below, but don't respin over this, and don't do a fixup.
[]
> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
[]
> > @@ -61,9 +63,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
> >  		{
> >  			struct acpi_madt_local_apic *p =
> >  			    (struct acpi_madt_local_apic *)header;
> > -			pr_info("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> > -				p->processor_id, p->id,
> > -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> > +			pr_debug("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
> > +				 p->processor_id, p->id,
> > +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
> 
> The whitespace changes makes each 1 line change into 3 line changes. In
> these situations, I would chose to leave the whitespace alone to keep
> the diffstat as small as possible. It makes it less likely to conflict
> with other patches and easier to find context.

I think it's mostly better to use a consistent indentation style
regardless of the number in whitespace changes surrounding the change.
Hanjun Guo March 10, 2015, 2:35 a.m. UTC | #5
On 2015?03?07? 04:31, Joe Perches wrote:
> On Fri, 2015-03-06 at 20:17 +0000, Grant Likely wrote:
>> On Wed, 25 Feb 2015 16:39:41 +0800 Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>> This patch just use pr_debug() instead of pr_info() for ioapic/iosapic,
>>> local apic/x2apic/sapic structures when scanning the MADT table to remove
>>> those verbose information, but leave other structures unchanged.
> []
>> One nitpick below, but don't respin over this, and don't do a fixup.
> []
>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> []
>>> @@ -61,9 +63,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>>>   		{
>>>   			struct acpi_madt_local_apic *p =
>>>   			    (struct acpi_madt_local_apic *)header;
>>> -			pr_info("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
>>> -				p->processor_id, p->id,
>>> -				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>>> +			pr_debug("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
>>> +				 p->processor_id, p->id,
>>> +				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
>>
>> The whitespace changes makes each 1 line change into 3 line changes. In
>> these situations, I would chose to leave the whitespace alone to keep
>> the diffstat as small as possible. It makes it less likely to conflict
>> with other patches and easier to find context.
>
> I think it's mostly better to use a consistent indentation style
> regardless of the number in whitespace changes surrounding the change.

I think both would be fine. Since this patch only touches acpi core
code and ACPI maintainer Rafael already acked it, I will keep it as
it is.

Thanks
Hanjun
diff mbox

Patch

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 93b8152..f4e5b88 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -23,6 +23,8 @@ 
  *
  */
 
+/* Uncomment next line to get verbose printout */
+/* #define DEBUG */
 #define pr_fmt(fmt) "ACPI: " fmt
 
 #include <linux/init.h>
@@ -61,9 +63,9 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_apic *p =
 			    (struct acpi_madt_local_apic *)header;
-			pr_info("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
-				p->processor_id, p->id,
-				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+			pr_debug("LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
+				 p->processor_id, p->id,
+				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;
 
@@ -71,9 +73,9 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_x2apic *p =
 			    (struct acpi_madt_local_x2apic *)header;
-			pr_info("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
-				p->local_apic_id, p->uid,
-				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+			pr_debug("X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
+				 p->local_apic_id, p->uid,
+				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;
 
@@ -81,8 +83,8 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_io_apic *p =
 			    (struct acpi_madt_io_apic *)header;
-			pr_info("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
-				p->id, p->address, p->global_irq_base);
+			pr_debug("IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
+				 p->id, p->address, p->global_irq_base);
 		}
 		break;
 
@@ -155,9 +157,9 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_io_sapic *p =
 			    (struct acpi_madt_io_sapic *)header;
-			pr_info("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
-				p->id, (void *)(unsigned long)p->address,
-				p->global_irq_base);
+			pr_debug("IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
+				 p->id, (void *)(unsigned long)p->address,
+				 p->global_irq_base);
 		}
 		break;
 
@@ -165,9 +167,9 @@  void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_sapic *p =
 			    (struct acpi_madt_local_sapic *)header;
-			pr_info("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
-				p->processor_id, p->id, p->eid,
-				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+			pr_debug("LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
+				 p->processor_id, p->id, p->eid,
+				 (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;