ACPI: Make Embedded Controller command timeout delay configurable

PATCH Applied Thomas Renninger 3 participants 8 messages Oct 21, 2010 – Oct 24, 2010 ACPI

Share & tools

Thomas Renninger

author

2010-10-21 16:25 UTC #0 lore raw

PATCH 1/1 ACPI: Make Embedded Controller command timeout delay configurable

Here and then there show up machines which need higher timeout values. Finding this on affected machines can be cumbersome, because ACPI_EC_DELAY is a compile option -> make it configurable via boot param.

This can even be provided writable at runtime via:
/sys/modules/acpi/parameters/ec_delay

Known machines where this helps:
Some HP machines where for whatever reasons specific EC accesses take very long at resume from S3 (in _WAK function).
The AE_TIME error is passed upwards and the ACPI interpreter will not execute the rest of the _WAK function which results in not properly initialized devices/variables with different side-effects.

Afaik, on some MSI machines this helped as well.

If this param is needed there probably are underlying problems like:
- EC firmware bug
- A kernel EC driver bug
- An ACPI interpreter behavior (e.g. timings when specific EC accesses happen and how) which the EC does not like
- ...
which should get evaluated further, but often are nasty or impossible to fix from OS side.

Signed-off-by Thomas Renninger <[email protected]>

CC [email protected]

CC "Rafael J. Wysocki" <[email protected]>

CC [email protected]

drivers/acpi/ec.c +7 -2

index f31291b..6499cc4 100644
@@ -83,6 +83,11 @@ enum {
 	EC_FLAGS_BLOCKED,		/* Transactions are blocked */
 };
 
+/* ec.c is compiled in acpi namespace so this shows up as acpi.ec_delay param */
+static unsigned int ec_delay __read_mostly = ACPI_EC_DELAY;
+module_param(ec_delay, uint, 0644);
+MODULE_PARM_DESC(ec_delay, "Timeout(ms) waited until an EC command completes");
+
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
 /* External interfaces use first EC only, so remember */
 typedef int (*acpi_ec_query_func) (void *data);

@@ -210,7 +215,7 @@ static int ec_poll(struct acpi_ec *ec)
 	int repeat = 2; /* number of command restarts */
 	while (repeat--) {
 		unsigned long delay = jiffies +
-			msecs_to_jiffies(ACPI_EC_DELAY);
+			msecs_to_jiffies(ec_delay);
 		do {
 			/* don't sleep with disabled interrupts */
 			if (EC_FLAGS_MSI || irqs_disabled()) {

@@ -265,7 +270,7 @@ static int ec_check_ibf0(struct acpi_ec *ec)
 
 static int ec_wait_ibf0(struct acpi_ec *ec)
 {
-	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	unsigned long delay = jiffies + msecs_to_jiffies(ec_delay);
 	/* interrupt wait manually if GPE mode is not active */
 	while (time_before(jiffies, delay))
 		if (wait_event_timeout(ec->wait, ec_check_ibf0(ec),

Rafael J. Wysocki

2010-10-21 19:54 UTC #1 lore raw

Quoted 3 lines · Thomas Renninger

Here and then there show up machines which need higher timeout values. Finding this on affected machines can be cumbersome, because ACPI_EC_DELAY is a compile option -> make it configurable via boot param.

I think the users should not be allowed to decrease the delay below certain reasonable limit (perhaps current value of ACPI_EC_DELAY).

Quoted 2 lines

This can even be provided writable at runtime via:
/sys/modules/acpi/parameters/ec_delay

Would it be safe?

Quoted 50 lines

Known machines where this helps:
Some HP machines where for whatever reasons specific EC accesses take very long at resume from S3 (in _WAK function).
The AE_TIME error is passed upwards and the ACPI interpreter will not execute the rest of the _WAK function which results in not properly initialized devices/variables with different side-effects.
Afaik, on some MSI machines this helped as well.
If this param is needed there probably are underlying problems like:
- EC firmware bug
- A kernel EC driver bug
- An ACPI interpreter behavior (e.g. timings when specific EC accesses happen and how) which the EC does not like
- ...
which should get evaluated further, but often are nasty or impossible to fix from OS side.
Signed-off-by Thomas Renninger <[email protected]>
CC [email protected]
CC "Rafael J. Wysocki" <[email protected]>
CC [email protected]
drivers/acpi/ec.c +6 -1
index f31291b..6499cc4 100644
@@ -83,6 +83,11 @@ enum {
 	EC_FLAGS_BLOCKED,		/* Transactions are blocked */
 };
 
+/* ec.c is compiled in acpi namespace so this shows up as acpi.ec_delay param */
+static unsigned int ec_delay __read_mostly = ACPI_EC_DELAY;
+module_param(ec_delay, uint, 0644);
+MODULE_PARM_DESC(ec_delay, "Timeout(ms) waited until an EC command completes");
+
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
 /* External interfaces use first EC only, so remember */
 typedef int (*acpi_ec_query_func) (void *data);
@@ -210,7 +215,7 @@ static int ec_poll(struct acpi_ec *ec)
 	int repeat = 2; /* number of command restarts */
 	while (repeat--) {
 		unsigned long delay = jiffies +
-			msecs_to_jiffies(ACPI_EC_DELAY);
+			msecs_to_jiffies(ec_delay);
 		do {
 			/* don't sleep with disabled interrupts */
 			if (EC_FLAGS_MSI || irqs_disabled()) {

drivers/acpi/ec.c static int ec_check_ibf0(struct acpi_ec *ec) · line 276

Quoted 8 lines

drivers/acpi/ec.c +1 -1

@@ -265,7 +270,7 @@ static int ec_check_ibf0(struct acpi_ec *ec)
 
 static int ec_wait_ibf0(struct acpi_ec *ec)
 {
-	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	unsigned long delay = jiffies + msecs_to_jiffies(ec_delay);
 	/* interrupt wait manually if GPE mode is not active */
 	while (time_before(jiffies, delay))
 		if (wait_event_timeout(ec->wait, ec_check_ibf0(ec),

Thanks,
Rafael

Len Brown

reviewer

2010-10-22 05:25 UTC #2 lore raw

Quoted 5 lines · Rafael J. Wysocki

Quoted 3 lines · Thomas Renninger
Here and then there show up machines which need higher timeout values. Finding this on affected machines can be cumbersome, because ACPI_EC_DELAY is a compile option -> make it configurable via boot param.

Quoted 7 lines

I think the users should not be allowed to decrease the delay below certain reasonable limit (perhaps current value of ACPI_EC_DELAY).
Quoted 2 lines
This can even be provided writable at runtime via:
/sys/modules/acpi/parameters/ec_delay
Would it be safe?

It is no less safe than the other sysfs knobs
that root can turn to hose the machine.

if we were concerned about the modparam becoming
a run-time API, we could make it boot-time only
and only readable at run-time.

At the moment I'm very concerned about the number of EC bugs, and so in general I'm delighted to see knobs to help debug.

cheers,
Len Brown, Intel Open Source Technology Center

Len Brown

reviewer Applied

2010-10-22 05:22 UTC #3 lore raw

In reply to Thomas Renninger

applied

thanks,
Len Brown, Intel Open Source Technology Center

Rafael J. Wysocki

2010-10-22 21:23 UTC #4 lore raw

Quoted 1 line · Len Brown

applied

What if I do ec_delay=0 ?

Rafael

Thomas Renninger

author

2010-10-22 22:46 UTC #5 lore raw

Quoted 6 lines · Rafael J. Wysocki

Quoted 1 line · Len Brown
applied
OK
What if I do ec_delay=0 ?

Same as with quite some other boot params:
Your machine won't boot.

Why should this be a problem?

Thomas

Rafael J. Wysocki

2010-10-22 23:44 UTC #6 lore raw

Quoted 11 lines · Thomas Renninger

Quoted 6 lines · Rafael J. Wysocki
Quoted 1 line · Len Brown
applied
OK
What if I do ec_delay=0 ?
Same as with quite some other boot params:
Your machine won't boot.
Why should this be a problem?

Because your intention is to allow the users to _increase_ the delay and not to decrease it. Decreasing it is known dangerous, so why don't you simply put a limit in there?

I know there are many boot params that will hurt you if not used with care, but is that a sufficient reason for adding another one?

Rafael

Thomas Renninger

author

2010-10-24 22:34 UTC #7 lore raw

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Thomas Renninger
Quoted 6 lines · Rafael J. Wysocki
Quoted 1 line · Len Brown
applied
OK
What if I do ec_delay=0 ?
Same as with quite some other boot params:
Your machine won't boot.

Quoted 8 lines

Quoted 1 line
Why should this be a problem?
Because your intention is to allow the users to _increase_ the delay and not to decrease it. Decreasing it is known dangerous, so why don't you simply put a limit in there?
I know there are many boot params that will hurt you if not used with care, but is that a sufficient reason for adding another one?

To be honest I have not thought about the debug aspect of this param when I sent it.
But I think Len is right and this can be used as a nice debug param to test whether there are EC accesses that take really long.
For example you have the requirement that your EC provides data for all requests in X ms. You boot acpi.ec_delay=X and if there are timeout complaints you can then have a look at your EC firmware or at the kernel/ACPI code what the reasons were that things timed out.

Nobody will decrease the limit, unless he wants to examine above problems. I can write kernel-parameters documentation, but I expect people finding this had a look at the code. If people see AE_TIME messages with a too low value, they should know that it obviously was the boot param they added and they can simply remove it again.

Thomas