[PATCH] Fix ACPI halt for certain DSDTs

[PATCH] Fix ACPI halt for certain DSDTs

[Valentin Dornauer]

[-- Attachment #1.1: Type: text/plain, Size: 2648 bytes --]

Hello!

GRUB2 currently seems to be unable to shut down my Fujitsu Lifebook
E744 laptop using ACPI. After some debugging, I nailed the problem
down to two problems within GRUB’s ‘acpihalt’ command. Please see
the attached patch for my solution.


Without this patch I get the following:
  grub> halt
  Unknown opcode 0x6
  Unknown opcode 0x12
  ACPI shutdown failed
  [.. Laptop hangs indefinitely ..]

With debugging enabled:
  grub> set debug=acpi
  grub> halt
  commands/i386/pc/acpi.c:56: Looking for RSDP. Scanning EBDA
  commands/i386/pc/acpi.c:70: Looking for RSDP. Scanning BIOS
  [. . .]
  commands/acpihalt.c:195: Opcode 0x8
  commands/acpihalt.c:196: Tell 1c0d
  commands/acpihalt.c:105: data type = 0x11
  commands/acpihalt.c:195: Opcode 0x6
  commands/acpihalt.c:196: Tell 1c1c
  Unknown opcode 0x6
  commands/acpihalt.c:380: SSDT = 0xdcffd000
  commands/acpihalt.c:195: Opcode 0x10
  commands/acpihalt.c:196: Tell 24
  [. . .]
  commands/acpihalt.c:195: Opcode 0x8
  commands/acpihalt.c:196: Tell 98
  commands/acpihalt.c:105: data type = 0x12
  commands/acpihalt.c:195: Opcode 0x12
  commands/acpihalt.c:196: Tell 2b1
  Unknown opcode 0x12
  commands/acpihalt.c:380: SSDT = 0xdcfe6000
  commands/acpihalt.c:195: Opcode 0x10
  commands/acpihalt.c:196: Tell 24
  [. . .]
  commands/acpihalt.c:195: Opcode 0x14
  commands/acpihalt.c:196: Tell aa2
  commands/acpihalt.c:386: SLP_TYP = -2, port = 0x1804
  ACPI shutdown failed

With the attached patch, GRUB successfully finds SLP_TYP (7, for
the laptop model mentioned above) and turns off the laptop.


The patch actually fixes two problems: first, the laptop's SSDT
contains an AliasOp (which consists of two NameStrings), which was
unknown to GRUB. Simply skip them when they're encountered.

The second problem was more subtle: the function that parses extended
opcodes, skip_ext_op(), does not treat the OpRegionOp correctly.
According to the ACPI spec, the 3rd and 4th parameters are actually
TermArgs, which are more complex than what skip_data_ref_object()
is designed to handle. Here's the snipped from the decompiled AML
that triggered the problem:

  OperationRegion (MBAR, SystemMemory, Add (ShiftLeft (\_SB.PCI0.MHBR, 0x0F), 0x5000), 0x1000)
  Field (MBAR, ByteAcc, NoLock, Preserve)
  {
              Offset (0x938),
      PWRU,   4,
              Offset (0x9A0),
      PPL1,   15,
      PL1E,   1,
      CLP1,   1
  }


The whole ACPI thing still confuses me and I'm beginning to understand
why everyone seems to hate ACPI, so if I got anything wrong, please
feel free to correct me.

- Valentin

[-- Attachment #1.2: 0001-Fix-ACPI-halt-for-certain-DSDTs.patch --]
[-- Type: application/octet-stream, Size: 5609 bytes --]

From 01fedb06ff6ab3ce4c8655e988a0577b93bc6253 Mon Sep 17 00:00:00 2001
From: Valentin Dornauer <valentin@unimplemented.org>
Date: Tue, 1 Jul 2014 01:49:52 +0200
Subject: [PATCH] Fix ACPI halt for certain DSDTs

The AML parser implements only a small subset of possible AML
opcodes. On the Fujitsu Lifebook E744 this and another bug in
the parser (incorrect handling of TermArg data types) would lead
to the laptop not turning off (_S5 not found).

* grub-core/commands/acpihalt.c: Support OpAlias in the AML parser;
in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
simply strings!); add function to skip TermArgs.
* include/grub/acpi.h: Add new opcodes
---
 ChangeLog                     |   12 +++++++++
 grub-core/commands/acpihalt.c |   55 +++++++++++++++++++++++++++++++++++++++--
 include/grub/acpi.h           |   14 +++++++++++
 3 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5109c5a..5433b12 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2014-06-30  Valentin Dornauer  <valentin@unimplemented.org>
+
+	The AML parser implements only a small subset of possible AML
+	opcodes. On the Fujitsu Lifebook E744 this and another bug in
+	the parser (incorrect handling of TermArg data types) would lead
+	to the laptop not turning off (_S5 not found).
+
+	* grub-core/commands/acpihalt.c: Support OpAlias in the AML parser;
+	in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
+	simply strings!); add function to skip TermArgs.
+	* include/grub/acpi.h: Add new opcodes
+
 2014-06-26  Colin Watson  <cjwatson@ubuntu.com>
 
 	* docs/grub-dev.texi (Finding your way around): The build system no
diff --git a/grub-core/commands/acpihalt.c b/grub-core/commands/acpihalt.c
index 83bdfe1..beb1045 100644
--- a/grub-core/commands/acpihalt.c
+++ b/grub-core/commands/acpihalt.c
@@ -136,6 +136,45 @@ skip_data_ref_object (const grub_uint8_t *ptr, const grub_uint8_t *end)
 }
 
 static inline grub_uint32_t
+skip_term (const grub_uint8_t *ptr, const grub_uint8_t *end)
+{
+  grub_uint32_t add;
+  const grub_uint8_t *ptr0 = ptr;
+
+  switch(*ptr)
+  {
+    case GRUB_ACPI_OPCODE_ADD:
+    case GRUB_ACPI_OPCODE_CONCAT:
+    case GRUB_ACPI_OPCODE_SUBTRACT:
+    case GRUB_ACPI_OPCODE_MULTIPLY:
+    case GRUB_ACPI_OPCODE_LSHIFT:
+    case GRUB_ACPI_OPCODE_RSHIFT:
+    case GRUB_ACPI_OPCODE_AND:
+    case GRUB_ACPI_OPCODE_NAND:
+    case GRUB_ACPI_OPCODE_OR:
+    case GRUB_ACPI_OPCODE_NOR:
+    case GRUB_ACPI_OPCODE_XOR:
+    case GRUB_ACPI_OPCODE_CONCATRES:
+      /*
+       * Parameters for these opcodes: TermArg, TermArg Target, see ACPI
+       * spec r5.0, page 828f.
+       */
+      ptr++;
+      ptr += add = skip_term (ptr, end);
+      if (add == 0)
+        return 0;
+      ptr += add = skip_term (ptr, end);
+      if (add == 0)
+        return 0;
+      ptr += skip_name_string (ptr, end);
+      break;
+    default:
+      return skip_data_ref_object (ptr, end);
+  }
+  return ptr - ptr0;
+}
+
+static inline grub_uint32_t
 skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
 {
   const grub_uint8_t *ptr0 = ptr;
@@ -156,10 +195,10 @@ skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
       ptr++;
       ptr += skip_name_string (ptr, end);
       ptr++;
-      ptr += add = skip_data_ref_object (ptr, end);
+      ptr += add = skip_term (ptr, end);
       if (!add)
 	return 0;
-      ptr += add = skip_data_ref_object (ptr, end);
+      ptr += add = skip_term (ptr, end);
       if (!add)
 	return 0;
       break;
@@ -180,6 +219,7 @@ skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
   return ptr - ptr0;
 }
 
+
 static int
 get_sleep_type (grub_uint8_t *table, grub_uint8_t *ptr, grub_uint8_t *end,
 		grub_uint8_t *scope, int scope_len)
@@ -250,6 +290,17 @@ get_sleep_type (grub_uint8_t *table, grub_uint8_t *ptr, grub_uint8_t *end,
 	  if (!add)
 	    return -1;
 	  break;
+	case GRUB_ACPI_OPCODE_ALIAS:
+	  ptr++;
+	  /* We need to skip two name strings */
+	  ptr += add = skip_name_string (ptr, end);
+	  if (!add)
+	    return -1;
+	  ptr += add = skip_name_string (ptr, end);
+	  if (!add)
+	    return -1;
+	  break;
+
 	case GRUB_ACPI_OPCODE_SCOPE:
 	  {
 	    int scope_sleep_type;
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 2ac2bd6..7ec0c30 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -192,6 +192,7 @@ enum
   {
     GRUB_ACPI_OPCODE_ZERO = 0, GRUB_ACPI_OPCODE_ONE = 1,
     GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
+    GRUB_ACPI_OPCODE_ALIAS = 0x06,
     GRUB_ACPI_OPCODE_WORD_CONST = 0x0b,
     GRUB_ACPI_OPCODE_DWORD_CONST = 0x0c,
     GRUB_ACPI_OPCODE_STRING_CONST = 0x0d,
@@ -201,6 +202,19 @@ enum
     GRUB_ACPI_OPCODE_METHOD = 0x14, GRUB_ACPI_OPCODE_EXTOP = 0x5b,
     GRUB_ACPI_OPCODE_CREATE_WORD_FIELD = 0x8b,
     GRUB_ACPI_OPCODE_CREATE_BYTE_FIELD = 0x8c,
+    GRUB_ACPI_OPCODE_ADD = 0x72,
+    GRUB_ACPI_OPCODE_CONCAT = 0x73,
+    GRUB_ACPI_OPCODE_SUBTRACT = 0x74,
+    GRUB_ACPI_OPCODE_MULTIPLY = 0x77,
+    GRUB_ACPI_OPCODE_DIVIDE = 0x78,
+    GRUB_ACPI_OPCODE_LSHIFT = 0x79,
+    GRUB_ACPI_OPCODE_RSHIFT = 0x7a,
+    GRUB_ACPI_OPCODE_AND = 0x7b,
+    GRUB_ACPI_OPCODE_NAND = 0x7c,
+    GRUB_ACPI_OPCODE_OR = 0x7d,
+    GRUB_ACPI_OPCODE_NOR = 0x7e,
+    GRUB_ACPI_OPCODE_XOR = 0x7f,
+    GRUB_ACPI_OPCODE_CONCATRES = 0x80,
     GRUB_ACPI_OPCODE_IF = 0xa0, GRUB_ACPI_OPCODE_ONES = 0xff
   };
 enum
-- 
1.7.10.4


[-- Attachment #1.3: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Fix ACPI halt for certain DSDTs

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

В Tue, 1 Jul 2014 01:55:22 +0200
Valentin Dornauer <valentin@unimplemented.org> пишет:

> Hello!
> 
> GRUB2 currently seems to be unable to shut down my Fujitsu Lifebook
> E744 laptop using ACPI. After some debugging, I nailed the problem
> down to two problems within GRUB’s ‘acpihalt’ command. Please see
> the attached patch for my solution.
> 

See below

> +	in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
> +	simply strings!); add function to skip TermArgs.

It does not really parse every possible value of Type2Opcode, so commit
message should probably reflect it.

> +    case GRUB_ACPI_OPCODE_ADD:
> +    case GRUB_ACPI_OPCODE_CONCAT:
> +    case GRUB_ACPI_OPCODE_SUBTRACT:
> +    case GRUB_ACPI_OPCODE_MULTIPLY:
> ...

Sort by name?

> +      if (add == 0)
> +        return 0;

You use if (!add) in another places. Let's be consistent :)

>      GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
> +    GRUB_ACPI_OPCODE_ALIAS = 0x06,

Looks like list is ordered by opcode value, so it should go between
GRUB_ACPI_OPCODE_NAME and GRUB_ACPI_OPCODE_BYTE_CONST. Same also next hunk.

> +    GRUB_ACPI_OPCODE_CONCATRES = 0x80,

In my copy of ACPI spec ConcatRes has opcode 0x84.

Otherwise looks good. Vladimir, is it OK to commit?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Fix ACPI halt for certain DSDTs

[Valentin Dornauer]

[-- Attachment #1.1: Type: text/plain, Size: 1343 bytes --]

Thanks for the review, Andrey.

On 2 Jul 2014, at 19:56, Andrey Borzenkov <arvidjaar@gmail.com> wrote:
>> +	in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
>> +	simply strings!); add function to skip TermArgs.
> 
> It does not really parse every possible value of Type2Opcode, so commit
> message should probably reflect it.
Done, I hope it’s clearer now.

> 
>> +    case GRUB_ACPI_OPCODE_ADD:
>> +    case GRUB_ACPI_OPCODE_CONCAT:
>> +    case GRUB_ACPI_OPCODE_SUBTRACT:
>> +    case GRUB_ACPI_OPCODE_MULTIPLY:
>> ...
> 
> Sort by name?
Done.

> 
>> +      if (add == 0)
>> +        return 0;
> 
> You use if (!add) in another places. Let's be consistent :)
Right, done.

> 
>>     GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
>> +    GRUB_ACPI_OPCODE_ALIAS = 0x06,
> 
> Looks like list is ordered by opcode value, so it should go between
> GRUB_ACPI_OPCODE_NAME and GRUB_ACPI_OPCODE_BYTE_CONST. Same also next hunk.
Done.

> 
>> +    GRUB_ACPI_OPCODE_CONCATRES = 0x80,
> 
> In my copy of ACPI spec ConcatRes has opcode 0x84.
Good catch, don’t know how that slipped through. While verifying the other opcodes
I stumbled upon some additional opcodes with the same operands: ModOp, IndexOp,
ToStringOp. I also included them in the new patch set.

- Valentin


[-- Attachment #1.2: 0001-Fix-ACPI-halt-for-certain-DSDTs.patch --]
[-- Type: application/octet-stream, Size: 6043 bytes --]

From 3d2a0a5a1f3d9b12e44b5a6f0c34d5e8a63f5e35 Mon Sep 17 00:00:00 2001
From: Valentin Dornauer <valentin@unimplemented.org>
Date: Tue, 1 Jul 2014 01:49:52 +0200
Subject: [PATCH] Fix ACPI halt for certain DSDTs

The AML parser implements only a small subset of possible AML
opcodes. On the Fujitsu Lifebook E744 this and another bug in
the parser (incorrect handling of TermArg data types) would lead
to the laptop not turning off (_S5 not found).

* grub-core/commands/acpihalt.c: Support OpAlias in the AML parser;
in skip_ext_op(), handle some Type2Opcodes more correctly (TermArgs
aren't always simply strings!); Add function to skip TermArgs
* include/grub/acpi.h: Add new opcodes
---
 ChangeLog                     |   12 +++++++++
 grub-core/commands/acpihalt.c |   59 +++++++++++++++++++++++++++++++++++++++--
 include/grub/acpi.h           |   19 ++++++++++++-
 3 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5109c5a..17f4a7e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2014-06-30  Valentin Dornauer  <valentin@unimplemented.org>
+
+	The AML parser implements only a small subset of possible AML
+	opcodes. On the Fujitsu Lifebook E744 this and another bug in
+	the parser (incorrect handling of TermArg data types) would lead
+	to the laptop not turning off (_S5 not found).
+
+	* grub-core/commands/acpihalt.c: Support OpAlias in the AML parser;
+	in skip_ext_op(), handle some Type2Opcodes more correctly (TermArgs
+	aren't always simply strings!); Add function to skip TermArgs
+	* include/grub/acpi.h: Add new opcodes
+
 2014-06-26  Colin Watson  <cjwatson@ubuntu.com>
 
 	* docs/grub-dev.texi (Finding your way around): The build system no
diff --git a/grub-core/commands/acpihalt.c b/grub-core/commands/acpihalt.c
index 83bdfe1..da68b5b 100644
--- a/grub-core/commands/acpihalt.c
+++ b/grub-core/commands/acpihalt.c
@@ -136,6 +136,49 @@ skip_data_ref_object (const grub_uint8_t *ptr, const grub_uint8_t *end)
 }
 
 static inline grub_uint32_t
+skip_term (const grub_uint8_t *ptr, const grub_uint8_t *end)
+{
+  grub_uint32_t add;
+  const grub_uint8_t *ptr0 = ptr;
+
+  switch(*ptr)
+  {
+    case GRUB_ACPI_OPCODE_ADD:
+    case GRUB_ACPI_OPCODE_AND:
+    case GRUB_ACPI_OPCODE_CONCAT:
+    case GRUB_ACPI_OPCODE_CONCATRES:
+    case GRUB_ACPI_OPCODE_DIVIDE:
+    case GRUB_ACPI_OPCODE_INDEX:
+    case GRUB_ACPI_OPCODE_LSHIFT:
+    case GRUB_ACPI_OPCODE_MOD:
+    case GRUB_ACPI_OPCODE_MULTIPLY:
+    case GRUB_ACPI_OPCODE_NAND:
+    case GRUB_ACPI_OPCODE_NOR:
+    case GRUB_ACPI_OPCODE_OR:
+    case GRUB_ACPI_OPCODE_RSHIFT:
+    case GRUB_ACPI_OPCODE_SUBTRACT:
+    case GRUB_ACPI_OPCODE_TOSTRING:
+    case GRUB_ACPI_OPCODE_XOR:
+      /*
+       * Parameters for these opcodes: TermArg, TermArg Target, see ACPI
+       * spec r5.0, page 828f.
+       */
+      ptr++;
+      ptr += add = skip_term (ptr, end);
+      if (!add)
+        return 0;
+      ptr += add = skip_term (ptr, end);
+      if (!add)
+        return 0;
+      ptr += skip_name_string (ptr, end);
+      break;
+    default:
+      return skip_data_ref_object (ptr, end);
+  }
+  return ptr - ptr0;
+}
+
+static inline grub_uint32_t
 skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
 {
   const grub_uint8_t *ptr0 = ptr;
@@ -156,10 +199,10 @@ skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
       ptr++;
       ptr += skip_name_string (ptr, end);
       ptr++;
-      ptr += add = skip_data_ref_object (ptr, end);
+      ptr += add = skip_term (ptr, end);
       if (!add)
 	return 0;
-      ptr += add = skip_data_ref_object (ptr, end);
+      ptr += add = skip_term (ptr, end);
       if (!add)
 	return 0;
       break;
@@ -180,6 +223,7 @@ skip_ext_op (const grub_uint8_t *ptr, const grub_uint8_t *end)
   return ptr - ptr0;
 }
 
+
 static int
 get_sleep_type (grub_uint8_t *table, grub_uint8_t *ptr, grub_uint8_t *end,
 		grub_uint8_t *scope, int scope_len)
@@ -250,6 +294,17 @@ get_sleep_type (grub_uint8_t *table, grub_uint8_t *ptr, grub_uint8_t *end,
 	  if (!add)
 	    return -1;
 	  break;
+	case GRUB_ACPI_OPCODE_ALIAS:
+	  ptr++;
+	  /* We need to skip two name strings */
+	  ptr += add = skip_name_string (ptr, end);
+	  if (!add)
+	    return -1;
+	  ptr += add = skip_name_string (ptr, end);
+	  if (!add)
+	    return -1;
+	  break;
+
 	case GRUB_ACPI_OPCODE_SCOPE:
 	  {
 	    int scope_sleep_type;
diff --git a/include/grub/acpi.h b/include/grub/acpi.h
index 2ac2bd6..f6e6a11 100644
--- a/include/grub/acpi.h
+++ b/include/grub/acpi.h
@@ -191,7 +191,8 @@ void grub_acpi_halt (void);
 enum
   {
     GRUB_ACPI_OPCODE_ZERO = 0, GRUB_ACPI_OPCODE_ONE = 1,
-    GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
+    GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_ALIAS = 0x06,
+    GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
     GRUB_ACPI_OPCODE_WORD_CONST = 0x0b,
     GRUB_ACPI_OPCODE_DWORD_CONST = 0x0c,
     GRUB_ACPI_OPCODE_STRING_CONST = 0x0d,
@@ -199,6 +200,22 @@ enum
     GRUB_ACPI_OPCODE_BUFFER = 0x11,
     GRUB_ACPI_OPCODE_PACKAGE = 0x12,
     GRUB_ACPI_OPCODE_METHOD = 0x14, GRUB_ACPI_OPCODE_EXTOP = 0x5b,
+    GRUB_ACPI_OPCODE_ADD = 0x72,
+    GRUB_ACPI_OPCODE_CONCAT = 0x73,
+    GRUB_ACPI_OPCODE_SUBTRACT = 0x74,
+    GRUB_ACPI_OPCODE_MULTIPLY = 0x77,
+    GRUB_ACPI_OPCODE_DIVIDE = 0x78,
+    GRUB_ACPI_OPCODE_LSHIFT = 0x79,
+    GRUB_ACPI_OPCODE_RSHIFT = 0x7a,
+    GRUB_ACPI_OPCODE_AND = 0x7b,
+    GRUB_ACPI_OPCODE_NAND = 0x7c,
+    GRUB_ACPI_OPCODE_OR = 0x7d,
+    GRUB_ACPI_OPCODE_NOR = 0x7e,
+    GRUB_ACPI_OPCODE_XOR = 0x7f,
+    GRUB_ACPI_OPCODE_CONCATRES = 0x84,
+    GRUB_ACPI_OPCODE_MOD = 0x85,
+    GRUB_ACPI_OPCODE_INDEX = 0x88,
+    GRUB_ACPI_OPCODE_TOSTRING = 0x9c,
     GRUB_ACPI_OPCODE_CREATE_WORD_FIELD = 0x8b,
     GRUB_ACPI_OPCODE_CREATE_BYTE_FIELD = 0x8c,
     GRUB_ACPI_OPCODE_IF = 0xa0, GRUB_ACPI_OPCODE_ONES = 0xff
-- 
1.7.10.4


[-- Attachment #1.3: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] Fix ACPI halt for certain DSDTs

[Valentin Dornauer]

Hello GRUB people,
any news on this patch? I’d be glad to clarify any open issues.

Thanks!
Valentin

On 3 Jul 2014, at 16:00, Valentin Dornauer <valentin@unimplemented.org> wrote:

> Thanks for the review, Andrey.
> 
> On 2 Jul 2014, at 19:56, Andrey Borzenkov <arvidjaar@gmail.com> wrote:
>>> +	in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
>>> +	simply strings!); add function to skip TermArgs.
>> 
>> It does not really parse every possible value of Type2Opcode, so commit
>> message should probably reflect it.
> Done, I hope it’s clearer now.
> 
>> 
>>> +    case GRUB_ACPI_OPCODE_ADD:
>>> +    case GRUB_ACPI_OPCODE_CONCAT:
>>> +    case GRUB_ACPI_OPCODE_SUBTRACT:
>>> +    case GRUB_ACPI_OPCODE_MULTIPLY:
>>> ...
>> 
>> Sort by name?
> Done.
> 
>> 
>>> +      if (add == 0)
>>> +        return 0;
>> 
>> You use if (!add) in another places. Let's be consistent :)
> Right, done.
> 
>> 
>>>    GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
>>> +    GRUB_ACPI_OPCODE_ALIAS = 0x06,
>> 
>> Looks like list is ordered by opcode value, so it should go between
>> GRUB_ACPI_OPCODE_NAME and GRUB_ACPI_OPCODE_BYTE_CONST. Same also next hunk.
> Done.
> 
>> 
>>> +    GRUB_ACPI_OPCODE_CONCATRES = 0x80,
>> 
>> In my copy of ACPI spec ConcatRes has opcode 0x84.
> Good catch, don’t know how that slipped through. While verifying the other opcodes
> I stumbled upon some additional opcodes with the same operands: ModOp, IndexOp,
> ToStringOp. I also included them in the new patch set.
> 
> - Valentin
> 
> <0001-Fix-ACPI-halt-for-certain-DSDTs.patch>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Fix ACPI halt for certain DSDTs

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

Committed, thanks.
On 03.07.2014 16:00, Valentin Dornauer wrote:
> Thanks for the review, Andrey.
> 
> On 2 Jul 2014, at 19:56, Andrey Borzenkov <arvidjaar@gmail.com> wrote:
>>> +	in skip_ext_op(), correctly parse OpRegionOp (TermArgs aren't always
>>> +	simply strings!); add function to skip TermArgs.
>>
>> It does not really parse every possible value of Type2Opcode, so commit
>> message should probably reflect it.
> Done, I hope it’s clearer now.
> 
>>
>>> +    case GRUB_ACPI_OPCODE_ADD:
>>> +    case GRUB_ACPI_OPCODE_CONCAT:
>>> +    case GRUB_ACPI_OPCODE_SUBTRACT:
>>> +    case GRUB_ACPI_OPCODE_MULTIPLY:
>>> ...
>>
>> Sort by name?
> Done.
> 
>>
>>> +      if (add == 0)
>>> +        return 0;
>>
>> You use if (!add) in another places. Let's be consistent :)
> Right, done.
> 
>>
>>>     GRUB_ACPI_OPCODE_NAME = 8, GRUB_ACPI_OPCODE_BYTE_CONST = 0x0a,
>>> +    GRUB_ACPI_OPCODE_ALIAS = 0x06,
>>
>> Looks like list is ordered by opcode value, so it should go between
>> GRUB_ACPI_OPCODE_NAME and GRUB_ACPI_OPCODE_BYTE_CONST. Same also next hunk.
> Done.
> 
>>
>>> +    GRUB_ACPI_OPCODE_CONCATRES = 0x80,
>>
>> In my copy of ACPI spec ConcatRes has opcode 0x84.
> Good catch, don’t know how that slipped through. While verifying the other opcodes
> I stumbled upon some additional opcodes with the same operands: ModOp, IndexOp,
> ToStringOp. I also included them in the new patch set.
> 
> - Valentin
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]