Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-27 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:39:42 AM Kieran Clancy wrote:
> On Thu, Feb 27, 2014 at 11:46 AM, Rafael J. Wysocki  
> wrote:
> > On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:
> >>
> >> Oh, and I should add that Juan and Dennis both helped to test the
> >> patch extensively.
> >
> > You can use Tested-by or Reported-and-tested-by for that.
> 
> Is it okay to have someone listed as both 'Reviewed-by' and
> 'Tested-by', or both 'Suggested-by' and 'Tested-by'?

Yes, these are different things essentially and each of them requires time
and effort on its own.

> Thank you for your help. Once I fix up these things, I will send a patch v2.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-27 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:39:42 AM Kieran Clancy wrote:
 On Thu, Feb 27, 2014 at 11:46 AM, Rafael J. Wysocki r...@rjwysocki.net 
 wrote:
  On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:
 
  Oh, and I should add that Juan and Dennis both helped to test the
  patch extensively.
 
  You can use Tested-by or Reported-and-tested-by for that.
 
 Is it okay to have someone listed as both 'Reviewed-by' and
 'Tested-by', or both 'Suggested-by' and 'Tested-by'?

Yes, these are different things essentially and each of them requires time
and effort on its own.

 Thank you for your help. Once I fix up these things, I will send a patch v2.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Kieran Clancy wrote:

On Thu, Feb 27, 2014 at 12:29 PM, Li Guang  wrote:
   

+#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to
query
+* when trying to clear the EC */

   


20 is enough?
the query index is length of a byte.
 

On my machine, 8 seems to be enough, so 20 seems to be a conservative
maximum. Just reading your other email, maybe we should set this to
32? or 40? 100?

If it's not enough, hopefully anyone seeing bugs will notice the
warning "maximum of X stale EC events cleared".

Here's what happens if I plug/replug the AC lots of times (more than
8) during suspend:

[ 8807.019800] ACPI : EC: --->  status = 0x29
[ 8807.019804] ACPI : EC: --->  data = 0x66
[ 8807.020790] ACPI : EC: --->  status = 0x29
[ 8807.020793] ACPI : EC: --->  data = 0x66
[ 8807.021793] ACPI : EC: --->  status = 0x29
[ 8807.021798] ACPI : EC: --->  data = 0x66
[ 8807.022831] ACPI : EC: --->  status = 0x29
[ 8807.022834] ACPI : EC: --->  data = 0x66
[ 8807.023788] ACPI : EC: --->  status = 0x29
[ 8807.023792] ACPI : EC: --->  data = 0x66
[ 8807.024787] ACPI : EC: --->  status = 0x29
[ 8807.024791] ACPI : EC: --->  data = 0x66
[ 8807.025787] ACPI : EC: --->  status = 0x29
[ 8807.025790] ACPI : EC: --->  data = 0x66
[ 8807.026787] ACPI : EC: --->  status = 0x29
[ 8807.026790] ACPI : EC: --->  data = 0x66
[ 8807.027786] ACPI : EC: --->  status = 0x09
[ 8807.027790] ACPI : EC: --->  data = 0x00
[ 8807.027792] ACPI : EC: 8 stale EC events cleared

Note that most of these have SCI_EVT set, but the OS is not notified
according to ACPI specs (seemingly because these events happened
during sleep).

The _Q66 method in my DSDT, is:

 P8XH (Zero, 0x66)
 If (LEqual (B1EX, One))
 {
 Notify (BAT1, 0x80)
 }

So, basically, this is supposed to notify that the battery (BAT1 =
PNP0C0A) has changed state, but they are stale events so we don't run
the handlers.

   

+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on
boot/resume */
   

seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)
 

In my mind this is referring to the function name (acpi_ec_)clear.
Perhaps we could just make the connection more explicit in the
comment:

/* needs acpi_ec_clear() on boot/resume */

Not sure if this is better?

   

+   /* Some hardware may need the EC to be cleared before use */
   

description is implicit, should specify what we clear is Q event, not EC.
 

Are Q events the only thing we can get from the EC data port? I've
read the relevant parts of the ACPI spec and I can't say I am 100%
sure.

   

I guess you want to clear Q events here,
EC usually has ACPI space to be read by cmd 80.

Thanks!



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 12:29 PM, Li Guang  wrote:
>> +#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to
>> query
>> +* when trying to clear the EC */
>>
>
>
> 20 is enough?
> the query index is length of a byte.

On my machine, 8 seems to be enough, so 20 seems to be a conservative
maximum. Just reading your other email, maybe we should set this to
32? or 40? 100?

If it's not enough, hopefully anyone seeing bugs will notice the
warning "maximum of X stale EC events cleared".

Here's what happens if I plug/replug the AC lots of times (more than
8) during suspend:

[ 8807.019800] ACPI : EC: ---> status = 0x29
[ 8807.019804] ACPI : EC: ---> data = 0x66
[ 8807.020790] ACPI : EC: ---> status = 0x29
[ 8807.020793] ACPI : EC: ---> data = 0x66
[ 8807.021793] ACPI : EC: ---> status = 0x29
[ 8807.021798] ACPI : EC: ---> data = 0x66
[ 8807.022831] ACPI : EC: ---> status = 0x29
[ 8807.022834] ACPI : EC: ---> data = 0x66
[ 8807.023788] ACPI : EC: ---> status = 0x29
[ 8807.023792] ACPI : EC: ---> data = 0x66
[ 8807.024787] ACPI : EC: ---> status = 0x29
[ 8807.024791] ACPI : EC: ---> data = 0x66
[ 8807.025787] ACPI : EC: ---> status = 0x29
[ 8807.025790] ACPI : EC: ---> data = 0x66
[ 8807.026787] ACPI : EC: ---> status = 0x29
[ 8807.026790] ACPI : EC: ---> data = 0x66
[ 8807.027786] ACPI : EC: ---> status = 0x09
[ 8807.027790] ACPI : EC: ---> data = 0x00
[ 8807.027792] ACPI : EC: 8 stale EC events cleared

Note that most of these have SCI_EVT set, but the OS is not notified
according to ACPI specs (seemingly because these events happened
during sleep).

The _Q66 method in my DSDT, is:

P8XH (Zero, 0x66)
If (LEqual (B1EX, One))
{
Notify (BAT1, 0x80)
}

So, basically, this is supposed to notify that the battery (BAT1 =
PNP0C0A) has changed state, but they are stale events so we don't run
the handlers.

>> +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on
>> boot/resume */
>
> seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
> seems too long :-)

In my mind this is referring to the function name (acpi_ec_)clear.
Perhaps we could just make the connection more explicit in the
comment:

/* needs acpi_ec_clear() on boot/resume */

Not sure if this is better?

>> +   /* Some hardware may need the EC to be cleared before use */
>
> description is implicit, should specify what we clear is Q event, not EC.

Are Q events the only thing we can get from the EC data port? I've
read the relevant parts of the ACPI spec and I can't say I am 100%
sure.

Thank you for your advice,
Kieran.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Juan Manuel Cabo wrote:

On 02/27/2014 12:14 AM, Li Guang wrote:
   

oh, sorry, I'm referring internal EC firmware code
for Q event queuing, not ACPI SPEC, ;-)
for machine you tested, 8 is the queue size,
but for some unknown also nasty EC firmwares(let's suppose it exists),
it may queue more Q events.
and I saw several firmwares queued 32 events by default,
then, let's say, they be used for some samsung products,
and also they also forgot to deal with sleep/resume state,
then, we'll also leave stale Q event there.

Thanks!

 

We tested each on our different samsung models (intel, amd), and it
was 8 across. But you're right, there might be more in the future.

  I even saw a bug report in ubuntu's launchpad of an HP with a similar
sounding problem, ( 
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.20/+bug/89860 )
which I have no idea if it was caused by the same issue, but if in the future,
the flag ec_clear_on_resume is used to match other DMI's, it might
be a good idea to make the max iteration count bigger.

   The only reason that there is a max iteration count, was to prevent
an unexpected case in which an unknown EC never returns 0 after
queue emptied. So far it hasn't been the case. Can we count on it?.
The loop currently does finish early when there are no more events.

I guess changing it 255 or 1000 would be enough, right?

   


can't imagine 1K bytes be dissipated on Q event,
EC's ram is usually expensive,
I think 255 is really enough. :-)

Thanks!



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Juan Manuel Cabo 
On 02/27/2014 12:14 AM, Li Guang wrote:
> oh, sorry, I'm referring internal EC firmware code
> for Q event queuing, not ACPI SPEC, ;-)
> for machine you tested, 8 is the queue size,
> but for some unknown also nasty EC firmwares(let's suppose it exists),
> it may queue more Q events.
> and I saw several firmwares queued 32 events by default,
> then, let's say, they be used for some samsung products,
> and also they also forgot to deal with sleep/resume state,
> then, we'll also leave stale Q event there.
>
> Thanks!
>

We tested each on our different samsung models (intel, amd), and it
was 8 across. But you're right, there might be more in the future.

 I even saw a bug report in ubuntu's launchpad of an HP with a similar
sounding problem, ( 
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.20/+bug/89860 )
which I have no idea if it was caused by the same issue, but if in the future,
the flag ec_clear_on_resume is used to match other DMI's, it might
be a good idea to make the max iteration count bigger.

  The only reason that there is a max iteration count, was to prevent
an unexpected case in which an unknown EC never returns 0 after
queue emptied. So far it hasn't been the case. Can we count on it?.
The loop currently does finish early when there are no more events.

I guess changing it 255 or 1000 would be enough, right?

Cheers!
-- 
Juan Manuel Cabo



>>  For us, a query is just: send 0x84 through EC CMD port, and read status
>> from CMD port and event type from EC DATA port. This is done with
>> the usual ec.c functions that would handle a query after a GPE interrupt,
>> but using them instead to poll (not GPE initiated) at awake. The EC would
>> then return status without 0x20 mask and 'event type'==0 when no more left.
>>
>> -- 
>> Juan Manuel Cabo
>>
>>
>>
>>   
enum {
EC_FLAGS_QUERY_PENDING,/* Query is pending */
 @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be 
 validated */
static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT 
 scan */
 +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on 
 boot/resume */


>>> seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
>>> seems too long :-)
>>>
>>> 
/* 
 --
 Transaction Management
 @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

EXPORT_SYMBOL(ec_get_handle);

 +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
 +
 +/* run with locked ec mutex */
 +static void acpi_ec_clear(struct acpi_ec *ec)
 +{
 +int i, status;
 +u8 value = 0;
 +
 +for (i = 0; i<   ACPI_EC_CLEAR_MAX; i++) {
 +status = acpi_ec_query_unlocked(ec,);
 +if (status || !value)
 +break;
 +}
 +
 +if (i == ACPI_EC_CLEAR_MAX)
 +pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
 +else
 +pr_info("%d stale EC events cleared\n", i);
 +}
 +
void acpi_ec_block_transactions(void)
{
struct acpi_ec *ec = first_ec;
 @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
mutex_lock(>mutex);
/* Allow transactions to be carried out again */
clear_bit(EC_FLAGS_BLOCKED,>flags);
 +
 +if (EC_FLAGS_CLEAR_ON_RESUME)
 +acpi_ec_clear(ec);
 +
mutex_unlock(>mutex);
}

 @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

/* EC is fully operational, allow queries */
clear_bit(EC_FLAGS_QUERY_PENDING,>flags);
 +
 +/* Some hardware may need the EC to be cleared before use */


>>> description is implicit, should specify what we clear is Q event, not EC.
>>>
>>> Thanks!
>>> Li Guang
>>>
>>> 
 +if (EC_FLAGS_CLEAR_ON_RESUME) {
 +mutex_lock(>mutex);
 +acpi_ec_clear(ec);
 +mutex_unlock(>mutex);
 +}
return ret;
}

 @@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
 dmi_system_id *id)
return 0;
}

 +/*
 + * On some hardware it is necessary to clear events accumulated by the EC 
 during
 + * sleep. These ECs stop reporting GPEs until they are manually polled, 
 if too
 + * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
 + *
 + * https://bugzilla.kernel.org/show_bug.cgi?id=44161
 + *
 + * Ideally, the EC should also be instructed not to accumulate events 
 during
 + * sleep (which Windows seems to do somehow), but the interface to

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Juan Manuel Cabo wrote:


   

that's really nasty EC firmware!
 

Yes!
And this bug has been unsolved for about two years.

   

20 is enough?
the query index is length of a byte.

 

According to our humble tests, 8 is the maximum number of
accumulated events. For instance, if the one plugs or unplugs the PSU
16 times, (or battery % changes 16 times) during S3 sleep, then the
EC returns no more than 8 events when polled by this patch or by
my userspace util.
And having reached 8 events, it won't produce its GPE until queried.
   


that surely indicts their EC firmware only queued 8 events,

   

the query index is length of a byte.
 

There is no query index, unless you refer to something else (I'm sorry if its
something that escapes me).
   


oh, sorry, I'm referring internal EC firmware code
for Q event queuing, not ACPI SPEC, ;-)
for machine you tested, 8 is the queue size,
but for some unknown also nasty EC firmwares(let's suppose it exists),
it may queue more Q events.
and I saw several firmwares queued 32 events by default,
then, let's say, they be used for some samsung products,
and also they also forgot to deal with sleep/resume state,
then, we'll also leave stale Q event there.

Thanks!


 For us, a query is just: send 0x84 through EC CMD port, and read status
from CMD port and event type from EC DATA port. This is done with
the usual ec.c functions that would handle a query after a GPE interrupt,
but using them instead to poll (not GPE initiated) at awake. The EC would
then return status without 0x20 mask and 'event type'==0 when no more left.

--
Juan Manuel Cabo



   

   enum {
   EC_FLAGS_QUERY_PENDING,/* Query is pending */
@@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
   static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
   static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
   static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan 
*/
+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */

   

seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)

 

   /* --
Transaction Management
@@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

   EXPORT_SYMBOL(ec_get_handle);

+static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
+
+/* run with locked ec mutex */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+int i, status;
+u8 value = 0;
+
+for (i = 0; i<   ACPI_EC_CLEAR_MAX; i++) {
+status = acpi_ec_query_unlocked(ec,);
+if (status || !value)
+break;
+}
+
+if (i == ACPI_EC_CLEAR_MAX)
+pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+else
+pr_info("%d stale EC events cleared\n", i);
+}
+
   void acpi_ec_block_transactions(void)
   {
   struct acpi_ec *ec = first_ec;
@@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
   mutex_lock(>mutex);
   /* Allow transactions to be carried out again */
   clear_bit(EC_FLAGS_BLOCKED,>flags);
+
+if (EC_FLAGS_CLEAR_ON_RESUME)
+acpi_ec_clear(ec);
+
   mutex_unlock(>mutex);
   }

@@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

   /* EC is fully operational, allow queries */
   clear_bit(EC_FLAGS_QUERY_PENDING,>flags);
+
+/* Some hardware may need the EC to be cleared before use */

   

description is implicit, should specify what we clear is Q event, not EC.

Thanks!
Li Guang

 

+if (EC_FLAGS_CLEAR_ON_RESUME) {
+mutex_lock(>mutex);
+acpi_ec_clear(ec);
+mutex_unlock(>mutex);
+}
   return ret;
   }

@@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
dmi_system_id *id)
   return 0;
   }

+/*
+ * On some hardware it is necessary to clear events accumulated by the EC 
during
+ * sleep. These ECs stop reporting GPEs until they are manually polled, if too
+ * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
+ *
+ * https://bugzilla.kernel.org/show_bug.cgi?id=44161
+ *
+ * Ideally, the EC should also be instructed not to accumulate events during
+ * sleep (which Windows seems to do somehow), but the interface to control this
+ * behaviour is not known at this time.
+ *
+ * Models known to be affected are Samsung 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
+ * however it is very likely that other Samsung models are affected.
+ *
+ * On systems which don't accumulate EC events during sleep, this extra check
+ * should be harmless.
+ */
+static int ec_clear_on_resume(const struct dmi_system_id *id)
+{
+pr_debug("Detected system needing EC poll on resume.\n");
+EC_FLAGS_CLEAR_ON_RESUME = 1;
+return 0;
+}
+
   static struct dmi_system_id ec_dmi_table[] __initdata = {
   {
   ec_skip_dsdt_scan, "Compal JFL92", {
@@ -965,6 +1023,9 @@ static struct

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Juan Manuel Cabo 
 
> that's really nasty EC firmware!

Yes!
And this bug has been unsolved for about two years.

> 20 is enough?
> the query index is length of a byte.
>
According to our humble tests, 8 is the maximum number of
accumulated events. For instance, if the one plugs or unplugs the PSU
16 times, (or battery % changes 16 times) during S3 sleep, then the
EC returns no more than 8 events when polled by this patch or by
my userspace util.
And having reached 8 events, it won't produce its GPE until queried.

> the query index is length of a byte.

There is no query index, unless you refer to something else (I'm sorry if its
something that escapes me).
For us, a query is just: send 0x84 through EC CMD port, and read status
from CMD port and event type from EC DATA port. This is done with
the usual ec.c functions that would handle a query after a GPE interrupt,
but using them instead to poll (not GPE initiated) at awake. The EC would
then return status without 0x20 mask and 'event type'==0 when no more left.

--
Juan Manuel Cabo 



>>
>>   enum {
>>   EC_FLAGS_QUERY_PENDING,/* Query is pending */
>> @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
>>   static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
>>   static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
>>   static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT 
>> scan */
>> +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume 
>> */
>>
>
> seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
> seems too long :-)
>
>>
>>   /* 
>> --
>>Transaction Management
>> @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)
>>
>>   EXPORT_SYMBOL(ec_get_handle);
>>
>> +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
>> +
>> +/* run with locked ec mutex */
>> +static void acpi_ec_clear(struct acpi_ec *ec)
>> +{
>> +int i, status;
>> +u8 value = 0;
>> +
>> +for (i = 0; i<  ACPI_EC_CLEAR_MAX; i++) {
>> +status = acpi_ec_query_unlocked(ec,);
>> +if (status || !value)
>> +break;
>> +}
>> +
>> +if (i == ACPI_EC_CLEAR_MAX)
>> +pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
>> +else
>> +pr_info("%d stale EC events cleared\n", i);
>> +}
>> +
>>   void acpi_ec_block_transactions(void)
>>   {
>>   struct acpi_ec *ec = first_ec;
>> @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
>>   mutex_lock(>mutex);
>>   /* Allow transactions to be carried out again */
>>   clear_bit(EC_FLAGS_BLOCKED,>flags);
>> +
>> +if (EC_FLAGS_CLEAR_ON_RESUME)
>> +acpi_ec_clear(ec);
>> +
>>   mutex_unlock(>mutex);
>>   }
>>
>> @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)
>>
>>   /* EC is fully operational, allow queries */
>>   clear_bit(EC_FLAGS_QUERY_PENDING,>flags);
>> +
>> +/* Some hardware may need the EC to be cleared before use */
>>
>
> description is implicit, should specify what we clear is Q event, not EC.
>
> Thanks!
> Li Guang
>
>> +if (EC_FLAGS_CLEAR_ON_RESUME) {
>> +mutex_lock(>mutex);
>> +acpi_ec_clear(ec);
>> +mutex_unlock(>mutex);
>> +}
>>   return ret;
>>   }
>>
>> @@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
>> dmi_system_id *id)
>>   return 0;
>>   }
>>
>> +/*
>> + * On some hardware it is necessary to clear events accumulated by the EC 
>> during
>> + * sleep. These ECs stop reporting GPEs until they are manually polled, if 
>> too
>> + * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
>> + *
>> + * https://bugzilla.kernel.org/show_bug.cgi?id=44161
>> + *
>> + * Ideally, the EC should also be instructed not to accumulate events during
>> + * sleep (which Windows seems to do somehow), but the interface to control 
>> this
>> + * behaviour is not known at this time.
>> + *
>> + * Models known to be affected are Samsung 
>> 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
>> + * however it is very likely that other Samsung models are affected.
>> + *
>> + * On systems which don't accumulate EC events during sleep, this extra 
>> check
>> + * should be harmless.
>> + */
>> +static int ec_clear_on_resume(const struct dmi_system_id *id)
>> +{
>> +pr_debug("Detected system needing EC poll on resume.\n");
>> +EC_FLAGS_CLEAR_ON_RESUME = 1;
>> +return 0;
>> +}
>> +
>>   static struct dmi_system_id ec_dmi_table[] __initdata = {
>>   {
>>   ec_skip_dsdt_scan, "Compal JFL92", {
>> @@ -965,6 +1023,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = 
>> {
>>   ec_validate_ecdt, "ASUS hardware", {
>>   DMI_MATCH(DMI_SYS_VENDOR, "ASUSTek Computer Inc."),
>>   DMI_MATCH(DMI_PRODUCT_NAME, "L4R"),}, NULL},
>> +{
>> +ec_clear_on_resume, "Samsung hardware", {
>> +DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Kieran Clancy wrote:

A number of Samsung notebooks (530Uxx/535Uxx/540Uxx/550Pxx/900Xxx/etc)
continue to log events during sleep (lid open/close, AC plug/unplug,
battery level change), which accumulate in the EC until a buffer fills.
After the buffer is full (tests suggest it holds 8 events), GPEs stop
being triggered for new events. This state persists on wake or even on
power cycle, and prevents new events from being registered until the EC
is manually polled.

   


that's really nasty EC firmware!


This is the root cause of a number of bugs, including AC not being
detected properly, lid close not triggering suspend, and low ambient
light not triggering the keyboard backlight. The bug also seemed to be
responsible for performance issues on at least one user's machine.

Juan Manuel Cabo found the cause of bug and the workaround of polling
the EC manually on wake.

This patch:
- Adds a function acpi_ec_clear() which polls the EC, at most
   ACPI_EC_CLEAR_MAX (currently 20) times. A warning is logged if this
   limit is reached.
- Adds a flag EC_FLAGS_CLEAR_ON_RESUME which is set to 1 if the DMI
   system vendor is Samsung. This check could be replaced by several more
   specific DMI vendor/product pairs, but it's likely that the bug
   affects more Samsung products than just the five series mentioned
   above. Further, it should not be harmful to run acpi_ec_clear() on
   systems without the bug; it will return immediately after finding no
   data waiting.
- Runs acpi_ec_clear() on initialisation (boot), from acpi_ec_add()
- Runs acpi_ec_clear() on wake, from acpi_ec_unblock_transactions()

References: https://bugzilla.kernel.org/show_bug.cgi?id=44161
References: https://bugzilla.kernel.org/show_bug.cgi?id=45461
References: https://bugzilla.kernel.org/show_bug.cgi?id=57271
Signed-off-by: Kieran Clancy
Signed-off-by: Lan Tianyu
Signed-off-by: Juan Manuel Cabo
Signed-off-by: Dennis Jansen
Tested-by: Maurizio D'Addona
Tested-by: San Zamoyski
---
  drivers/acpi/ec.c | 61 +++
  1 file changed, 61 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 959d41a..f437d9a 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -67,6 +67,8 @@ enum ec_command {
  #define ACPI_EC_DELAY 500 /* Wait 500ms max. during EC ops */
  #define ACPI_EC_UDELAY_GLK1000/* Wait 1ms max. to get global lock */
  #define ACPI_EC_MSI_UDELAY550 /* Wait 550us for MSI EC */
+#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to query
+* when trying to clear the EC */
   


20 is enough?
the query index is length of a byte.



  enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
@@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
  static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
  static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
  static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */
   


seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)



  /* --
   Transaction Management
@@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

  EXPORT_SYMBOL(ec_get_handle);

+static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
+
+/* run with locked ec mutex */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+   int i, status;
+   u8 value = 0;
+
+   for (i = 0; i<  ACPI_EC_CLEAR_MAX; i++) {
+   status = acpi_ec_query_unlocked(ec,);
+   if (status || !value)
+   break;
+   }
+
+   if (i == ACPI_EC_CLEAR_MAX)
+   pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
+   else
+   pr_info("%d stale EC events cleared\n", i);
+}
+
  void acpi_ec_block_transactions(void)
  {
struct acpi_ec *ec = first_ec;
@@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
mutex_lock(>mutex);
/* Allow transactions to be carried out again */
clear_bit(EC_FLAGS_BLOCKED,>flags);
+
+   if (EC_FLAGS_CLEAR_ON_RESUME)
+   acpi_ec_clear(ec);
+
mutex_unlock(>mutex);
  }

@@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

/* EC is fully operational, allow queries */
clear_bit(EC_FLAGS_QUERY_PENDING,>flags);
+
+   /* Some hardware may need the EC to be cleared before use */
   


description is implicit, should specify what we clear is Q event, not EC.

Thanks!
Li Guang


+   if (EC_FLAGS_CLEAR_ON_RESUME) {
+   mutex_lock(>mutex);
+   acpi_ec_clear(ec);
+   mutex_unlock(>mutex);
+   }
return ret;
  }

@@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 11:46 AM, Rafael J. Wysocki  wrote:
> On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:
>>
>> Oh, and I should add that Juan and Dennis both helped to test the
>> patch extensively.
>
> You can use Tested-by or Reported-and-tested-by for that.

Is it okay to have someone listed as both 'Reviewed-by' and
'Tested-by', or both 'Suggested-by' and 'Tested-by'?

Thank you for your help. Once I fix up these things, I will send a patch v2.

Kieran.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:
> On Thu, Feb 27, 2014 at 11:15 AM, Kieran Clancy  
> wrote:
> >
> > I am the primary author, but I based the loop which calls
> > acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
> > initial idea (userspace workaround), and Dennis and Lan Tianyu
> > reviewed and suggested some changes to the code.
> 
> Oh, and I should add that Juan and Dennis both helped to test the
> patch extensively.

You can use Tested-by or Reported-and-tested-by for that.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:15:05 AM Kieran Clancy wrote:
> On Thu, Feb 27, 2014 at 10:15 AM, Rafael J. Wysocki  
> wrote:
> > On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
> >> Signed-off-by: Kieran Clancy 
> >> Signed-off-by: Lan Tianyu 
> >> Signed-off-by: Juan Manuel Cabo 
> >> Signed-off-by: Dennis Jansen 
> >
> > There are too many sign-offs under this patch.  I suppose some of them
> > should be Acked-by or Reviewed-by.
> >
> > Are you the author?
> 
> Sorry about that, I wasn't sure of the best way to acknowledge the
> people involved.
> 
> I am the primary author, but I based the loop which calls
> acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
> initial idea (userspace workaround), and Dennis and Lan Tianyu
> reviewed and suggested some changes to the code.
> 
> What would you usually do this kind of situation?

I usually add acknowledgements to the changelog without using any tags,
like "This patch is based on previous work by ."  And then you
can give a link to that work in References:.

> I suppose I could put something such as:
>
> Signed-off-by: Kieran Clancy 
> Signed-off-by: Lan Tianyu 
> Suggested-by: Juan Manuel Cabo 
> Reviewed-by: Lan Tianyu 
> Reviewed-by: Dennis Jansen 
> 
> Would this be preferable?

Yes, that'd be fine by me.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 11:15 AM, Kieran Clancy  wrote:
>
> I am the primary author, but I based the loop which calls
> acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
> initial idea (userspace workaround), and Dennis and Lan Tianyu
> reviewed and suggested some changes to the code.

Oh, and I should add that Juan and Dennis both helped to test the
patch extensively.

Kieran
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 10:15 AM, Rafael J. Wysocki  wrote:
> On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
>> Signed-off-by: Kieran Clancy 
>> Signed-off-by: Lan Tianyu 
>> Signed-off-by: Juan Manuel Cabo 
>> Signed-off-by: Dennis Jansen 
>
> There are too many sign-offs under this patch.  I suppose some of them
> should be Acked-by or Reviewed-by.
>
> Are you the author?

Sorry about that, I wasn't sure of the best way to acknowledge the
people involved.

I am the primary author, but I based the loop which calls
acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
initial idea (userspace workaround), and Dennis and Lan Tianyu
reviewed and suggested some changes to the code.

What would you usually do this kind of situation?

I suppose I could put something such as:

Signed-off-by: Kieran Clancy 
Signed-off-by: Lan Tianyu 
Suggested-by: Juan Manuel Cabo 
Reviewed-by: Lan Tianyu 
Reviewed-by: Dennis Jansen 

Would this be preferable?

Thanks,
Kieran Clancy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
> A number of Samsung notebooks (530Uxx/535Uxx/540Uxx/550Pxx/900Xxx/etc)
> continue to log events during sleep (lid open/close, AC plug/unplug,
> battery level change), which accumulate in the EC until a buffer fills.
> After the buffer is full (tests suggest it holds 8 events), GPEs stop
> being triggered for new events. This state persists on wake or even on
> power cycle, and prevents new events from being registered until the EC
> is manually polled.
> 
> This is the root cause of a number of bugs, including AC not being
> detected properly, lid close not triggering suspend, and low ambient
> light not triggering the keyboard backlight. The bug also seemed to be
> responsible for performance issues on at least one user's machine.
> 
> Juan Manuel Cabo found the cause of bug and the workaround of polling
> the EC manually on wake.
> 
> This patch:
> - Adds a function acpi_ec_clear() which polls the EC, at most
>   ACPI_EC_CLEAR_MAX (currently 20) times. A warning is logged if this
>   limit is reached.
> - Adds a flag EC_FLAGS_CLEAR_ON_RESUME which is set to 1 if the DMI
>   system vendor is Samsung. This check could be replaced by several more
>   specific DMI vendor/product pairs, but it's likely that the bug
>   affects more Samsung products than just the five series mentioned
>   above. Further, it should not be harmful to run acpi_ec_clear() on
>   systems without the bug; it will return immediately after finding no
>   data waiting.
> - Runs acpi_ec_clear() on initialisation (boot), from acpi_ec_add()
> - Runs acpi_ec_clear() on wake, from acpi_ec_unblock_transactions()
> 
> References: https://bugzilla.kernel.org/show_bug.cgi?id=44161
> References: https://bugzilla.kernel.org/show_bug.cgi?id=45461
> References: https://bugzilla.kernel.org/show_bug.cgi?id=57271
> Signed-off-by: Kieran Clancy 
> Signed-off-by: Lan Tianyu 
> Signed-off-by: Juan Manuel Cabo 
> Signed-off-by: Dennis Jansen 

There are too many sign-offs under this patch.  I suppose some of them
should be Acked-by or Reviewed-by.

Are you the author?

> Tested-by: Maurizio D'Addona 
> Tested-by: San Zamoyski 
> ---
>  drivers/acpi/ec.c | 61 
> +++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 959d41a..f437d9a 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -67,6 +67,8 @@ enum ec_command {
>  #define ACPI_EC_DELAY500 /* Wait 500ms max. during EC 
> ops */
>  #define ACPI_EC_UDELAY_GLK   1000/* Wait 1ms max. to get global lock */
>  #define ACPI_EC_MSI_UDELAY   550 /* Wait 550us for MSI EC */
> +#define ACPI_EC_CLEAR_MAX20  /* Maximum number of events to query
> +  * when trying to clear the EC */
>  
>  enum {
>   EC_FLAGS_QUERY_PENDING, /* Query is pending */
> @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
>  static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
>  static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
>  static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan 
> */
> +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */
>  
>  /* --
>   Transaction Management
> @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)
>  
>  EXPORT_SYMBOL(ec_get_handle);
>  
> +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
> +
> +/* run with locked ec mutex */
> +static void acpi_ec_clear(struct acpi_ec *ec)
> +{
> + int i, status;
> + u8 value = 0;
> +
> + for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
> + status = acpi_ec_query_unlocked(ec, );
> + if (status || !value)
> + break;
> + }
> +
> + if (i == ACPI_EC_CLEAR_MAX)
> + pr_warn("Warning: Maximum of %d stale EC events cleared\n", i);
> + else
> + pr_info("%d stale EC events cleared\n", i);
> +}
> +
>  void acpi_ec_block_transactions(void)
>  {
>   struct acpi_ec *ec = first_ec;
> @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
>   mutex_lock(>mutex);
>   /* Allow transactions to be carried out again */
>   clear_bit(EC_FLAGS_BLOCKED, >flags);
> +
> + if (EC_FLAGS_CLEAR_ON_RESUME)
> + acpi_ec_clear(ec);
> +
>   mutex_unlock(>mutex);
>  }
>  
> @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)
>  
>   /* EC is fully operational, allow queries */
>   clear_bit(EC_FLAGS_QUERY_PENDING, >flags);
> +
> + /* Some hardware may need the EC to be cleared before use */
> + if (EC_FLAGS_CLEAR_ON_RESUME) {
> + mutex_lock(>mutex);
> + acpi_ec_clear(ec);
> + mutex_unlock(>mutex);
> + }
>   return ret;
>  }
>  
> @@ -922,6 +956,30 @@

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
 A number of Samsung notebooks (530Uxx/535Uxx/540Uxx/550Pxx/900Xxx/etc)
 continue to log events during sleep (lid open/close, AC plug/unplug,
 battery level change), which accumulate in the EC until a buffer fills.
 After the buffer is full (tests suggest it holds 8 events), GPEs stop
 being triggered for new events. This state persists on wake or even on
 power cycle, and prevents new events from being registered until the EC
 is manually polled.
 
 This is the root cause of a number of bugs, including AC not being
 detected properly, lid close not triggering suspend, and low ambient
 light not triggering the keyboard backlight. The bug also seemed to be
 responsible for performance issues on at least one user's machine.
 
 Juan Manuel Cabo found the cause of bug and the workaround of polling
 the EC manually on wake.
 
 This patch:
 - Adds a function acpi_ec_clear() which polls the EC, at most
   ACPI_EC_CLEAR_MAX (currently 20) times. A warning is logged if this
   limit is reached.
 - Adds a flag EC_FLAGS_CLEAR_ON_RESUME which is set to 1 if the DMI
   system vendor is Samsung. This check could be replaced by several more
   specific DMI vendor/product pairs, but it's likely that the bug
   affects more Samsung products than just the five series mentioned
   above. Further, it should not be harmful to run acpi_ec_clear() on
   systems without the bug; it will return immediately after finding no
   data waiting.
 - Runs acpi_ec_clear() on initialisation (boot), from acpi_ec_add()
 - Runs acpi_ec_clear() on wake, from acpi_ec_unblock_transactions()
 
 References: https://bugzilla.kernel.org/show_bug.cgi?id=44161
 References: https://bugzilla.kernel.org/show_bug.cgi?id=45461
 References: https://bugzilla.kernel.org/show_bug.cgi?id=57271
 Signed-off-by: Kieran Clancy clancy.kie...@gmail.com
 Signed-off-by: Lan Tianyu tianyu@intel.com
 Signed-off-by: Juan Manuel Cabo juanmanuel.c...@gmail.com
 Signed-off-by: Dennis Jansen dennis.jan...@web.de

There are too many sign-offs under this patch.  I suppose some of them
should be Acked-by or Reviewed-by.

Are you the author?

 Tested-by: Maurizio D'Addona mauritiusd...@gmail.com
 Tested-by: San Zamoyski s...@plusnet.pl
 ---
  drivers/acpi/ec.c | 61 
 +++
  1 file changed, 61 insertions(+)
 
 diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
 index 959d41a..f437d9a 100644
 --- a/drivers/acpi/ec.c
 +++ b/drivers/acpi/ec.c
 @@ -67,6 +67,8 @@ enum ec_command {
  #define ACPI_EC_DELAY500 /* Wait 500ms max. during EC 
 ops */
  #define ACPI_EC_UDELAY_GLK   1000/* Wait 1ms max. to get global lock */
  #define ACPI_EC_MSI_UDELAY   550 /* Wait 550us for MSI EC */
 +#define ACPI_EC_CLEAR_MAX20  /* Maximum number of events to query
 +  * when trying to clear the EC */
  
  enum {
   EC_FLAGS_QUERY_PENDING, /* Query is pending */
 @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
  static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
  static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
  static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan 
 */
 +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */
  
  /* --
   Transaction Management
 @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)
  
  EXPORT_SYMBOL(ec_get_handle);
  
 +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
 +
 +/* run with locked ec mutex */
 +static void acpi_ec_clear(struct acpi_ec *ec)
 +{
 + int i, status;
 + u8 value = 0;
 +
 + for (i = 0; i  ACPI_EC_CLEAR_MAX; i++) {
 + status = acpi_ec_query_unlocked(ec, value);
 + if (status || !value)
 + break;
 + }
 +
 + if (i == ACPI_EC_CLEAR_MAX)
 + pr_warn(Warning: Maximum of %d stale EC events cleared\n, i);
 + else
 + pr_info(%d stale EC events cleared\n, i);
 +}
 +
  void acpi_ec_block_transactions(void)
  {
   struct acpi_ec *ec = first_ec;
 @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
   mutex_lock(ec-mutex);
   /* Allow transactions to be carried out again */
   clear_bit(EC_FLAGS_BLOCKED, ec-flags);
 +
 + if (EC_FLAGS_CLEAR_ON_RESUME)
 + acpi_ec_clear(ec);
 +
   mutex_unlock(ec-mutex);
  }
  
 @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)
  
   /* EC is fully operational, allow queries */
   clear_bit(EC_FLAGS_QUERY_PENDING, ec-flags);
 +
 + /* Some hardware may need the EC to be cleared before use */
 + if (EC_FLAGS_CLEAR_ON_RESUME) {
 + mutex_lock(ec-mutex);
 + acpi_ec_clear(ec);
 + mutex_unlock(ec-mutex);
 + }
   return ret;
  }
  
 @@

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 10:15 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:
 On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
 Signed-off-by: Kieran Clancy clancy.kie...@gmail.com
 Signed-off-by: Lan Tianyu tianyu@intel.com
 Signed-off-by: Juan Manuel Cabo juanmanuel.c...@gmail.com
 Signed-off-by: Dennis Jansen dennis.jan...@web.de

 There are too many sign-offs under this patch.  I suppose some of them
 should be Acked-by or Reviewed-by.

 Are you the author?

Sorry about that, I wasn't sure of the best way to acknowledge the
people involved.

I am the primary author, but I based the loop which calls
acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
initial idea (userspace workaround), and Dennis and Lan Tianyu
reviewed and suggested some changes to the code.

What would you usually do this kind of situation?

I suppose I could put something such as:

Signed-off-by: Kieran Clancy clancy.kie...@gmail.com
Signed-off-by: Lan Tianyu tianyu@intel.com
Suggested-by: Juan Manuel Cabo juanmanuel.c...@gmail.com
Reviewed-by: Lan Tianyu tianyu@intel.com
Reviewed-by: Dennis Jansen dennis.jan...@web.de

Would this be preferable?

Thanks,
Kieran Clancy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 11:15 AM, Kieran Clancy clancy.kie...@gmail.com wrote:

 I am the primary author, but I based the loop which calls
 acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
 initial idea (userspace workaround), and Dennis and Lan Tianyu
 reviewed and suggested some changes to the code.

Oh, and I should add that Juan and Dennis both helped to test the
patch extensively.

Kieran
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:15:05 AM Kieran Clancy wrote:
 On Thu, Feb 27, 2014 at 10:15 AM, Rafael J. Wysocki r...@rjwysocki.net 
 wrote:
  On Thursday, February 27, 2014 02:12:40 AM Kieran Clancy wrote:
  Signed-off-by: Kieran Clancy clancy.kie...@gmail.com
  Signed-off-by: Lan Tianyu tianyu@intel.com
  Signed-off-by: Juan Manuel Cabo juanmanuel.c...@gmail.com
  Signed-off-by: Dennis Jansen dennis.jan...@web.de
 
  There are too many sign-offs under this patch.  I suppose some of them
  should be Acked-by or Reviewed-by.
 
  Are you the author?
 
 Sorry about that, I wasn't sure of the best way to acknowledge the
 people involved.
 
 I am the primary author, but I based the loop which calls
 acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
 initial idea (userspace workaround), and Dennis and Lan Tianyu
 reviewed and suggested some changes to the code.
 
 What would you usually do this kind of situation?

I usually add acknowledgements to the changelog without using any tags,
like This patch is based on previous work by somebody.  And then you
can give a link to that work in References:.

 I suppose I could put something such as:

 Signed-off-by: Kieran Clancy clancy.kie...@gmail.com
 Signed-off-by: Lan Tianyu tianyu@intel.com
 Suggested-by: Juan Manuel Cabo juanmanuel.c...@gmail.com
 Reviewed-by: Lan Tianyu tianyu@intel.com
 Reviewed-by: Dennis Jansen dennis.jan...@web.de
 
 Would this be preferable?

Yes, that'd be fine by me.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Rafael J. Wysocki 
On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:
 On Thu, Feb 27, 2014 at 11:15 AM, Kieran Clancy clancy.kie...@gmail.com 
 wrote:
 
  I am the primary author, but I based the loop which calls
  acpi_ec_query_unlocked() on a patch by Lan Tianyu. Juan provided the
  initial idea (userspace workaround), and Dennis and Lan Tianyu
  reviewed and suggested some changes to the code.
 
 Oh, and I should add that Juan and Dennis both helped to test the
 patch extensively.

You can use Tested-by or Reported-and-tested-by for that.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 11:46 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:
 On Thursday, February 27, 2014 11:29:53 AM Kieran Clancy wrote:

 Oh, and I should add that Juan and Dennis both helped to test the
 patch extensively.

 You can use Tested-by or Reported-and-tested-by for that.

Is it okay to have someone listed as both 'Reviewed-by' and
'Tested-by', or both 'Suggested-by' and 'Tested-by'?

Thank you for your help. Once I fix up these things, I will send a patch v2.

Kieran.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Kieran Clancy wrote:

A number of Samsung notebooks (530Uxx/535Uxx/540Uxx/550Pxx/900Xxx/etc)
continue to log events during sleep (lid open/close, AC plug/unplug,
battery level change), which accumulate in the EC until a buffer fills.
After the buffer is full (tests suggest it holds 8 events), GPEs stop
being triggered for new events. This state persists on wake or even on
power cycle, and prevents new events from being registered until the EC
is manually polled.

   


that's really nasty EC firmware!


This is the root cause of a number of bugs, including AC not being
detected properly, lid close not triggering suspend, and low ambient
light not triggering the keyboard backlight. The bug also seemed to be
responsible for performance issues on at least one user's machine.

Juan Manuel Cabo found the cause of bug and the workaround of polling
the EC manually on wake.

This patch:
- Adds a function acpi_ec_clear() which polls the EC, at most
   ACPI_EC_CLEAR_MAX (currently 20) times. A warning is logged if this
   limit is reached.
- Adds a flag EC_FLAGS_CLEAR_ON_RESUME which is set to 1 if the DMI
   system vendor is Samsung. This check could be replaced by several more
   specific DMI vendor/product pairs, but it's likely that the bug
   affects more Samsung products than just the five series mentioned
   above. Further, it should not be harmful to run acpi_ec_clear() on
   systems without the bug; it will return immediately after finding no
   data waiting.
- Runs acpi_ec_clear() on initialisation (boot), from acpi_ec_add()
- Runs acpi_ec_clear() on wake, from acpi_ec_unblock_transactions()

References: https://bugzilla.kernel.org/show_bug.cgi?id=44161
References: https://bugzilla.kernel.org/show_bug.cgi?id=45461
References: https://bugzilla.kernel.org/show_bug.cgi?id=57271
Signed-off-by: Kieran Clancyclancy.kie...@gmail.com
Signed-off-by: Lan Tianyutianyu@intel.com
Signed-off-by: Juan Manuel Cabojuanmanuel.c...@gmail.com
Signed-off-by: Dennis Jansendennis.jan...@web.de
Tested-by: Maurizio D'Addonamauritiusd...@gmail.com
Tested-by: San Zamoyskis...@plusnet.pl
---
  drivers/acpi/ec.c | 61 +++
  1 file changed, 61 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 959d41a..f437d9a 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -67,6 +67,8 @@ enum ec_command {
  #define ACPI_EC_DELAY 500 /* Wait 500ms max. during EC ops */
  #define ACPI_EC_UDELAY_GLK1000/* Wait 1ms max. to get global lock */
  #define ACPI_EC_MSI_UDELAY550 /* Wait 550us for MSI EC */
+#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to query
+* when trying to clear the EC */
   


20 is enough?
the query index is length of a byte.



  enum {
EC_FLAGS_QUERY_PENDING, /* Query is pending */
@@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
  static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
  static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
  static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan */
+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */
   


seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)



  /* --
   Transaction Management
@@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

  EXPORT_SYMBOL(ec_get_handle);

+static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
+
+/* run with locked ec mutex */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+   int i, status;
+   u8 value = 0;
+
+   for (i = 0; i  ACPI_EC_CLEAR_MAX; i++) {
+   status = acpi_ec_query_unlocked(ec,value);
+   if (status || !value)
+   break;
+   }
+
+   if (i == ACPI_EC_CLEAR_MAX)
+   pr_warn(Warning: Maximum of %d stale EC events cleared\n, i);
+   else
+   pr_info(%d stale EC events cleared\n, i);
+}
+
  void acpi_ec_block_transactions(void)
  {
struct acpi_ec *ec = first_ec;
@@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
mutex_lock(ec-mutex);
/* Allow transactions to be carried out again */
clear_bit(EC_FLAGS_BLOCKED,ec-flags);
+
+   if (EC_FLAGS_CLEAR_ON_RESUME)
+   acpi_ec_clear(ec);
+
mutex_unlock(ec-mutex);
  }

@@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

/* EC is fully operational, allow queries */
clear_bit(EC_FLAGS_QUERY_PENDING,ec-flags);
+
+   /* Some hardware may need the EC to be cleared before use */
   


description is implicit, should specify what we clear is Q event, not EC.

Thanks!
Li Guang


+   if (EC_FLAGS_CLEAR_ON_RESUME) {
+   mutex_lock(ec-mutex);
+   acpi_ec_clear(ec);
+

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Juan Manuel Cabo 
 
 that's really nasty EC firmware!

Yes!
And this bug has been unsolved for about two years.

 20 is enough?
 the query index is length of a byte.

According to our humble tests, 8 is the maximum number of
accumulated events. For instance, if the one plugs or unplugs the PSU
16 times, (or battery % changes 16 times) during S3 sleep, then the
EC returns no more than 8 events when polled by this patch or by
my userspace util.
And having reached 8 events, it won't produce its GPE until queried.

 the query index is length of a byte.

There is no query index, unless you refer to something else (I'm sorry if its
something that escapes me).
For us, a query is just: send 0x84 through EC CMD port, and read status
from CMD port and event type from EC DATA port. This is done with
the usual ec.c functions that would handle a query after a GPE interrupt,
but using them instead to poll (not GPE initiated) at awake. The EC would
then return status without 0x20 mask and 'event type'==0 when no more left.

--
Juan Manuel Cabo juanmanuel.c...@gmail.com




   enum {
   EC_FLAGS_QUERY_PENDING,/* Query is pending */
 @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
   static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
   static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
   static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT 
 scan */
 +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume 
 */


 seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
 seems too long :-)


   /* 
 --
Transaction Management
 @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

   EXPORT_SYMBOL(ec_get_handle);

 +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
 +
 +/* run with locked ec mutex */
 +static void acpi_ec_clear(struct acpi_ec *ec)
 +{
 +int i, status;
 +u8 value = 0;
 +
 +for (i = 0; i  ACPI_EC_CLEAR_MAX; i++) {
 +status = acpi_ec_query_unlocked(ec,value);
 +if (status || !value)
 +break;
 +}
 +
 +if (i == ACPI_EC_CLEAR_MAX)
 +pr_warn(Warning: Maximum of %d stale EC events cleared\n, i);
 +else
 +pr_info(%d stale EC events cleared\n, i);
 +}
 +
   void acpi_ec_block_transactions(void)
   {
   struct acpi_ec *ec = first_ec;
 @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
   mutex_lock(ec-mutex);
   /* Allow transactions to be carried out again */
   clear_bit(EC_FLAGS_BLOCKED,ec-flags);
 +
 +if (EC_FLAGS_CLEAR_ON_RESUME)
 +acpi_ec_clear(ec);
 +
   mutex_unlock(ec-mutex);
   }

 @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

   /* EC is fully operational, allow queries */
   clear_bit(EC_FLAGS_QUERY_PENDING,ec-flags);
 +
 +/* Some hardware may need the EC to be cleared before use */


 description is implicit, should specify what we clear is Q event, not EC.

 Thanks!
 Li Guang

 +if (EC_FLAGS_CLEAR_ON_RESUME) {
 +mutex_lock(ec-mutex);
 +acpi_ec_clear(ec);
 +mutex_unlock(ec-mutex);
 +}
   return ret;
   }

 @@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
 dmi_system_id *id)
   return 0;
   }

 +/*
 + * On some hardware it is necessary to clear events accumulated by the EC 
 during
 + * sleep. These ECs stop reporting GPEs until they are manually polled, if 
 too
 + * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
 + *
 + * https://bugzilla.kernel.org/show_bug.cgi?id=44161
 + *
 + * Ideally, the EC should also be instructed not to accumulate events during
 + * sleep (which Windows seems to do somehow), but the interface to control 
 this
 + * behaviour is not known at this time.
 + *
 + * Models known to be affected are Samsung 
 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
 + * however it is very likely that other Samsung models are affected.
 + *
 + * On systems which don't accumulate EC events during sleep, this extra 
 check
 + * should be harmless.
 + */
 +static int ec_clear_on_resume(const struct dmi_system_id *id)
 +{
 +pr_debug(Detected system needing EC poll on resume.\n);
 +EC_FLAGS_CLEAR_ON_RESUME = 1;
 +return 0;
 +}
 +
   static struct dmi_system_id ec_dmi_table[] __initdata = {
   {
   ec_skip_dsdt_scan, Compal JFL92, {
 @@ -965,6 +1023,9 @@ static struct dmi_system_id ec_dmi_table[] __initdata = 
 {
   ec_validate_ecdt, ASUS hardware, {
   DMI_MATCH(DMI_SYS_VENDOR, ASUSTek Computer Inc.),
   DMI_MATCH(DMI_PRODUCT_NAME, L4R),}, NULL},
 +{
 +ec_clear_on_resume, Samsung hardware, {
 +DMI_MATCH(DMI_SYS_VENDOR, SAMSUNG ELECTRONICS CO., LTD.)}, NULL},
   {},
   };





--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Juan Manuel Cabo wrote:


   

that's really nasty EC firmware!
 

Yes!
And this bug has been unsolved for about two years.

   

20 is enough?
the query index is length of a byte.

 

According to our humble tests, 8 is the maximum number of
accumulated events. For instance, if the one plugs or unplugs the PSU
16 times, (or battery % changes 16 times) during S3 sleep, then the
EC returns no more than 8 events when polled by this patch or by
my userspace util.
And having reached 8 events, it won't produce its GPE until queried.
   


that surely indicts their EC firmware only queued 8 events,

   

the query index is length of a byte.
 

There is no query index, unless you refer to something else (I'm sorry if its
something that escapes me).
   


oh, sorry, I'm referring internal EC firmware code
for Q event queuing, not ACPI SPEC, ;-)
for machine you tested, 8 is the queue size,
but for some unknown also nasty EC firmwares(let's suppose it exists),
it may queue more Q events.
and I saw several firmwares queued 32 events by default,
then, let's say, they be used for some samsung products,
and also they also forgot to deal with sleep/resume state,
then, we'll also leave stale Q event there.

Thanks!


 For us, a query is just: send 0x84 through EC CMD port, and read status
from CMD port and event type from EC DATA port. This is done with
the usual ec.c functions that would handle a query after a GPE interrupt,
but using them instead to poll (not GPE initiated) at awake. The EC would
then return status without 0x20 mask and 'event type'==0 when no more left.

--
Juan Manuel Cabojuanmanuel.c...@gmail.com



   

   enum {
   EC_FLAGS_QUERY_PENDING,/* Query is pending */
@@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
   static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
   static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be validated */
   static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT scan 
*/
+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on boot/resume */

   

seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)

 

   /* --
Transaction Management
@@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

   EXPORT_SYMBOL(ec_get_handle);

+static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
+
+/* run with locked ec mutex */
+static void acpi_ec_clear(struct acpi_ec *ec)
+{
+int i, status;
+u8 value = 0;
+
+for (i = 0; i   ACPI_EC_CLEAR_MAX; i++) {
+status = acpi_ec_query_unlocked(ec,value);
+if (status || !value)
+break;
+}
+
+if (i == ACPI_EC_CLEAR_MAX)
+pr_warn(Warning: Maximum of %d stale EC events cleared\n, i);
+else
+pr_info(%d stale EC events cleared\n, i);
+}
+
   void acpi_ec_block_transactions(void)
   {
   struct acpi_ec *ec = first_ec;
@@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
   mutex_lock(ec-mutex);
   /* Allow transactions to be carried out again */
   clear_bit(EC_FLAGS_BLOCKED,ec-flags);
+
+if (EC_FLAGS_CLEAR_ON_RESUME)
+acpi_ec_clear(ec);
+
   mutex_unlock(ec-mutex);
   }

@@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

   /* EC is fully operational, allow queries */
   clear_bit(EC_FLAGS_QUERY_PENDING,ec-flags);
+
+/* Some hardware may need the EC to be cleared before use */

   

description is implicit, should specify what we clear is Q event, not EC.

Thanks!
Li Guang

 

+if (EC_FLAGS_CLEAR_ON_RESUME) {
+mutex_lock(ec-mutex);
+acpi_ec_clear(ec);
+mutex_unlock(ec-mutex);
+}
   return ret;
   }

@@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
dmi_system_id *id)
   return 0;
   }

+/*
+ * On some hardware it is necessary to clear events accumulated by the EC 
during
+ * sleep. These ECs stop reporting GPEs until they are manually polled, if too
+ * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
+ *
+ * https://bugzilla.kernel.org/show_bug.cgi?id=44161
+ *
+ * Ideally, the EC should also be instructed not to accumulate events during
+ * sleep (which Windows seems to do somehow), but the interface to control this
+ * behaviour is not known at this time.
+ *
+ * Models known to be affected are Samsung 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
+ * however it is very likely that other Samsung models are affected.
+ *
+ * On systems which don't accumulate EC events during sleep, this extra check
+ * should be harmless.
+ */
+static int ec_clear_on_resume(const struct dmi_system_id *id)
+{
+pr_debug(Detected system needing EC poll on resume.\n);
+EC_FLAGS_CLEAR_ON_RESUME = 1;
+return 0;
+}
+
   static struct dmi_system_id ec_dmi_table[] __initdata = {
   {
   ec_skip_dsdt_scan, Compal JFL92, {
@@ -965,6

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Juan Manuel Cabo 
On 02/27/2014 12:14 AM, Li Guang wrote:
 oh, sorry, I'm referring internal EC firmware code
 for Q event queuing, not ACPI SPEC, ;-)
 for machine you tested, 8 is the queue size,
 but for some unknown also nasty EC firmwares(let's suppose it exists),
 it may queue more Q events.
 and I saw several firmwares queued 32 events by default,
 then, let's say, they be used for some samsung products,
 and also they also forgot to deal with sleep/resume state,
 then, we'll also leave stale Q event there.

 Thanks!


We tested each on our different samsung models (intel, amd), and it
was 8 across. But you're right, there might be more in the future.

 I even saw a bug report in ubuntu's launchpad of an HP with a similar
sounding problem, ( 
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.20/+bug/89860 )
which I have no idea if it was caused by the same issue, but if in the future,
the flag ec_clear_on_resume is used to match other DMI's, it might
be a good idea to make the max iteration count bigger.

  The only reason that there is a max iteration count, was to prevent
an unexpected case in which an unknown EC never returns 0 after
queue emptied. So far it hasn't been the case. Can we count on it?.
The loop currently does finish early when there are no more events.

I guess changing it 255 or 1000 would be enough, right?

Cheers!
-- 
Juan Manuel Cabojuanmanuel.c...@gmail.com



  For us, a query is just: send 0x84 through EC CMD port, and read status
 from CMD port and event type from EC DATA port. This is done with
 the usual ec.c functions that would handle a query after a GPE interrupt,
 but using them instead to poll (not GPE initiated) at awake. The EC would
 then return status without 0x20 mask and 'event type'==0 when no more left.

 -- 
 Juan Manuel Cabojuanmanuel.c...@gmail.com



   
enum {
EC_FLAGS_QUERY_PENDING,/* Query is pending */
 @@ -116,6 +118,7 @@ EXPORT_SYMBOL(first_ec);
static int EC_FLAGS_MSI; /* Out-of-spec MSI controller */
static int EC_FLAGS_VALIDATE_ECDT; /* ASUStec ECDTs need to be 
 validated */
static int EC_FLAGS_SKIP_DSDT_SCAN; /* Not all BIOS survive early DSDT 
 scan */
 +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on 
 boot/resume */


 seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
 seems too long :-)

 
/* 
 --
 Transaction Management
 @@ -440,6 +443,26 @@ acpi_handle ec_get_handle(void)

EXPORT_SYMBOL(ec_get_handle);

 +static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data);
 +
 +/* run with locked ec mutex */
 +static void acpi_ec_clear(struct acpi_ec *ec)
 +{
 +int i, status;
 +u8 value = 0;
 +
 +for (i = 0; i   ACPI_EC_CLEAR_MAX; i++) {
 +status = acpi_ec_query_unlocked(ec,value);
 +if (status || !value)
 +break;
 +}
 +
 +if (i == ACPI_EC_CLEAR_MAX)
 +pr_warn(Warning: Maximum of %d stale EC events cleared\n, i);
 +else
 +pr_info(%d stale EC events cleared\n, i);
 +}
 +
void acpi_ec_block_transactions(void)
{
struct acpi_ec *ec = first_ec;
 @@ -463,6 +486,10 @@ void acpi_ec_unblock_transactions(void)
mutex_lock(ec-mutex);
/* Allow transactions to be carried out again */
clear_bit(EC_FLAGS_BLOCKED,ec-flags);
 +
 +if (EC_FLAGS_CLEAR_ON_RESUME)
 +acpi_ec_clear(ec);
 +
mutex_unlock(ec-mutex);
}

 @@ -821,6 +848,13 @@ static int acpi_ec_add(struct acpi_device *device)

/* EC is fully operational, allow queries */
clear_bit(EC_FLAGS_QUERY_PENDING,ec-flags);
 +
 +/* Some hardware may need the EC to be cleared before use */


 description is implicit, should specify what we clear is Q event, not EC.

 Thanks!
 Li Guang

 
 +if (EC_FLAGS_CLEAR_ON_RESUME) {
 +mutex_lock(ec-mutex);
 +acpi_ec_clear(ec);
 +mutex_unlock(ec-mutex);
 +}
return ret;
}

 @@ -922,6 +956,30 @@ static int ec_enlarge_storm_threshold(const struct 
 dmi_system_id *id)
return 0;
}

 +/*
 + * On some hardware it is necessary to clear events accumulated by the EC 
 during
 + * sleep. These ECs stop reporting GPEs until they are manually polled, 
 if too
 + * many events are accumulated. (e.g. Samsung Series 5/9 notebooks)
 + *
 + * https://bugzilla.kernel.org/show_bug.cgi?id=44161
 + *
 + * Ideally, the EC should also be instructed not to accumulate events 
 during
 + * sleep (which Windows seems to do somehow), but the interface to 
 control this
 + * behaviour is not known at this time.
 + *
 + * Models known to be affected are Samsung 
 530Uxx/535Uxx/540Uxx/550Pxx/900Xxx,
 + * however it is very likely that other Samsung models are affected.
 + *
 + * On systems which don't accumulate EC events during sleep, this extra 
 check
 + * should be harmless.
 + */
 +static int

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Juan Manuel Cabo wrote:

On 02/27/2014 12:14 AM, Li Guang wrote:
   

oh, sorry, I'm referring internal EC firmware code
for Q event queuing, not ACPI SPEC, ;-)
for machine you tested, 8 is the queue size,
but for some unknown also nasty EC firmwares(let's suppose it exists),
it may queue more Q events.
and I saw several firmwares queued 32 events by default,
then, let's say, they be used for some samsung products,
and also they also forgot to deal with sleep/resume state,
then, we'll also leave stale Q event there.

Thanks!

 

We tested each on our different samsung models (intel, amd), and it
was 8 across. But you're right, there might be more in the future.

  I even saw a bug report in ubuntu's launchpad of an HP with a similar
sounding problem, ( 
https://bugs.launchpad.net/ubuntu/+source/linux-source-2.6.20/+bug/89860 )
which I have no idea if it was caused by the same issue, but if in the future,
the flag ec_clear_on_resume is used to match other DMI's, it might
be a good idea to make the max iteration count bigger.

   The only reason that there is a max iteration count, was to prevent
an unexpected case in which an unknown EC never returns 0 after
queue emptied. So far it hasn't been the case. Can we count on it?.
The loop currently does finish early when there are no more events.

I guess changing it 255 or 1000 would be enough, right?

   


can't imagine 1K bytes be dissipated on Q event,
EC's ram is usually expensive,
I think 255 is really enough. :-)

Thanks!



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Kieran Clancy 
On Thu, Feb 27, 2014 at 12:29 PM, Li Guang lig.f...@cn.fujitsu.com wrote:
 +#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to
 query
 +* when trying to clear the EC */



 20 is enough?
 the query index is length of a byte.

On my machine, 8 seems to be enough, so 20 seems to be a conservative
maximum. Just reading your other email, maybe we should set this to
32? or 40? 100?

If it's not enough, hopefully anyone seeing bugs will notice the
warning maximum of X stale EC events cleared.

Here's what happens if I plug/replug the AC lots of times (more than
8) during suspend:

[ 8807.019800] ACPI : EC: --- status = 0x29
[ 8807.019804] ACPI : EC: --- data = 0x66
[ 8807.020790] ACPI : EC: --- status = 0x29
[ 8807.020793] ACPI : EC: --- data = 0x66
[ 8807.021793] ACPI : EC: --- status = 0x29
[ 8807.021798] ACPI : EC: --- data = 0x66
[ 8807.022831] ACPI : EC: --- status = 0x29
[ 8807.022834] ACPI : EC: --- data = 0x66
[ 8807.023788] ACPI : EC: --- status = 0x29
[ 8807.023792] ACPI : EC: --- data = 0x66
[ 8807.024787] ACPI : EC: --- status = 0x29
[ 8807.024791] ACPI : EC: --- data = 0x66
[ 8807.025787] ACPI : EC: --- status = 0x29
[ 8807.025790] ACPI : EC: --- data = 0x66
[ 8807.026787] ACPI : EC: --- status = 0x29
[ 8807.026790] ACPI : EC: --- data = 0x66
[ 8807.027786] ACPI : EC: --- status = 0x09
[ 8807.027790] ACPI : EC: --- data = 0x00
[ 8807.027792] ACPI : EC: 8 stale EC events cleared

Note that most of these have SCI_EVT set, but the OS is not notified
according to ACPI specs (seemingly because these events happened
during sleep).

The _Q66 method in my DSDT, is:

P8XH (Zero, 0x66)
If (LEqual (B1EX, One))
{
Notify (BAT1, 0x80)
}

So, basically, this is supposed to notify that the battery (BAT1 =
PNP0C0A) has changed state, but they are stale events so we don't run
the handlers.

 +static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on
 boot/resume */

 seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
 seems too long :-)

In my mind this is referring to the function name (acpi_ec_)clear.
Perhaps we could just make the connection more explicit in the
comment:

/* needs acpi_ec_clear() on boot/resume */

Not sure if this is better?

 +   /* Some hardware may need the EC to be cleared before use */

 description is implicit, should specify what we clear is Q event, not EC.

Are Q events the only thing we can get from the EC data port? I've
read the relevant parts of the ACPI spec and I can't say I am 100%
sure.

Thank you for your advice,
Kieran.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / EC: Clear stale EC events on Samsung systems

2014-02-26 Thread Li Guang 

Kieran Clancy wrote:

On Thu, Feb 27, 2014 at 12:29 PM, Li Guanglig.f...@cn.fujitsu.com  wrote:
   

+#define ACPI_EC_CLEAR_MAX  20  /* Maximum number of events to
query
+* when trying to clear the EC */

   


20 is enough?
the query index is length of a byte.
 

On my machine, 8 seems to be enough, so 20 seems to be a conservative
maximum. Just reading your other email, maybe we should set this to
32? or 40? 100?

If it's not enough, hopefully anyone seeing bugs will notice the
warning maximum of X stale EC events cleared.

Here's what happens if I plug/replug the AC lots of times (more than
8) during suspend:

[ 8807.019800] ACPI : EC: ---  status = 0x29
[ 8807.019804] ACPI : EC: ---  data = 0x66
[ 8807.020790] ACPI : EC: ---  status = 0x29
[ 8807.020793] ACPI : EC: ---  data = 0x66
[ 8807.021793] ACPI : EC: ---  status = 0x29
[ 8807.021798] ACPI : EC: ---  data = 0x66
[ 8807.022831] ACPI : EC: ---  status = 0x29
[ 8807.022834] ACPI : EC: ---  data = 0x66
[ 8807.023788] ACPI : EC: ---  status = 0x29
[ 8807.023792] ACPI : EC: ---  data = 0x66
[ 8807.024787] ACPI : EC: ---  status = 0x29
[ 8807.024791] ACPI : EC: ---  data = 0x66
[ 8807.025787] ACPI : EC: ---  status = 0x29
[ 8807.025790] ACPI : EC: ---  data = 0x66
[ 8807.026787] ACPI : EC: ---  status = 0x29
[ 8807.026790] ACPI : EC: ---  data = 0x66
[ 8807.027786] ACPI : EC: ---  status = 0x09
[ 8807.027790] ACPI : EC: ---  data = 0x00
[ 8807.027792] ACPI : EC: 8 stale EC events cleared

Note that most of these have SCI_EVT set, but the OS is not notified
according to ACPI specs (seemingly because these events happened
during sleep).

The _Q66 method in my DSDT, is:

 P8XH (Zero, 0x66)
 If (LEqual (B1EX, One))
 {
 Notify (BAT1, 0x80)
 }

So, basically, this is supposed to notify that the battery (BAT1 =
PNP0C0A) has changed state, but they are stale events so we don't run
the handlers.

   

+static int EC_FLAGS_CLEAR_ON_RESUME; /* EC should be polled on
boot/resume */
   

seems name is implicit, what about EC_FLAGS_QEVENT_CLR_ON_RESUME?
seems too long :-)
 

In my mind this is referring to the function name (acpi_ec_)clear.
Perhaps we could just make the connection more explicit in the
comment:

/* needs acpi_ec_clear() on boot/resume */

Not sure if this is better?

   

+   /* Some hardware may need the EC to be cleared before use */
   

description is implicit, should specify what we clear is Q event, not EC.
 

Are Q events the only thing we can get from the EC data port? I've
read the relevant parts of the ACPI spec and I can't say I am 100%
sure.

   

I guess you want to clear Q events here,
EC usually has ACPI space to be read by cmd 80.

Thanks!



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/