| Message ID | 1495885877-7906-1-git-send-email-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <pali.rohar@gmail.com> 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. > - 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. > + 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.
On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote: > On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <pali.rohar@gmail.com> > 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. > > > > - 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. > > + 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.
On Saturday 27 May 2017 15:17:29 Pali Rohár wrote: > On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote: > > On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <pali.rohar@gmail.com> > > > > 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. > > > > > > - 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. > > > > + 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. 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).
On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote: >> On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <pali.rohar@gmail.com> >> wrote: >> > Remove >> > also reserved member as it does not have any defined meaning or >> > type yet. >> > - 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. >> > + 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.
On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: > On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> > wrote: > > On Saturday 27 May 2017 15:07:09 Andy Shevchenko wrote: > >> On Sat, May 27, 2017 at 2:51 PM, Pali Rohár <pali.rohar@gmail.com> > >> > >> wrote: > >> > Remove > >> > also reserved member as it does not have any defined meaning or > >> > type yet. > >> > > >> > - 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. > > >> > + 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. Both is OK for me. Do you want to send a new patch with %2pE?
On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: >> On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> >> 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. > > Both is OK for me. Do you want to send a new patch with %2pE? To me it looks slightly cleaner.
On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: > >> On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> > >> 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. > > > > 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?
On Thu, 2017-06-08 at 08:16 -0700, Darren Hart wrote: > On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: > > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: > > > > On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> > > > > 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. > > > > > > 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);
On Thu, Jun 8, 2017 at 6:38 PM, Joe Perches <joe@perches.com> wrote: > On Thu, 2017-06-08 at 08:16 -0700, Darren Hart wrote: >> On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: >> > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: >> > > > On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> >> > > > 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. >> > > >> > > 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.
On Thu, Jun 8, 2017 at 8:42 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jun 8, 2017 at 6:38 PM, Joe Perches <joe@perches.com> wrote: >> On Thu, 2017-06-08 at 08:16 -0700, Darren Hart wrote: >>> On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: >>> > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >>> > > 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?
On Thursday 08 June 2017 19:44:39 Andy Shevchenko wrote: > On Thu, Jun 8, 2017 at 8:42 PM, Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > On Thu, Jun 8, 2017 at 6:38 PM, Joe Perches <joe@perches.com> wrote: > >> On Thu, 2017-06-08 at 08:16 -0700, Darren Hart wrote: > >>> On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: > >>> > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár > >>> > <pali.rohar@gmail.com> wrote: > >>> > > 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? 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.
On Thursday 08 June 2017 08:16:18 Darren Hart wrote: > On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: > > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: > > >> On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> > > >> 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. > > > > > > 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.
On Fri, Jun 9, 2017 at 11:29 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Thursday 08 June 2017 08:16:18 Darren Hart wrote: >> On Sat, May 27, 2017 at 11:49:22PM +0300, Andy Shevchenko wrote: >> > On Sat, May 27, 2017 at 11:48 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > > On Saturday 27 May 2017 15:33:14 Andy Shevchenko wrote: >> > >> On Sat, May 27, 2017 at 4:17 PM, Pali Rohár <pali.rohar@gmail.com> >> > >> 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. >> > > >> > > 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
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index ceeb8c1..cd7045f 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -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) {
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 <pali.rohar@gmail.com> --- drivers/platform/x86/wmi.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)