acpi-test tree on eeepc: EC error message on second resume

I've just run an acpi-test kernel on my EeePC and noticed a new issue. It seems to be caused (or revealed) by the EC interrupt transaction patch.

On the second suspend/resume cycle, I see a kernel error message.

[   78.747707] ACPI: Waking up from system sleep state S3
[   79.330001] ACPI: EC: input buffer not empty, aborting transaction
[   79.423327] ACPI: EC: non-query interrupt received, switching to

interrupt mode

I still don't see any issues in the code. I'll try getting a DEBUG trace to see the EC interrupts. Any other suggestions?

Quoted 5 lines · Alan Jenkins

I've just run an acpi-test kernel on my EeePC and noticed a new issue. It seems to be caused (or revealed) by the EC interrupt transaction patch.

On the second suspend/resume cycle, I see a kernel error message.

Quoted 8 lines

[   78.747707] ACPI: Waking up from system sleep state S3
[   79.330001] ACPI: EC: input buffer not empty, aborting transaction
[   79.423327] ACPI: EC: non-query interrupt received, switching to

interrupt mode

I still don't see any issues in the code. I'll try getting a DEBUG trace to see the EC interrupts. Any other suggestions?

Not really, but is this reproducible? I mean, does it happen always on the second resume and does it happen on every next resume after the first one?

Thanks,
Rafael

Quoted 16 lines · Rafael J. Wysocki

Quoted 5 lines · Alan Jenkins

I've just run an acpi-test kernel on my EeePC and noticed a new issue. It seems to be caused (or revealed) by the EC interrupt transaction patch.

On the second suspend/resume cycle, I see a kernel error message.

Quoted 8 lines

[   78.747707] ACPI: Waking up from system sleep state S3
[   79.330001] ACPI: EC: input buffer not empty, aborting transaction
[   79.423327] ACPI: EC: non-query interrupt received, switching to

interrupt mode

I still don't see any issues in the code. I'll try getting a DEBUG trace to see the EC interrupts. Any other suggestions?

Quoted 6 lines

Not really, but is this reproducible? I mean, does it happen always on the second resume and does it happen on every next resume after the first one?

Thanks,
Rafael

Ah. No, I spoke to soon. It happened on the second resume the first two times I tried it. But this third time with DEBUG enabled, it happened on the first suspend/resume.

And it doesn't happen on all subsequent resumes either. I've had one suspend/resume without the error, just after a suspend/resume with the error.

So it's not deterministic, but it is easy to reproduce.

Quoted 34 lines · Alan Jenkins

Quoted 17 lines · Rafael J. Wysocki

Quoted 5 lines · Alan Jenkins

I've just run an acpi-test kernel on my EeePC and noticed a new issue. It seems to be caused (or revealed) by the EC interrupt transaction patch.

On the second suspend/resume cycle, I see a kernel error message.

Quoted 9 lines

[   78.747707] ACPI: Waking up from system sleep state S3
[   79.330001] ACPI: EC: input buffer not empty, aborting transaction
[   79.423327] ACPI: EC: non-query interrupt received, switching to

interrupt mode

I still don't see any issues in the code. I'll try getting a DEBUG trace to see the EC interrupts. Any other suggestions?

Quoted 7 lines

Not really, but is this reproducible? I mean, does it happen always on the second resume and does it happen on every next resume after the first one?

Thanks,
Rafael

Ah. No, I spoke to soon. It happened on the second resume the first two times I tried it. But this third time with DEBUG enabled, it happened on the first suspend/resume.

And it doesn't happen on all subsequent resumes either. I've had one suspend/resume without the error, just after a suspend/resume with the error.

Quoted 2 lines

So it's not deterministic, but it is easy to reproduce.

I can't reproduce this on any hardware available to me, so far.

Is this related to any other problem, like things not working etc.?

I think I found the problem. The "input buffer empty" wait depends on "interrupt mode" to work properly, and we don't immediately enable the interrupt on resume. The wait should have a polling fallback anyway, to be consistent with the other transaction waits.

Alan

Quoted 6 lines · Alan Jenkins

I think I found the problem. The "input buffer empty" wait depends on "interrupt mode" to work properly, and we don't immediately enable the interrupt on resume. The wait should have a polling fallback anyway, to be consistent with the other transaction waits.

Alan

Yep, I think something like attached patch may help:

Quoted 7 lines · Alexey Starikovskiy

Quoted 5 lines · Alan Jenkins

I think I found the problem. The "input buffer empty" wait depends on "interrupt mode" to work properly, and we don't immediately enable the interrupt on resume. The wait should have a polling fallback anyway, to be consistent with the other transaction waits.

Quoted 4 lines

Quoted 2 lines

Alan

Yep, I think something like attached patch may help:

[Can you please append patches instead of or apart from attaching them? That would make it easier to comment them.]

if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),

drivers/acpi/ec.c +2 -1

-                               msecs_to_jiffies(ACPI_EC_DELAY))) {
+                               msecs_to_jiffies(ACPI_EC_DELAY)) &&
+           !ec_check_ibf0(ec)) {

Shouldn't this go under the spinlock? Surely it can race with the GPE handler.

Quoted 9 lines · Rafael J. Wysocki

Quoted 7 lines · Alexey Starikovskiy

Quoted 5 lines · Alan Jenkins

I think I found the problem. The "input buffer empty" wait depends on "interrupt mode" to work properly, and we don't immediately enable the interrupt on resume. The wait should have a polling fallback anyway, to be consistent with the other transaction waits.

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

Alan

Yep, I think something like attached patch may help:

[Can you please append patches instead of or apart from attaching them? That would make it easier to comment them.]

Ok.

Quoted 5 lines

if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),

drivers/acpi/ec.c +2 -1

-                               msecs_to_jiffies(ACPI_EC_DELAY))) {
+                               msecs_to_jiffies(ACPI_EC_DELAY)) &&
+           !ec_check_ibf0(ec)) {

Quoted 3 lines

Shouldn't this go under the spinlock? Surely it can race with the GPE handler.

No, we discussed this before -- we are outside of the transaction, thus no GPE
activity could interfere with ec_check_ibf0.

Quoted 2 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE activity could interfere with ec_check_ibf0.

Ok, this is in the process context and we don't really expect to get an interrupt at this point, but what happens if the EC generates an event that's not related to any transiaction. Is that guaranteed to never happen?

Thanks,
Rafael

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE
activity could interfere with ec_check_ibf0.

Ok, this is in the process context and we don't really expect to get an interrupt at this point, but what happens if the EC generates an event that's not related to any transiaction. Is that guaranteed to never happen?

Interrupt handler in this case can't cause a change to status register, thus our read of it will not be affected by interrupt.

Quoted 6 lines · Alexey Starikovskiy

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE
activity could interfere with ec_check_ibf0.

Quoted 7 lines

Quoted 4 lines

Ok, this is in the process context and we don't really expect to get an interrupt at this point, but what happens if the EC generates an event that's not related to any transiaction. Is that guaranteed to never happen?

Interrupt handler in this case can't cause a change to status register, thus our read of it will not be affected by interrupt.

Ok, thanks.

Alan, does the patch work for you?

Rafael

Quoted 17 lines · Rafael J. Wysocki

Quoted 11 lines · Alexey Starikovskiy

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE activity could interfere with ec_check_ibf0.

Ok, this is in the process context and we don't really expect to get an interrupt at this point, but what happens if the EC generates an event that's not related to any transiaction. Is that guaranteed to never happen?

Interrupt handler in this case can't cause a change to status register, thus our read of it will not be affected by interrupt.

Ok, thanks.

Alan, does the patch work for you?

Quoted 2 lines

Rafael

Yes. Two reboot cycles, three suspend/resume cycles each, and no error message.

I hope we have a better fix in mind though :-P. The patch doesn't solve the unnecessary 500ms delay when this thing happens.

drivers/acpi/ec.c +4 -0

+       }
+       return -ETIME;
+}
+ 

Quoted 28 lines · Alan Jenkins

Quoted 14 lines · Rafael J. Wysocki

Quoted 12 lines · Alexey Starikovskiy

Quoted 10 lines · Rafael J. Wysocki

Quoted 4 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE
activity could interfere with ec_check_ibf0.

Ok, this is in the process context and we don't really expect to get an
interrupt at this point, but what happens if the EC generates an event that's
not related to any transiaction. Is that guaranteed to never happen?

Quoted 9 lines

Quoted 3 lines

Interrupt handler in this case can't cause a change to status register, thus our read of it will not be affected by interrupt.

Ok, thanks.

Alan, does the patch work for you?

Rafael

Yes. Two reboot cycles, three suspend/resume cycles each, and no error message.

Quoted 2 lines

I hope we have a better fix in mind though :-P. The patch doesn't solve the unnecessary 500ms delay when this thing happens.

Something like this?

Regards,
Alex.

Quoted 36 lines · Alexis Starikovskiy

Quoted 34 lines · Alan Jenkins

Quoted 22 lines · Rafael J. Wysocki

Quoted 15 lines · Alexey Starikovskiy

Quoted 5 lines · Rafael J. Wysocki

Quoted 5 lines

No, we discussed this before -- we are outside of the transaction, thus no GPE
activity could interfere with ec_check_ibf0.

Quoted 7 lines

Ok, this is in the process context and we don't really expect to get an
interrupt at this point, but what happens if the EC generates an event that's
not related to any transiaction. Is that guaranteed to never happen?

Quoted 4 lines

Interrupt handler in this case can't cause a change to status register, thus our read of it will not be affected by interrupt.

Quoted 7 lines

Ok, thanks.

Alan, does the patch work for you?

Rafael

Yes. Two reboot cycles, three suspend/resume cycles each, and no error message.

Quoted 8 lines

Quoted 3 lines

I hope we have a better fix in mind though :-P. The patch doesn't solve the unnecessary 500ms delay when this thing happens.

Something like this?

Regards,
Alex.

You sent it as an attachment again :-).

That should work, odd as it looks. We don't need to worry about the GPE workaround because that's only active _inside_ the transaction. I don't know what Zhao thinks is missing.

Sorry I can't test right now. I tried to install 3D support on my laptop for showing-off purposes, and somehow broke X.

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

---

 drivers/acpi/ec.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

drivers/acpi/ec.c +13 -3

index 69f5f78..f9c0782 100644
@@ -300,6 +300,18 @@ static int ec_check_ibf0(struct acpi_ec *ec)
 	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
+static int ec_wait_ibf0(struct acpi_ec *ec)
+{
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	/* interrupt wait manually if GPE mode is not active */
+	unsigned long timeout = test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ?
+		msecs_to_jiffies(ACPI_EC_DELAY) : msecs_to_jiffies(1);
+	while (time_before(jiffies, delay))
+		if (wait_event_timeout(ec->wait, ec_check_ibf0(ec), timeout))
+			return 0;
+	return -ETIME;
+}
+
 static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t,
 			       int force_poll)
 {

@@ -317,8 +329,7 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t,
 			goto unlock;
 		}
 	}
-	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
-				msecs_to_jiffies(ACPI_EC_DELAY))) {
+	if (ec_wait_ibf0(ec)) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
 		status = -ETIME;
 
--

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