platform/x86: wmi: Fix printing info about WDG structure

PATCH Superseded Pali Rohár 4 participants 17 messages May 27, 2017 – Jun 13, 2017 x86 platform drivers

Share & tools

Pali Rohár

author

2017-05-27 11:51 UTC #0 lore raw

PATCH 1/1 platform/x86: wmi: Fix printing info about WDG structure

object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.

As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/platform/x86/wmi.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/platform/x86/wmi.c +4 -3

index ceeb8c1..cd7045f 100644
@@ -352,9 +352,10 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 static void wmi_dump_wdg(const struct guid_block *g)
 {
 	pr_info("%pUL:\n", g->guid);
-	pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);
-	pr_info("\tnotify_id: %02X\n", g->notify_id);
-	pr_info("\treserved: %02X\n", g->reserved);
+	if (g->flags & ACPI_WMI_EVENT)
+		pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+	else
+		pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);
 	pr_info("\tinstance_count: %d\n", g->instance_count);
 	pr_info("\tflags: %#x", g->flags);
 	if (g->flags) {

Signature · 1 line

1.7.9.5

Andy Shevchenko

2017-05-27 13:07 UTC #1 lore raw

drivers/platform/x86/wmi.c acpi_status wmi_set_block(const char *guid_string, u8 instance, · line 357

Quoted 12 lines · Pali Rohár

object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.
As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.
drivers/platform/x86/wmi.c +0 -3
-       pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);
-       pr_info("\tnotify_id: %02X\n", g->notify_id);
 
-       pr_info("\treserved: %02X\n", g->reserved);

Do we need this? Commit message doesn't clarify.

drivers/platform/x86/wmi.c acpi_status wmi_set_block(const char *guid_string, u8 instance, · line 358

Quoted 5 lines

drivers/platform/x86/wmi.c +4 -0

+       if (g->flags & ACPI_WMI_EVENT)
+               pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+       else
 
+               pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);

If this can still contain non-printable characters the %*pE can help instead.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pali Rohár

author

2017-05-27 13:17 UTC #2 lore raw

Quoted 12 lines · On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote:

Quoted 10 lines · Pali Rohár
object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.
As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.

Quoted 7 lines

Quoted 5 lines
- pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]); - pr_info("\tnotify_id: %02X\n",
g->notify_id);
- pr_info("\treserved: %02X\n", g->reserved);
Do we need this? Commit message doesn't clarify.

I wrote to commit message that reserved does not have defined meaning nor type. Also reserved overlap with object_id[1] so for non-event should not be print at all. And as it is reserved, I removed it.

Quoted 9 lines

Quoted 6 lines
drivers/platform/x86/wmi.c +4 -0
+       if (g->flags & ACPI_WMI_EVENT)
+               pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+       else
 
+               pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]);
If this can still contain non-printable characters the %*pE can help instead.

Those are printable ASCII. object_id contains two characters which are suffix for ACPI method.

Problem was only for events when we tried to print notify_id as object_id. notify_id is binary and overlaps with object_id.

Signature · 2 lines

Pali Rohár
[email protected]

Pali Rohár

author

2017-05-27 13:23 UTC #3 lore raw

Quoted 33 lines · On Saturday 27 May 2017 15:17:29 Pali Rohár wrote:

Quoted 13 lines · On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <[email protected]>
Quoted 10 lines · wrote:
object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.
As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.
Quoted 7 lines
Quoted 5 lines
- pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]); - pr_info("\tnotify_id: %02X\n",
g->notify_id);
- pr_info("\treserved: %02X\n", g->reserved);
Do we need this? Commit message doesn't clarify.
I wrote to commit message that reserved does not have defined meaning nor type. Also reserved overlap with object_id[1] so for non-event should not be print at all. And as it is reserved, I removed it.
Quoted 7 lines
Quoted 6 lines
drivers/platform/x86/wmi.c +4 -0
+       if (g->flags & ACPI_WMI_EVENT)
+               pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+       else
 
+               pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]);

Quoted 8 lines

Quoted 2 lines
If this can still contain non-printable characters the %*pE can help instead.
Those are printable ASCII. object_id contains two characters which are suffix for ACPI method.
Problem was only for events when we tried to print notify_id as object_id. notify_id is binary and overlaps with object_id.

Before patch modprobe wmi debug_dump_wdg=1 print to dmesg:

wmi: 8D9DDCBC-A997-11DA-B012-B622A1EF5492:
wmi: object_id: AA
wmi: notify_id: 41
wmi: reserved: 41
wmi: instance_count: 1
wmi: flags: 0x0
wmi: A80593CE-A997-11DA-B012-B622A1EF5492:
wmi: object_id: BA
wmi: notify_id: 42
wmi: reserved: 41
wmi: instance_count: 1
wmi: flags: 0x2 ACPI_WMI_METHOD
wmi: 9DBB5994-A997-11DA-B012-B622A1EF5492:
wmi: object_id: <<some_binary_char>>
wmi: notify_id: D0
wmi: reserved: 00
wmi: instance_count: 1
wmi: flags: 0x8 ACPI_WMI_EVENT
wmi: A3776CE0-1E88-11DB-A98B-0800200C9A66:
wmi: object_id: BC
wmi: notify_id: 42
wmi: reserved: 43
wmi: instance_count: 1
wmi: flags: 0x0
wmi: 05901221-D566-11D1-B2F0-00A0C9062910:
wmi: object_id: MO
wmi: notify_id: 4D
wmi: reserved: 4F
wmi: instance_count: 1
wmi: flags: 0x0

(where <<some_binary_char>> was 0xD0)

After patch it prints:

wmi: 8D9DDCBC-A997-11DA-B012-B622A1EF5492:
wmi: object_id: AA
wmi: instance_count: 1
wmi: flags: 0x0
wmi: A80593CE-A997-11DA-B012-B622A1EF5492:
wmi: object_id: BA
wmi: instance_count: 1
wmi: flags: 0x2 ACPI_WMI_METHOD
wmi: 9DBB5994-A997-11DA-B012-B622A1EF5492:
wmi: notify_id: 0xD0
wmi: instance_count: 1
wmi: flags: 0x8 ACPI_WMI_EVENT
wmi: A3776CE0-1E88-11DB-A98B-0800200C9A66:
wmi: object_id: BC
wmi: instance_count: 1
wmi: flags: 0x0
wmi: 05901221-D566-11D1-B2F0-00A0C9062910:
wmi: object_id: MO
wmi: instance_count: 1
wmi: flags: 0x0

I hope it is more clear right now.

Basically output now contains only meaningful parsed members (event contains notify_id, method contains object_id).

Signature · 2 lines

Pali Rohár
[email protected]

Andy Shevchenko

2017-05-27 13:33 UTC #4 lore raw

In reply to Pali Rohár

Quoted 8 lines · Pali Rohár

Quoted 6 lines · On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote:
Quoted 3 lines · Pali Rohár
Remove
also reserved member as it does not have any defined meaning or type yet.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
- pr_info("\treserved: %02X\n", g->reserved);
Do we need this? Commit message doesn't clarify.
I wrote to commit message that reserved does not have defined meaning nor type. Also reserved overlap with object_id[1] so for non-event should not be print at all. And as it is reserved, I removed it.

Oh, indeed.

Quoted 11 lines

Quoted 5 lines
Quoted 2 lines
+ pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]);
If this can still contain non-printable characters the %*pE can help instead.
Those are printable ASCII. object_id contains two characters which are suffix for ACPI method.
Problem was only for events when we tried to print notify_id as object_id. notify_id is binary and overlaps with object_id.

Okay, got it. But on your opinion does it make sense to do

pr_info("\tobject_id: %2pE\n", g->object_id);

instead?

For ASCII it will go as is previously, otherwise escaping would be done.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pali Rohár

author

2017-05-27 20:48 UTC #5 lore raw

Quoted 33 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:

Quoted 8 lines · Pali Rohár
Quoted 7 lines · On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <[email protected]>
Quoted 4 lines · wrote:
Remove
also reserved member as it does not have any defined meaning or type yet.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
- pr_info("\treserved: %02X\n", g->reserved);
Do we need this? Commit message doesn't clarify.
I wrote to commit message that reserved does not have defined meaning nor type. Also reserved overlap with object_id[1] so for non-event should not be print at all. And as it is reserved, I removed it.
Oh, indeed.
Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
+ pr_info("\tobject_id: %c%c\n", g->object_id[0],
g->object_id[1]);
If this can still contain non-printable characters the %*pE can help instead.
Those are printable ASCII. object_id contains two characters which are suffix for ACPI method.
Problem was only for events when we tried to print notify_id as object_id. notify_id is binary and overlaps with object_id.

Quoted 8 lines

Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?
For ASCII it will go as is previously, otherwise escaping would be done.

Both is OK for me. Do you want to send a new patch with %2pE?

Signature · 2 lines

Pali Rohár
[email protected]

Andy Shevchenko

2017-05-27 20:49 UTC #6 lore raw

Quoted 6 lines · Pali Rohár

Quoted 5 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do

Quoted 8 lines

Quoted 6 lines
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?
For ASCII it will go as is previously, otherwise escaping would be done.
Both is OK for me. Do you want to send a new patch with %2pE?

To me it looks slightly cleaner.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Darren Hart

2017-06-08 15:16 UTC #7 lore raw

Quoted 9 lines · Andy Shevchenko

Quoted 8 lines · Pali Rohár
Quoted 7 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
instead?
For ASCII it will go as is previously, otherwise escaping would be done.
Both is OK for me. Do you want to send a new patch with %2pE?
To me it looks slightly cleaner.

I don't want to let this one fall through the cracks. Pali, is a new one coming?

Signature · 2 lines

Darren Hart
VMware Open Source Technology Center

Joe Perches

2017-06-08 15:38 UTC #8 lore raw

Quoted 11 lines · Darren Hart

Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Pali Rohár
Quoted 8 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
For ASCII it will go as is previously, otherwise escaping would be done.
Both is OK for me. Do you want to send a new patch with %2pE?
To me it looks slightly cleaner.
I don't want to let this one fall through the cracks. Pali, is a new one coming?

Perhaps

	pr_info("\t%*pE\n", (int)sizeof(g->object_id), g->object_id);

Andy Shevchenko

2017-06-08 17:42 UTC #9 lore raw

Quoted 15 lines · Joe Perches

Quoted 14 lines · Darren Hart
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Pali Rohár
Quoted 10 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?
For ASCII it will go as is previously, otherwise escaping would be done.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Both is OK for me. Do you want to send a new patch with %2pE?
To me it looks slightly cleaner.
I don't want to let this one fall through the cracks. Pali, is a new one coming?
Perhaps
        pr_info("\t%*pE\n", (int)sizeof(g->object_id), g->object_id);

It will print some noisy characters as far as I understood the initial intention.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2017-06-08 17:44 UTC #10 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 7 lines · Joe Perches
Quoted 3 lines · Darren Hart
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Pali Rohár
On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
Perhaps
        pr_info("\t%*pE\n", (int)sizeof(g->object_id), g->object_id);
It will print some noisy characters as far as I understood the initial intention.

Ah, sorry, it will work, though sligtly differently. It will take extra argument from the stack which we can avoid. I don't expect the size of that growing. Pali?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pali Rohár

author

2017-06-08 18:18 UTC #11 lore raw

Quoted 14 lines · On Thursday 08 June 2017 19:44:39 Andy Shevchenko wrote:

On Thu, Jun 8, 2017 at 8:42 PM, Andy Shevchenko
Quoted 11 lines · <[email protected]> wrote:
Quoted 9 lines · Joe Perches
Quoted 4 lines · Darren Hart
Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Pali Rohár
On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
Perhaps
        pr_info("\t%*pE\n", (int)sizeof(g->object_id),
        g->object_id);

Quoted 6 lines

Quoted 2 lines
It will print some noisy characters as far as I understood the initial intention.
Ah, sorry, it will work, though sligtly differently. It will take extra argument from the stack which we can avoid. I don't expect the size of that growing. Pali?

ObjectID is always 2 bytes. And it should be printable ASCII (probably only [A-Za-z0-9_]) as it is used as ACPI method name.

Size is not going to change.

Signature · 2 lines

Pali Rohár
[email protected]

Pali Rohár

author

2017-06-09 08:29 UTC #12 lore raw

In reply to Darren Hart

Quoted 12 lines · On Thursday 08 June 2017 08:16:18 Darren Hart wrote:

Quoted 11 lines · Andy Shevchenko
Quoted 10 lines · Pali Rohár
Quoted 9 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
For ASCII it will go as is previously, otherwise escaping would be done.
Both is OK for me. Do you want to send a new patch with %2pE?
To me it looks slightly cleaner.
I don't want to let this one fall through the cracks. Pali, is a new one coming?

So.. which modifier to use? Andy already proposed two alternatives and basically I'm fine all 3 which are in this thread.

Signature · 2 lines

Pali Rohár
[email protected]

Andy Shevchenko

2017-06-09 09:29 UTC #13 lore raw

Quoted 16 lines · Pali Rohár

Quoted 15 lines · On Thursday 08 June 2017 08:16:18 Darren Hart wrote:
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Pali Rohár
Quoted 11 lines · On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote:
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <[email protected]> wrote:
Okay, got it. But on your opinion does it make sense to do
pr_info("\tobject_id: %2pE\n", g->object_id);
instead?
For ASCII it will go as is previously, otherwise escaping would be done.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Both is OK for me. Do you want to send a new patch with %2pE?
To me it looks slightly cleaner.
I don't want to let this one fall through the cracks. Pali, is a new one coming?
So.. which modifier to use? Andy already proposed two alternatives and basically I'm fine all 3 which are in this thread.

I would go with %2pE.
Rationale:
- we are not going to change size
- E is just a guard against _possible_ broken data

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pali Rohár

author

2017-06-10 10:57 UTC #14 lore raw

PATCH v2 1/1 platform/x86: wmi: Fix printing info about WDG structure

In reply to Pali Rohár

As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v2:
* Use %2pE instead of %c%c as suggested by Andy Shevchenko

---
 drivers/platform/x86/wmi.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/platform/x86/wmi.c +4 -3

index ceeb8c1..6d6f2a9 100644
@@ -352,9 +352,10 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 static void wmi_dump_wdg(const struct guid_block *g)
 {
 	pr_info("%pUL:\n", g->guid);
-	pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);
-	pr_info("\tnotify_id: %02X\n", g->notify_id);
-	pr_info("\treserved: %02X\n", g->reserved);
+	if (g->flags & ACPI_WMI_EVENT)
+		pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+	else
+		pr_info("\tobject_id: %2pE\n", g->object_id);
 	pr_info("\tinstance_count: %d\n", g->instance_count);
 	pr_info("\tflags: %#x", g->flags);
 	if (g->flags) {

Signature · 1 line

1.7.9.5

Andy Shevchenko

Reviewed-by on v2 1/1

2017-06-10 11:22 UTC #15 lore raw

Quoted 9 lines · Pali Rohár

object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.
As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.
Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 28 lines

---
Changes since v2:
* Use %2pE instead of %c%c as suggested by Andy Shevchenko

---
 drivers/platform/x86/wmi.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/platform/x86/wmi.c +4 -4

index ceeb8c1..6d6f2a9 100644
@@ -352,9 +352,10 @@ acpi_status wmi_set_block(const char *guid_string, u8 instance,
 static void wmi_dump_wdg(const struct guid_block *g)
 {
        pr_info("%pUL:\n", g->guid);
-       pr_info("\tobject_id: %c%c\n", g->object_id[0], g->object_id[1]);
-       pr_info("\tnotify_id: %02X\n", g->notify_id);
-       pr_info("\treserved: %02X\n", g->reserved);
+       if (g->flags & ACPI_WMI_EVENT)
+               pr_info("\tnotify_id: 0x%02X\n", g->notify_id);
+       else
+               pr_info("\tobject_id: %2pE\n", g->object_id);
        pr_info("\tinstance_count: %d\n", g->instance_count);
        pr_info("\tflags: %#x", g->flags);
        if (g->flags) {
--

1.7.9.5

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Darren Hart

2017-06-13 16:52 UTC #16 lore raw

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Pali Rohár
object_id and notify_id are in one union structure and their meaning is defined by flags. Therefore do not print notify_id for non-event block and do not print object_id for event block. Remove also reserved member as it does not have any defined meaning or type yet.

Quoted 6 lines

Quoted 4 lines
As object_id and notify_id union members overlaps and have different types, it caused that kernel print to dmesg binary data. This patch eliminates it.
Signed-off-by Pali Rohár <[email protected]>
Reviewed-by Andy Shevchenko <[email protected]>

Thanks Pali, nice fix. Queued to testing.

Signature · 2 lines

Darren Hart
VMware Open Source Technology Center