Skip to contentSkip to conversation

ACPICA / Interpreter: Remove redundant newline

PATCH In review Borislav Petkov 4 participants 19 messages Sep 9, 2016 – Sep 23, 2016

Subsystems (2)
ACPI ACPI component architecture (acpica)
lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

PATCH 1/1 ACPICA / Interpreter: Remove redundant newline

From: Borislav Petkov <[email protected]>

acpi_info() already issues a '\n' so remove it in the call.

Signed-off-by Borislav Petkov <[email protected]>
Cc Robert Moore <[email protected]>
Cc Lv Zheng <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Cc [email protected]
Cc [email protected]
---
 drivers/acpi/acpica/tbxfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/acpica/tbxfload.c +1 -1
index ac71abcd32bb..e7119b7ccd79 100644
@@ -240,7 +240,7 @@ acpi_status acpi_tb_load_namespace(void)
 	}
 
 	if (!tables_failed) {
-		ACPI_INFO(("%u ACPI AML tables successfully acquired and loaded\n", tables_loaded));
+		ACPI_INFO(("%u ACPI AML tables successfully acquired and loaded", tables_loaded));
 	} else {
 		ACPI_ERROR((AE_INFO,
 			    "%u table load failures, %u successful",
Signature · 1 line
2.10.0
#1 lore raw

Is this a big deal?

We do this on purpose for AcpiExec, to make the screen output more readable.

Quoted 43 lines

-----Original Message-----
From: Borislav Petkov [mailto:bp(a)alien8.de]
Sent: Friday, September 9, 2016 9:13 AM
To: LKML <linux-kernel(a)vger.kernel.org>
Cc: Moore, Robert <robert.moore(a)intel.com>; Zheng, Lv <lv.zheng(a)intel.com>; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; Len Brown <lenb(a)kernel.org>; linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: [PATCH] ACPICA / Interpreter: Remove redundant newline

From: Borislav Petkov <bp(a)suse.de>

acpi_info() already issues a '\n' so remove it in the call.

Signed-off-by Borislav Petkov <bp(a)suse.de>
Cc Robert Moore <robert.moore(a)intel.com>
Cc Lv Zheng <lv.zheng(a)intel.com>
Cc "Rafael J. Wysocki" <rafael.j.wysocki(a)intel.com>
Cc Len Brown <lenb(a)kernel.org>
Cc linux-acpi(a)vger.kernel.org
Cc devel(a)acpica.org
---
 drivers/acpi/acpica/tbxfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/acpi/acpica/tbxfload.c index ac71abcd32bb..e7119b7ccd79 100644

drivers/acpi/acpica/tbxfload.c +2 -2
@@ -240,7 +240,7 @@ acpi_status acpi_tb_load_namespace(void)
 	}
 
 	if (!tables_failed) {
-		ACPI_INFO(("%u ACPI AML tables successfully acquired and
 loaded\n", tables_loaded));
+		ACPI_INFO(("%u ACPI AML tables successfully acquired and
 loaded",
+tables_loaded));
 	} else {
 		ACPI_ERROR((AE_INFO,
 			    "%u table load failures, %u successful",
--

2.10.0

author
#2 lore raw
Quoted 3 lines · Robert

Is this a big deal?

We do this on purpose for AcpiExec, to make the screen output more readable.

Please do not top-post.

What do you mean "big deal"? All other ACPI_INFO calls don't have a "\n" at the end except this one. How does one "\n" make some output more readable?

(Btw, I have no idea what AcpiExec is. Grepping the kernel tree doesn't give any results).

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#3 lore raw
Quoted 2 lines

Please do not top-post.

You're not going to get a lot of help by scolding me. Bob

Quoted 29 lines

-----Original Message-----
From: Borislav Petkov [mailto:bp(a)alien8.de]
Sent: Friday, September 9, 2016 11:41 AM
To: Moore, Robert <robert.moore(a)intel.com>
Cc: LKML <linux-kernel(a)vger.kernel.org>; Zheng, Lv <lv.zheng(a)intel.com>; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; Len Brown <lenb(a)kernel.org>; linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [PATCH] ACPICA / Interpreter: Remove redundant newline

Quoted 3 lines · Robert

Is this a big deal?

We do this on purpose for AcpiExec, to make the screen output more

readable.

Please do not top-post.

What do you mean "big deal"? All other ACPI_INFO calls don't have a "\n" at the end except this one. How does one "\n" make some output more readable?

(Btw, I have no idea what AcpiExec is. Grepping the kernel tree doesn't give any results).

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

author
#4 lore raw
Quoted 5 lines · Robert
Quoted 2 lines

Please do not top-post.

You're not going to get a lot of help by scolding me.

How is "Please do not top-post" scolding you? The stress being on "Please".

How much more polite can I be?!?

Geez.

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#5 lore raw
In reply to Borislav Petkov
Quoted 3 lines · Borislav Petkov
Quoted 2 lines · Robert

Is this a big deal?
We do this on purpose for AcpiExec, to make the screen output more readable.

[]

Quoted 3 lines

What do you mean "big deal"? All other ACPI_INFO calls don't have a "\n" at the end except this one. How does one "\n" make some output more readable?

Blank lines in logging/dmesg generally don't add value.

I would prefer if the unnecessary double parentheses also were removed in these macro uses and ##__VA_ARGS__ was used instead.

/*
* Error reporting. Callers module and line number are inserted by AE_INFO,
* the plist contains a set of parens to allow variable-length lists.
* These macros are used for both the debug and non-debug versions of the code. */

#define ACPI_INFO(plist)                acpi_info plist
#define ACPI_WARNING(plist)             acpi_warning plist
#define ACPI_EXCEPTION(plist)           acpi_exception plist
#define ACPI_ERROR(plist)               acpi_error plist
#define ACPI_BIOS_WARNING(plist)        acpi_bios_warning plist
#define ACPI_BIOS_ERROR(plist)          acpi_bios_error plist

It would also be good if format/argument verification was done here and in the non-debug macro variants.

#define ACPI_INFO(plist)
#define ACPI_WARNING(plist)
#define ACPI_EXCEPTION(plist)
#define ACPI_ERROR(plist)
#define ACPI_BIOS_WARNING(plist)
#define ACPI_BIOS_ERROR(plist)

#6 lore raw

Hi,

Quoted 12 lines

From: Joe Perches [mailto:joe(a)perches.com]
Subject: Re: [PATCH] ACPICA / Interpreter: Remove redundant newline

Quoted 3 lines · Borislav Petkov
Quoted 2 lines · Robert

Is this a big deal?
We do this on purpose for AcpiExec, to make the screen output more readable.

[]

Quoted 3 lines

What do you mean "big deal"? All other ACPI_INFO calls don't have a "\n" at the end except this one. How does one "\n" make some output more readable?

Quoted 5 lines

Blank lines in logging/dmesg generally don't add value.

I would prefer if the unnecessary double parentheses also were removed in these macro uses and ##__VA_ARGS__ was used instead.

Ideally correct.
But __VA_ARGS__ is not portable.
And ACPICA is used in other environment.
I'd prefer to eliminate debugging/logging macros, but implement debugging/logging functions instead. As stdarg is more portable than __VA_ARGS__.

Thanks and best regards
Lv

Quoted 22 lines

/*
* Error reporting. Callers module and line number are inserted by AE_INFO,
* the plist contains a set of parens to allow variable-length lists.
* These macros are used for both the debug and non-debug versions of the code. */

#define ACPI_INFO(plist)                acpi_info plist
#define ACPI_WARNING(plist)             acpi_warning plist
#define ACPI_EXCEPTION(plist)           acpi_exception plist
#define ACPI_ERROR(plist)               acpi_error plist
#define ACPI_BIOS_WARNING(plist)        acpi_bios_warning plist
#define ACPI_BIOS_ERROR(plist)          acpi_bios_error plist

It would also be good if format/argument verification was done here and in the non-debug macro variants.

#define ACPI_INFO(plist)
#define ACPI_WARNING(plist)
#define ACPI_EXCEPTION(plist)
#define ACPI_ERROR(plist)
#define ACPI_BIOS_WARNING(plist)
#define ACPI_BIOS_ERROR(plist)

#7 lore raw
In reply to Moore, Robert

Well, I never thought I would write a couple lines of code like this, but here is a solution that should make everyone happy.

source/components/tables/tbxfload.c +6 -1
index 6a937b1..73ee1a2 100644
@@ -334,7 +334,7 @@ AcpiTbLoadNamespace (
     if (!TablesFailed)
     {
         ACPI_INFO ((
-            "%u ACPI AML tables successfully acquired and loaded\n",
+            "%u ACPI AML tables successfully acquired and loaded",
             TablesLoaded));
     }
     else

@@ -348,6 +348,11 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;
     }
 
+#ifdef ACPI_APPLICATION
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
+#endif
+
+
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);
author
#8 lore raw
Quoted 17 lines · Robert

Well, I never thought I would write a couple lines of code like this, but here is a solution that should make everyone happy.

source/components/tables/tbxfload.c +1 -1
index 6a937b1..73ee1a2 100644
@@ -334,7 +334,7 @@ AcpiTbLoadNamespace (
     if (!TablesFailed)
     {
         ACPI_INFO ((
-            "%u ACPI AML tables successfully acquired and loaded\n",
+            "%u ACPI AML tables successfully acquired and loaded",
             TablesLoaded));
     }
     else

@@ -348,6 +348,11 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;

source/components/tables/tbxfload.c AcpiTbLoadNamespace ( · line 358

Quoted 10 lines
source/components/tables/tbxfload.c +5 -0
     }
 
+#ifdef ACPI_APPLICATION
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
+#endif
+
+
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);

Works for me. A lot of kernel code does stuff like that already:

$ git grep -E "\(\"\\\n\"\)" | wc -l
1550
Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#9 lore raw
Quoted 3 lines · Borislav Petkov

Works for me. A lot of kernel code does stuff like that already:

$ git grep -E "\(\"\\\n\"\)" | wc -l
1550

<shrug> If you want.

and fyi:

Most of those are pr_cont uses for multiple printks to a single output line terminations.

author
#10 lore raw
Quoted 2 lines · Joe Perches

Most of those are pr_cont uses for multiple printks to a single output line terminations.

Not really:

$ git grep -E "\(\"\\\n\"\)" | grep -i cont | wc -l
185

Regardless, it doesn't really matter what those are.

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#11 lore raw
In reply to Moore, Robert

Hi,

Quoted 22 lines

From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-owner(a)vger.kernel.org] On Behalf Of Moore, Robert
David E <david.e.box(a)intel.com>
Subject: RE: [PATCH] ACPICA / Interpreter: Remove redundant newline

Well, I never thought I would write a couple lines of code like this, but here is a solution that should make everyone happy.

source/components/tables/tbxfload.c +1 -1
index 6a937b1..73ee1a2 100644
@@ -334,7 +334,7 @@ AcpiTbLoadNamespace (
     if (!TablesFailed)
     {
         ACPI_INFO ((
-            "%u ACPI AML tables successfully acquired and loaded\n",
+            "%u ACPI AML tables successfully acquired and loaded",
             TablesLoaded));
     }
     else

source/components/tables/tbxfload.c AcpiTbLoadNamespace ( · line 355

Quoted 8 lines
source/components/tables/tbxfload.c +5 -0
@@ -348,6 +348,11 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;
     }
 
+#ifdef ACPI_APPLICATION
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
+#endif
+
+

IMO, these lines should be in ACPICA upstream, in a file under tools/acpiexec.

Thanks
Lv

Quoted 8 lines
source/components/tables/tbxfload.c +0 -1
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw
Quoted 40 lines

-----Original Message-----
From: Zheng, Lv
Sent: Tuesday, September 13, 2016 8:11 PM
To: Moore, Robert <robert.moore(a)intel.com>; Moore, Robert <robert.moore(a)intel.com>; Borislav Petkov <bp(a)alien8.de>; LKML <linux- kernel(a)vger.kernel.org>
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org; Wysocki, Rafael J <rafael.j.wysocki(a)intel.com>; Box, David E <david.e.box(a)intel.com>
Subject: RE: [PATCH] ACPICA / Interpreter: Remove redundant newline

Hi,

Quoted 26 lines

From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-owner(a)vger.kernel.org] On Behalf Of Moore, Robert David E <david.e.box(a)intel.com>
Subject: RE: [PATCH] ACPICA / Interpreter: Remove redundant newline

Well, I never thought I would write a couple lines of code like this, but here is a solution that should make everyone happy.

source/components/tables/tbxfload.c +1 -1
index 6a937b1..73ee1a2 100644
@@ -334,7 +334,7 @@ AcpiTbLoadNamespace (
     if (!TablesFailed)
     {
         ACPI_INFO ((
-            "%u ACPI AML tables successfully acquired and loaded\n",
+            "%u ACPI AML tables successfully acquired and loaded",
             TablesLoaded));
     }
     else

@@ -348,6 +348,11 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;
     }
Quoted 5 lines

source/components/tables/tbxfload.c AcpiTbLoadNamespace ( · line 355

Quoted 4 lines
source/components/tables/tbxfload.c +4 -0
+#ifdef ACPI_APPLICATION
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n")); #endif
+
+

[Moore, Robert]

I have no problem with this, I just want to get it over with. It is not a big deal, let's leave it at that and not make it a big issue.

Quoted 14 lines

IMO, these lines should be in ACPICA upstream, in a file under tools/acpiexec.

Thanks
Lv

Quoted 8 lines
source/components/tables/tbxfload.c +0 -1
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

#13 lore raw
In reply to Borislav Petkov

The newline is intentional for acpiexec.
So you should fix this issue in acpiexec, aka, in ACPICA upstream.

Thanks
Lv

Quoted 39 lines

From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-owner(a)vger.kernel.org] On Behalf Of Borislav Petkov
Subject: [PATCH] ACPICA / Interpreter: Remove redundant newline

From: Borislav Petkov <bp(a)suse.de>

acpi_info() already issues a '\n' so remove it in the call.

Signed-off-by Borislav Petkov <bp(a)suse.de>
Cc Robert Moore <robert.moore(a)intel.com>
Cc Lv Zheng <lv.zheng(a)intel.com>
Cc "Rafael J. Wysocki" <rafael.j.wysocki(a)intel.com>
Cc Len Brown <lenb(a)kernel.org>
Cc linux-acpi(a)vger.kernel.org
Cc devel(a)acpica.org
---
 drivers/acpi/acpica/tbxfload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/acpica/tbxfload.c +1 -2
index ac71abcd32bb..e7119b7ccd79 100644
@@ -240,7 +240,7 @@ acpi_status acpi_tb_load_namespace(void)
 	}
 
 	if (!tables_failed) {
-		ACPI_INFO(("%u ACPI AML tables successfully acquired and loaded\n", tables_loaded));
+		ACPI_INFO(("%u ACPI AML tables successfully acquired and loaded", tables_loaded));
 	} else {
 		ACPI_ERROR((AE_INFO,
 			    "%u table load failures, %u successful",
--

2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#14 lore raw
Quoted 2 lines · Lv

The newline is intentional for acpiexec.
So you should fix this issue in acpiexec, aka, in ACPICA upstream.

If you elaborate as to from where I get the sources and against which branch/version/...?, I can try to send you a patch.

Signature · 5 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
#15 lore raw

Hi,

Quoted 10 lines

From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-owner(a)vger.kernel.org] On Behalf Of Borislav Petkov
Subject: Re: [PATCH] ACPICA / Interpreter: Remove redundant newline

Quoted 2 lines · Lv

The newline is intentional for acpiexec.
So you should fix this issue in acpiexec, aka, in ACPICA upstream.

If you elaborate as to from where I get the sources and against which branch/version/...?, I can try to send you a patch.

You can obtain a local copy of acpica.git via:

$ git clone https://github.com/acpica/acpica

The fix patch should only be sent to devel(a)acpica.org. And you'll see it in Linux upstream after an acpica release cycle.

Regards
Lv

Quoted 11 lines

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#16 lore raw
Quoted 5 lines · Lv

You can obtain a local copy of acpica.git via:

$ git clone https://github.com/acpica/acpica

The fix patch should only be sent to [email protected]. And you'll see it in Linux upstream after an acpica release cycle.

No need, Robert did it already:

1d435008fd9e ("Update an info message during table load phase.")

Thanks.

Signature · 4 lines
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
#17 lore raw

PATCH 1/1 acpiexec: Move an acpiexec specific purposed new line to acpiexec specific file

Quoted 3 lines

No need, Robert did it already:

1d435008fd9e ("Update an info message during table load phase.")

Not exactly what I meant. I meant the following on top of the above mentioned commit. As the newline is an acpiexec specific logic, it should be kept in acpiexec, shouldn't be left polluting a common file. Lv Zheng.

Signed-off-by Lv Zheng <[email protected]>

To: Borislav Petkov <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Len Brown <[email protected]>
Cc: <[email protected]>
--

source/components/tables/tbxfload.c +0 -5
Index: acpica/source/components/tables/tbxfload.c
===================================================================
@@ -348,11 +348,6 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;
     }
 
-#ifdef ACPI_APPLICATION
-    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
-#endif
-
-
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);
source/tools/acpiexec/aetables.c +1 -0
Index: acpica/source/tools/acpiexec/aetables.c
===================================================================
@@ -625,6 +625,7 @@ AeLoadTables (
 
     Status = AcpiLoadTables ();
     ACPI_CHECK_OK (AcpiLoadTables, Status);
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
 
     /*
      * Test run-time control method installation. Do it twice to test code
#18 lore raw

PATCH 1/1 acpiexec: Move an acpiexec specific purposed new line to acpiexec specific file

In reply to Borislav Petkov
Quoted 3 lines

No need, Robert did it already:

1d435008fd9e ("Update an info message during table load phase.")

Not exactly what I meant. I meant the following on top of the above mentioned commit. As the newline is an acpiexec specific logic, it should be kept in acpiexec, shouldn't be left polluting a common file. Lv Zheng.

Signed-off-by: Lv Zheng <lv.zheng(a)intel.com>
--

source/components/tables/tbxfload.c +0 -5
Index: acpica/source/components/tables/tbxfload.c
===================================================================
@@ -348,11 +348,6 @@ AcpiTbLoadNamespace (
         Status = AE_CTRL_TERMINATE;
     }
 
-#ifdef ACPI_APPLICATION
-    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
-#endif
-
-
 UnlockAndExit:
     (void) AcpiUtReleaseMutex (ACPI_MTX_TABLES);
     return_ACPI_STATUS (Status);
source/tools/acpiexec/aetables.c +1 -0
Index: acpica/source/tools/acpiexec/aetables.c
===================================================================
@@ -625,6 +625,7 @@ AeLoadTables (
 
     Status = AcpiLoadTables ();
     ACPI_CHECK_OK (AcpiLoadTables, Status);
+    ACPI_DEBUG_PRINT_RAW ((ACPI_DB_INIT, "\n"));
 
     /*
      * Test run-time control method installation. Do it twice to test code