* [PATCH] Fix ACPI halt for certain DSDTs
@ 2014-06-30 23:55 Valentin Dornauer
2014-07-02 17:56 ` Andrey Borzenkov
0 siblings, 1 reply; 5+ messages in thread
From: Valentin Dornauer @ 2014-06-30 23:55 UTC (permalink / raw)
To: grub-devel
[-- 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 --]
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Fix ACPI halt for certain DSDTs
2014-06-30 23:55 [PATCH] Fix ACPI halt for certain DSDTs Valentin Dornauer
@ 2014-07-02 17:56 ` Andrey Borzenkov
2014-07-03 14:00 ` Valentin Dornauer
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Borzenkov @ 2014-07-02 17:56 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: valentin
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix ACPI halt for certain DSDTs
2014-07-02 17:56 ` Andrey Borzenkov
@ 2014-07-03 14:00 ` Valentin Dornauer
2014-08-19 22:30 ` Valentin Dornauer
2014-09-21 16:59 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 2 replies; 5+ messages in thread
From: Valentin Dornauer @ 2014-07-03 14:00 UTC (permalink / raw)
To: Andrey Borzenkov; +Cc: The development of GNU GRUB
[-- 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 --]
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Fix ACPI halt for certain DSDTs
2014-07-03 14:00 ` Valentin Dornauer
@ 2014-08-19 22:30 ` Valentin Dornauer
2014-09-21 16:59 ` Vladimir 'φ-coder/phcoder' Serbinenko
1 sibling, 0 replies; 5+ messages in thread
From: Valentin Dornauer @ 2014-08-19 22:30 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: Andrey Borzenkov
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix ACPI halt for certain DSDTs
2014-07-03 14:00 ` Valentin Dornauer
2014-08-19 22:30 ` Valentin Dornauer
@ 2014-09-21 16:59 ` Vladimir 'φ-coder/phcoder' Serbinenko
1 sibling, 0 replies; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2014-09-21 16:59 UTC (permalink / raw)
To: The development of GNU GRUB
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-21 17:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-30 23:55 [PATCH] Fix ACPI halt for certain DSDTs Valentin Dornauer
2014-07-02 17:56 ` Andrey Borzenkov
2014-07-03 14:00 ` Valentin Dornauer
2014-08-19 22:30 ` Valentin Dornauer
2014-09-21 16:59 ` Vladimir 'φ-coder/phcoder' Serbinenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).