ACPI: EC: Don't do transaction from GPE handler in poll mode.

PATCH In review Alexey Starikovskiy 3 participants 6 messages Nov 11, 2008 1 message not in the archive ACPI

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20081111080614.11705.79007.stgit@thinkpad

Alexey Starikovskiy

author

2008-11-11 08:09 UTC #1 lore raw

PATCH 1/1 ACPI: EC: Don't do transaction from GPE handler in poll mode.

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/ec.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

drivers/acpi/ec.c +8 -3

index 628b076..b1dcc08 100644
@@ -576,11 +576,15 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 
-	gpe_transaction(ec, status);
-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}
 
 	ec_check_sci(ec, status);
+#if 0
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */

@@ -589,6 +593,7 @@ static u32 acpi_ec_gpe_handler(void *data)
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 	}
+#endif
 	return ACPI_INTERRUPT_HANDLED;
 }

Zhao Yakui

2008-11-11 08:52 UTC #2 lore raw

Quoted 17 lines · Alexey Starikovskiy

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004
Signed-off-by Alexey Starikovskiy <[email protected]>
---
 drivers/acpi/ec.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/acpi/ec.c +0 -0
index 628b076..b1dcc08 100644
@@ -576,11 +576,15 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 

drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 585

Quoted 10 lines

drivers/acpi/ec.c +6 -3

-	gpe_transaction(ec, status);
-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}

It seems that the EC won't be switched to EC GPE mode in the EC GPE handler again after the following is added.

drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 587

Quoted 2 lines

drivers/acpi/ec.c +0 -0
 #if 0
 #endif

Is it appropriate?

If what I said is incorrect, please ignore it.
Thanks.

Quoted 18 lines

drivers/acpi/ec.c +1 -0
 	ec_check_sci(ec, status);
+#if 0
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
drivers/acpi/ec.c +1 -1
 	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */
@@ -589,6 +593,7 @@ static u32 acpi_ec_gpe_handler(void *data)
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 	}
+#endif
 	return ACPI_INTERRUPT_HANDLED;
 }
 
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Zhao Yakui

2008-11-11 09:18 UTC #3 lore raw

In reply to Alexey Starikovskiy

Quoted 18 lines · Alexey Starikovskiy

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/ec.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

drivers/acpi/ec.c +0 -1

index 628b076..b1dcc08 100644
@@ -576,11 +576,15 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 
-	gpe_transaction(ec, status);

drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 586

Quoted 10 lines

drivers/acpi/ec.c +6 -2

-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}
 
 	ec_check_sci(ec, status);

Sorry that I don't describe it very clearly in the previous email. It seems that the EC won't be switched from polling mode to EC GPE mode after the following is added.

drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 588

Quoted 2 lines

drivers/acpi/ec.c +0 -0
 #if 0
 #endif

In such case the EC will always work in polling mode. And EC GPE
storm also can't be detected.

I am not sure what I have said is correct. If not correct, please ignore it.
If correct, is it appropriate that the default work mode is set as EC GPE mode?
Thanks.
Yakui

Quoted 17 lines

drivers/acpi/ec.c +1 -0
+#if 0
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
drivers/acpi/ec.c +1 -1
 	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */
@@ -589,6 +593,7 @@ static u32 acpi_ec_gpe_handler(void *data)
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 	}
+#endif
 	return ACPI_INTERRUPT_HANDLED;
 }
 
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

author

2008-11-11 09:50 UTC #4 lore raw

Yakui,
Thanks, I've sent my development patch -- I disabled switch to interrupt mode in order to test poll mode behaviour in presence of interrupts.

Proper patch will not include "#if 0", only if () around gpe_transaction().

Regards,
Alex.

Quoted 62 lines · Zhao Yakui

Quoted 18 lines · Alexey Starikovskiy
Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004
Signed-off-by Alexey Starikovskiy <[email protected]>
---
 drivers/acpi/ec.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)
drivers/acpi/ec.c +0 -1
index 628b076..b1dcc08 100644
@@ -576,11 +576,15 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 
-	gpe_transaction(ec, status);
drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 586
Quoted 10 lines
drivers/acpi/ec.c +6 -2
-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}
 
 	ec_check_sci(ec, status);
Sorry that I don't describe it very clearly in the previous email. It seems that the EC won't be switched from polling mode to EC GPE mode after the following is added.
drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(void *data) · line 588
Quoted 2 lines
drivers/acpi/ec.c +0 -0
 #if 0
 #endif
In such case the EC will always work in polling mode. And EC GPE
storm also can't be detected.
I am not sure what I have said is correct. If not correct, please ignore it.
If correct, is it appropriate that the default work mode is set as EC GPE mode?
Thanks.
Yakui
Quoted 17 lines
drivers/acpi/ec.c +1 -0
+#if 0
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
drivers/acpi/ec.c +1 -1
 	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */
@@ -589,6 +593,7 @@ static u32 acpi_ec_gpe_handler(void *data)
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 	}
+#endif
 	return ACPI_INTERRUPT_HANDLED;
 }
 
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Starikovskiy

author

2008-11-11 09:54 UTC #5 lore raw

PATCH 1/1 ACPI: EC: Don't do transaction from GPE handler in poll mode.

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/ec.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

drivers/acpi/ec.c +6 -3

index 628b076..985fb50 100644
@@ -576,9 +576,12 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 
-	gpe_transaction(ec, status);
-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}
 
 	ec_check_sci(ec, status);
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&

Len Brown

reviewer Applied

2008-11-11 22:22 UTC #6 lore raw

applied.

thanks,
-len

Quoted 30 lines · Alexey Starikovskiy

Referencies: http://bugzilla.kernel.org/show_bug.cgi?id=12004

Signed-off-by Alexey Starikovskiy <[email protected]>

---

 drivers/acpi/ec.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

drivers/acpi/ec.c +6 -3

index 628b076..985fb50 100644
@@ -576,9 +576,12 @@ static u32 acpi_ec_gpe_handler(void *data)
 	pr_debug(PREFIX "~~~> interrupt\n");
 	status = acpi_ec_read_status(ec);
 
-	gpe_transaction(ec, status);
-	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
-		wake_up(&ec->wait);
+	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) {
+		gpe_transaction(ec, status);
+		if (ec_transaction_done(ec) &&
+		    (status & ACPI_EC_FLAG_IBF) == 0)
+			wake_up(&ec->wait);
+	}
 
 	ec_check_sci(ec, status);
 	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&