linux-kernel - RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1AE640813FDE7649BE1B193DEA596E880265EEBB@SHSMSX101.ccr.corp.intel.com>
Date:	Fri, 22 Aug 2014 01:06:29 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Brown, Len" <len.brown@...el.com>
CC:	Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued
 before completing the previous QR_EC.

Hi, Rafael

I think PATCH 1 is required for now and this PATCH is not that useful if you are planning to merge my storming fix, the logic has already been cleaned there.

This flag originally is used to mark the period after SCI_EVT is indicated and before the QR_EC is polled.
This flag is useful because during this period some malfunctioning firmware may trigger GPE storm as we haven't handled SCI_EVT right in the IRQ context.
But so far we haven’t ACPICA GPE APIs ready to achieve such storm prevention, the original use case is not that obvious now….
Actually this flag is currently only used to masking query issuing...

This patch change the flag to be used for this case – marking the period after SCI_EVT is indicated and before the QR_EC is completed.
Since the original usage is actually not implemented, we can do this change.

But you also can ignore this patch as the cleanup in the storming series is cleaner than this.
In that series, the both cases are marked and thus can be protected by further code.

So it’s up to you whether you need PATCH 2 or not. ☺
If you took it, I would re-base the storming series to reflect this change.

Thanks and best regards
-Lv

From: Wysocki, Rafael J 
Sent: Friday, August 22, 2014 3:55 AM
To: Zheng, Lv; Brown, Len
Cc: Lv Zheng; linux-kernel@...r.kernel.org; linux-acpi@...r.kernel.org
Subject: RE: [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC.

Looks OK to me.

Rafael

-------- Original message --------
From "Zheng, Lv" <lv.zheng@...el.com> 
Date: 21/08/2014 08:41 (GMT+01:00) 
To "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,"Brown, Len" <len.brown@...el.com> 
Cc "Zheng, Lv" <lv.zheng@...el.com>,Lv Zheng <zetalog@...il.com>,linux-kernel@...r.kernel.org,linux-acpi@...r.kernel.org 
Subject [PATCH 2/2] ACPI/EC: Add support to disallow QR_EC to be issued before completing the previous QR_EC. 

There is platform refusing to respond QR_EC when SCI_EVT isn't set.
A known such platform is Acer Aspire V5-573G.

By disallowing QR_EC issuing without completing the previous one, we are
able to reduce the possibilities to trigger issues on such platforms.

Note that this fix can only reduce the occurrence rate of this issue, but
this issue may still occur when such a platform doesn't clear SCI_EVT
before or immediately after completing the previous QR_EC transaction. This
patch cannot fix CLEAR_ON_RESUME quirk which also relies on the assumption
that the platforms are able to respond even when SCI_EVT isn't set.

But this patch is still useful as it can help to reduce the number of
scheduled QR_EC work items.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=82611
Reported-and-tested-by: Alexander Mezin <mezin.alexander@...il.com>
Signed-off-by: Lv Zheng <lv.zheng@...el.com>
---
 drivers/acpi/ec.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5e1ed31..9922cc4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -303,11 +303,11 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
         /* following two actions should be kept atomic */
         ec->curr = t;
         start_transaction(ec);
-       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
-               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         spin_unlock_irqrestore(&ec->lock, tmp);
         ret = ec_poll(ec);
         spin_lock_irqsave(&ec->lock, tmp);
+       if (ec->curr->command == ACPI_EC_COMMAND_QUERY)
+               clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
         ec->curr = NULL;
         spin_unlock_irqrestore(&ec->lock, tmp);
         return ret;
-- 
1.7.10