All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] ACPI / EC: Fix several code coverity issues
@ 2015-09-24  6:54 ` Lv Zheng
  0 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

This patch fixes several code coverity issues in the EC driver.
One serious issue that can cause memory leak is marked as stable material.

Lv Zheng (3):
  ACPI / EC: Fix a memory leak issue in acpi_ec_query()
  ACPI / EC: Fix query handler related issues
  ACPI / EC: Fix a race issue in acpi_ec_guard_event()

 drivers/acpi/ec.c |  113 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 39 deletions(-)

-- 
1.7.10

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 0/3] ACPI / EC: Fix several code coverity issues
@ 2015-09-24  6:54 ` Lv Zheng
  0 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

This patch fixes several code coverity issues in the EC driver.
One serious issue that can cause memory leak is marked as stable material.

Lv Zheng (3):
  ACPI / EC: Fix a memory leak issue in acpi_ec_query()
  ACPI / EC: Fix query handler related issues
  ACPI / EC: Fix a race issue in acpi_ec_guard_event()

 drivers/acpi/ec.c |  113 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 74 insertions(+), 39 deletions(-)

-- 
1.7.10


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] ACPI / EC: Fix a memory leak issue in acpi_ec_query()
  2015-09-24  6:54 ` Lv Zheng
@ 2015-09-24  6:54   ` Lv Zheng
  -1 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

When query handler is not found, "result" is actually stil 0, and
"struct acpi_ec_query" is not NULL, so the deletion code of
"struct acpi_ec_query" at the end of the function cannot be invoked.
As a consequence, memory leak can be observed.

The issue is introduced by this commit:
  Commit: 02b771b64b73226052d6e731a0987db3b47281e9
  Subject: ACPI / EC: Fix an issue caused by the serialized _Qxx

This patch fixes such memory leakage.

Cc: stable@vger.kernel.org  # 4.3.1+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2614a83..42c66b6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1044,8 +1044,10 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 		goto err_exit;
 
 	mutex_lock(&ec->mutex);
+	result = -ENODATA;
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
+			result = 0;
 			q->handler = acpi_ec_get_query_handler(handler);
 			ec_dbg_evt("Query(0x%02x) scheduled",
 				   q->handler->query_bit);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 1/3] ACPI / EC: Fix a memory leak issue in acpi_ec_query()
@ 2015-09-24  6:54   ` Lv Zheng
  0 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

When query handler is not found, "result" is actually stil 0, and
"struct acpi_ec_query" is not NULL, so the deletion code of
"struct acpi_ec_query" at the end of the function cannot be invoked.
As a consequence, memory leak can be observed.

The issue is introduced by this commit:
  Commit: 02b771b64b73226052d6e731a0987db3b47281e9
  Subject: ACPI / EC: Fix an issue caused by the serialized _Qxx

This patch fixes such memory leakage.

Cc: stable@vger.kernel.org  # 4.3.1+
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2614a83..42c66b6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1044,8 +1044,10 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 		goto err_exit;
 
 	mutex_lock(&ec->mutex);
+	result = -ENODATA;
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
+			result = 0;
 			q->handler = acpi_ec_get_query_handler(handler);
 			ec_dbg_evt("Query(0x%02x) scheduled",
 				   q->handler->query_bit);
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] ACPI / EC: Fix query handler related issues
  2015-09-24  6:54 ` Lv Zheng
@ 2015-09-24  6:54   ` Lv Zheng
  -1 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

1. acpi_ec_remove_query_handlers()
This patch refines the query handler removal logic implemented in
acpi_ec_remove_query_handler(), making it to invoke new
acpi_ec_remove_query_handlers() API, and ensuring all other removal code
paths to invoke the new API to honor the reference count of the query
handlers.

2. acpi_ec_get_query_handler_by_value()
This patch also refines the query handler search logic originally
implemented in acpi_ec_query(), collecting it into
acpi_ec_get_query_handler_by_value(). And since schedule_work() can ensure
the serilization of acpi_ec_event_handler(), we needn't put the
mutex_lock() around schedule_work().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   73 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 42c66b6..ddaaa9d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -929,6 +929,23 @@ acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler)
 	return handler;
 }
 
+static struct acpi_ec_query_handler *
+acpi_ec_get_query_handler_by_value(struct acpi_ec *ec, u8 value)
+{
+	struct acpi_ec_query_handler *handler;
+	bool found = false;
+
+	mutex_lock(&ec->mutex);
+	list_for_each_entry(handler, &ec->list, node) {
+		if (value == handler->query_bit) {
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&ec->mutex);
+	return found ? acpi_ec_get_query_handler(handler) : NULL;
+}
+
 static void acpi_ec_query_handler_release(struct kref *kref)
 {
 	struct acpi_ec_query_handler *handler =
@@ -964,14 +981,15 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 }
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
-void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+static void acpi_ec_remove_query_handlers(struct acpi_ec *ec,
+					  bool remove_all, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
 	LIST_HEAD(free_list);
 
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		if (query_bit == handler->query_bit) {
+		if (remove_all || query_bit == handler->query_bit) {
 			list_del_init(&handler->node);
 			list_add(&handler->node, &free_list);
 		}
@@ -980,6 +998,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 	list_for_each_entry_safe(handler, tmp, &free_list, node)
 		acpi_ec_put_query_handler(handler);
 }
+
+void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+{
+	acpi_ec_remove_query_handlers(ec, false, query_bit);
+}
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
 static struct acpi_ec_query *acpi_ec_create_query(u8 *pval)
@@ -1025,7 +1048,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
-	struct acpi_ec_query_handler *handler;
 	struct acpi_ec_query *q;
 
 	q = acpi_ec_create_query(&value);
@@ -1043,25 +1065,26 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	if (result)
 		goto err_exit;
 
-	mutex_lock(&ec->mutex);
-	result = -ENODATA;
-	list_for_each_entry(handler, &ec->list, node) {
-		if (value == handler->query_bit) {
-			result = 0;
-			q->handler = acpi_ec_get_query_handler(handler);
-			ec_dbg_evt("Query(0x%02x) scheduled",
-				   q->handler->query_bit);
-			/*
-			 * It is reported that _Qxx are evaluated in a
-			 * parallel way on Windows:
-			 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
-			 */
-			if (!schedule_work(&q->work))
-				result = -EBUSY;
-			break;
-		}
+	q->handler = acpi_ec_get_query_handler_by_value(ec, value);
+	if (!q->handler) {
+		result = -ENODATA;
+		goto err_exit;
+	}
+
+	/*
+	 * It is reported that _Qxx are evaluated in a parallel way on
+	 * Windows:
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
+	 *
+	 * Put this log entry before schedule_work() in order to make
+	 * it appearing before any other log entries occurred during the
+	 * work queue execution.
+	 */
+	ec_dbg_evt("Query(0x%02x) scheduled", value);
+	if (!schedule_work(&q->work)) {
+		ec_dbg_evt("Query(0x%02x) overlapped", value);
+		result = -EBUSY;
 	}
-	mutex_unlock(&ec->mutex);
 
 err_exit:
 	if (result && q)
@@ -1354,19 +1377,13 @@ static int acpi_ec_add(struct acpi_device *device)
 static int acpi_ec_remove(struct acpi_device *device)
 {
 	struct acpi_ec *ec;
-	struct acpi_ec_query_handler *handler, *tmp;
 
 	if (!device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
 	ec_remove_handlers(ec);
-	mutex_lock(&ec->mutex);
-	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		list_del(&handler->node);
-		kfree(handler);
-	}
-	mutex_unlock(&ec->mutex);
+	acpi_ec_remove_query_handlers(ec, true, 0);
 	release_region(ec->data_addr, 1);
 	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] ACPI / EC: Fix query handler related issues
@ 2015-09-24  6:54   ` Lv Zheng
  0 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

1. acpi_ec_remove_query_handlers()
This patch refines the query handler removal logic implemented in
acpi_ec_remove_query_handler(), making it to invoke new
acpi_ec_remove_query_handlers() API, and ensuring all other removal code
paths to invoke the new API to honor the reference count of the query
handlers.

2. acpi_ec_get_query_handler_by_value()
This patch also refines the query handler search logic originally
implemented in acpi_ec_query(), collecting it into
acpi_ec_get_query_handler_by_value(). And since schedule_work() can ensure
the serilization of acpi_ec_event_handler(), we needn't put the
mutex_lock() around schedule_work().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   73 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 42c66b6..ddaaa9d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -929,6 +929,23 @@ acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler)
 	return handler;
 }
 
+static struct acpi_ec_query_handler *
+acpi_ec_get_query_handler_by_value(struct acpi_ec *ec, u8 value)
+{
+	struct acpi_ec_query_handler *handler;
+	bool found = false;
+
+	mutex_lock(&ec->mutex);
+	list_for_each_entry(handler, &ec->list, node) {
+		if (value == handler->query_bit) {
+			found = true;
+			break;
+		}
+	}
+	mutex_unlock(&ec->mutex);
+	return found ? acpi_ec_get_query_handler(handler) : NULL;
+}
+
 static void acpi_ec_query_handler_release(struct kref *kref)
 {
 	struct acpi_ec_query_handler *handler =
@@ -964,14 +981,15 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 }
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
-void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+static void acpi_ec_remove_query_handlers(struct acpi_ec *ec,
+					  bool remove_all, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
 	LIST_HEAD(free_list);
 
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		if (query_bit == handler->query_bit) {
+		if (remove_all || query_bit == handler->query_bit) {
 			list_del_init(&handler->node);
 			list_add(&handler->node, &free_list);
 		}
@@ -980,6 +998,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 	list_for_each_entry_safe(handler, tmp, &free_list, node)
 		acpi_ec_put_query_handler(handler);
 }
+
+void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
+{
+	acpi_ec_remove_query_handlers(ec, false, query_bit);
+}
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
 static struct acpi_ec_query *acpi_ec_create_query(u8 *pval)
@@ -1025,7 +1048,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
-	struct acpi_ec_query_handler *handler;
 	struct acpi_ec_query *q;
 
 	q = acpi_ec_create_query(&value);
@@ -1043,25 +1065,26 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 	if (result)
 		goto err_exit;
 
-	mutex_lock(&ec->mutex);
-	result = -ENODATA;
-	list_for_each_entry(handler, &ec->list, node) {
-		if (value == handler->query_bit) {
-			result = 0;
-			q->handler = acpi_ec_get_query_handler(handler);
-			ec_dbg_evt("Query(0x%02x) scheduled",
-				   q->handler->query_bit);
-			/*
-			 * It is reported that _Qxx are evaluated in a
-			 * parallel way on Windows:
-			 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
-			 */
-			if (!schedule_work(&q->work))
-				result = -EBUSY;
-			break;
-		}
+	q->handler = acpi_ec_get_query_handler_by_value(ec, value);
+	if (!q->handler) {
+		result = -ENODATA;
+		goto err_exit;
+	}
+
+	/*
+	 * It is reported that _Qxx are evaluated in a parallel way on
+	 * Windows:
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
+	 *
+	 * Put this log entry before schedule_work() in order to make
+	 * it appearing before any other log entries occurred during the
+	 * work queue execution.
+	 */
+	ec_dbg_evt("Query(0x%02x) scheduled", value);
+	if (!schedule_work(&q->work)) {
+		ec_dbg_evt("Query(0x%02x) overlapped", value);
+		result = -EBUSY;
 	}
-	mutex_unlock(&ec->mutex);
 
 err_exit:
 	if (result && q)
@@ -1354,19 +1377,13 @@ static int acpi_ec_add(struct acpi_device *device)
 static int acpi_ec_remove(struct acpi_device *device)
 {
 	struct acpi_ec *ec;
-	struct acpi_ec_query_handler *handler, *tmp;
 
 	if (!device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
 	ec_remove_handlers(ec);
-	mutex_lock(&ec->mutex);
-	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
-		list_del(&handler->node);
-		kfree(handler);
-	}
-	mutex_unlock(&ec->mutex);
+	acpi_ec_remove_query_handlers(ec, true, 0);
 	release_region(ec->data_addr, 1);
 	release_region(ec->command_addr, 1);
 	device->driver_data = NULL;
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ACPI / EC: Fix a race issue in acpi_ec_guard_event()
  2015-09-24  6:54 ` Lv Zheng
@ 2015-09-24  6:54   ` Lv Zheng
  -1 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

In acpi_ec_guard_event(), EC transaction state machine variables should be
checked with the EC spinlock locked.
The bug doesn't trigger any real issue now because this bug can only occur
when the ec_event_clearing=event mode is applied while there is no user
currently using this mode.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ddaaa9d..2687a01 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -441,17 +441,31 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
+	bool guarded = true;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	/*
+	 * If firmware SCI_EVT clearing timing is "event", we actually
+	 * don't know when the SCI_EVT will be cleared by firmware after
+	 * evaluating _Qxx, so we need to re-check SCI_EVT after waiting an
+	 * acceptable period.
+	 *
+	 * The guarding period begins when EC_FLAGS_QUERY_PENDING is
+	 * flagged, which means SCI_EVT check has just been performed.
+	 * But if the current transaction is ACPI_EC_COMMAND_QUERY, the
+	 * guarding should have already been performed (via
+	 * EC_FLAGS_QUERY_GUARDING) and should not be applied so that the
+	 * ACPI_EC_COMMAND_QUERY transaction can be transitioned into
+	 * ACPI_EC_COMMAND_POLL state immediately.
+	 */
 	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
 	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY ||
 	    !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) ||
 	    (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY))
-		return false;
-
-	/*
-	 * Postpone the query submission to allow the firmware to proceed,
-	 * we shouldn't check SCI_EVT before the firmware reflagging it.
-	 */
-	return true;
+		guarded = false;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	return guarded;
 }
 
 static int ec_transaction_polled(struct acpi_ec *ec)
@@ -597,6 +611,7 @@ static int ec_guard(struct acpi_ec *ec)
 	unsigned long guard = usecs_to_jiffies(ec_polling_guard);
 	unsigned long timeout = ec->timestamp + guard;
 
+	/* Ensure guarding period before polling EC status */
 	do {
 		if (ec_busy_polling) {
 			/* Perform busy polling */
@@ -606,11 +621,13 @@ static int ec_guard(struct acpi_ec *ec)
 		} else {
 			/*
 			 * Perform wait polling
-			 *
-			 * For SCI_EVT clearing timing of "event",
-			 * performing guarding before re-checking the
-			 * SCI_EVT. Otherwise, such guarding is not needed
-			 * due to the old practices.
+			 * 1. Wait the transaction to be completed by the
+			 *    GPE handler after the transaction enters
+			 *    ACPI_EC_COMMAND_POLL state.
+			 * 2. A special guarding logic is also required
+			 *    for event clearing mode "event" before the
+			 *    transaction enters ACPI_EC_COMMAND_POLL
+			 *    state.
 			 */
 			if (!ec_transaction_polled(ec) &&
 			    !acpi_ec_guard_event(ec))
@@ -620,7 +637,6 @@ static int ec_guard(struct acpi_ec *ec)
 					       guard))
 				return 0;
 		}
-		/* Guard the register accesses for the polling modes */
 	} while (time_before(jiffies, timeout));
 	return -ETIME;
 }
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ACPI / EC: Fix a race issue in acpi_ec_guard_event()
@ 2015-09-24  6:54   ` Lv Zheng
  0 siblings, 0 replies; 9+ messages in thread
From: Lv Zheng @ 2015-09-24  6:54 UTC (permalink / raw)
  To: Rafael J. Wysocki, Len Brown; +Cc: Lv Zheng, Lv Zheng, linux-kernel, linux-acpi

In acpi_ec_guard_event(), EC transaction state machine variables should be
checked with the EC spinlock locked.
The bug doesn't trigger any real issue now because this bug can only occur
when the ec_event_clearing=event mode is applied while there is no user
currently using this mode.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ddaaa9d..2687a01 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -441,17 +441,31 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
 
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
+	bool guarded = true;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ec->lock, flags);
+	/*
+	 * If firmware SCI_EVT clearing timing is "event", we actually
+	 * don't know when the SCI_EVT will be cleared by firmware after
+	 * evaluating _Qxx, so we need to re-check SCI_EVT after waiting an
+	 * acceptable period.
+	 *
+	 * The guarding period begins when EC_FLAGS_QUERY_PENDING is
+	 * flagged, which means SCI_EVT check has just been performed.
+	 * But if the current transaction is ACPI_EC_COMMAND_QUERY, the
+	 * guarding should have already been performed (via
+	 * EC_FLAGS_QUERY_GUARDING) and should not be applied so that the
+	 * ACPI_EC_COMMAND_QUERY transaction can be transitioned into
+	 * ACPI_EC_COMMAND_POLL state immediately.
+	 */
 	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
 	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY ||
 	    !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) ||
 	    (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY))
-		return false;
-
-	/*
-	 * Postpone the query submission to allow the firmware to proceed,
-	 * we shouldn't check SCI_EVT before the firmware reflagging it.
-	 */
-	return true;
+		guarded = false;
+	spin_unlock_irqrestore(&ec->lock, flags);
+	return guarded;
 }
 
 static int ec_transaction_polled(struct acpi_ec *ec)
@@ -597,6 +611,7 @@ static int ec_guard(struct acpi_ec *ec)
 	unsigned long guard = usecs_to_jiffies(ec_polling_guard);
 	unsigned long timeout = ec->timestamp + guard;
 
+	/* Ensure guarding period before polling EC status */
 	do {
 		if (ec_busy_polling) {
 			/* Perform busy polling */
@@ -606,11 +621,13 @@ static int ec_guard(struct acpi_ec *ec)
 		} else {
 			/*
 			 * Perform wait polling
-			 *
-			 * For SCI_EVT clearing timing of "event",
-			 * performing guarding before re-checking the
-			 * SCI_EVT. Otherwise, such guarding is not needed
-			 * due to the old practices.
+			 * 1. Wait the transaction to be completed by the
+			 *    GPE handler after the transaction enters
+			 *    ACPI_EC_COMMAND_POLL state.
+			 * 2. A special guarding logic is also required
+			 *    for event clearing mode "event" before the
+			 *    transaction enters ACPI_EC_COMMAND_POLL
+			 *    state.
 			 */
 			if (!ec_transaction_polled(ec) &&
 			    !acpi_ec_guard_event(ec))
@@ -620,7 +637,6 @@ static int ec_guard(struct acpi_ec *ec)
 					       guard))
 				return 0;
 		}
-		/* Guard the register accesses for the polling modes */
 	} while (time_before(jiffies, timeout));
 	return -ETIME;
 }
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/3] ACPI / EC: Fix several code coverity issues
  2015-09-24  6:54 ` Lv Zheng
                   ` (3 preceding siblings ...)
  (?)
@ 2015-10-05 20:38 ` Rafael J. Wysocki
  -1 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2015-10-05 20:38 UTC (permalink / raw)
  To: Lv Zheng; +Cc: Rafael J. Wysocki, Len Brown, Lv Zheng, linux-kernel, linux-acpi

On Thursday, September 24, 2015 02:54:26 PM Lv Zheng wrote:
> This patch fixes several code coverity issues in the EC driver.
> One serious issue that can cause memory leak is marked as stable material.
> 
> Lv Zheng (3):
>   ACPI / EC: Fix a memory leak issue in acpi_ec_query()
>   ACPI / EC: Fix query handler related issues
>   ACPI / EC: Fix a race issue in acpi_ec_guard_event()
> 
>  drivers/acpi/ec.c |  113 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 74 insertions(+), 39 deletions(-)

Series applied, thanks!

Rafael

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-10-05 20:38 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-24  6:54 [PATCH 0/3] ACPI / EC: Fix several code coverity issues Lv Zheng
2015-09-24  6:54 ` Lv Zheng
2015-09-24  6:54 ` [PATCH 1/3] ACPI / EC: Fix a memory leak issue in acpi_ec_query() Lv Zheng
2015-09-24  6:54   ` Lv Zheng
2015-09-24  6:54 ` [PATCH 2/3] ACPI / EC: Fix query handler related issues Lv Zheng
2015-09-24  6:54   ` Lv Zheng
2015-09-24  6:54 ` [PATCH 3/3] ACPI / EC: Fix a race issue in acpi_ec_guard_event() Lv Zheng
2015-09-24  6:54   ` Lv Zheng
2015-10-05 20:38 ` [PATCH 0/3] ACPI / EC: Fix several code coverity issues Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.