[PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[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.

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 <trenn@suse.de>
CC: lenb@kernel.org
CC: "Rafael J. Wysocki" <rjw@sisk.pl>
CC: linux-acpi@vger.kernel.org

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f31291b..6499cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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),

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Rafael J. Wysocki]

On Thursday, October 21, 2010, Thomas Renninger wrote:
> 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).

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

Would it be safe?

> 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 <trenn@suse.de>
> CC: lenb@kernel.org
> CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> CC: linux-acpi@vger.kernel.org
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index f31291b..6499cc4 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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),

Thanks,
Rafael

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Len Brown]

On Thu, 21 Oct 2010, Rafael J. Wysocki wrote:

> On Thursday, October 21, 2010, Thomas Renninger wrote:
> > 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).
> 
> > 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

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Rafael J. Wysocki]

On Friday, October 22, 2010, Len Brown wrote:
> applied

OK

What if I do ec_delay=0 ?

Rafael

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Thomas Renninger]

On Friday 22 October 2010 11:22:00 pm Rafael J. Wysocki wrote:
> On Friday, October 22, 2010, Len Brown wrote:
> > 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

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Rafael J. Wysocki]

On Saturday, October 23, 2010, Thomas Renninger wrote:
> On Friday 22 October 2010 11:22:00 pm Rafael J. Wysocki wrote:
> > On Friday, October 22, 2010, Len Brown wrote:
> > > 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

Re: [PATCH] ACPI: Make Embedded Controller command timeout delay configurable

[Thomas Renninger]

On Saturday 23 October 2010 01:43:24 am Rafael J. Wysocki wrote:
> On Saturday, October 23, 2010, Thomas Renninger wrote:
> > On Friday 22 October 2010 11:22:00 pm Rafael J. Wysocki wrote:
> > > On Friday, October 22, 2010, Len Brown wrote:
> > > > 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?
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