GPE handling

PATCH In review Li, Shaohua 4 participants 8 messages Nov 9, 2007 – Feb 2, 2008 ACPI

Share & tools

Li, Shaohua

author

2007-11-09 07:52 UTC #0 lore raw

Bob & Ming,
I'm working on adding wakeup GPE support at system runtime, this capability can help us
Identify which device invokes a wakeup event at runtime, this is required for runtime device
Power management.

Below is the ASL code. For example, _L0c, USB3 will send a wakeup GPE and invoke
a notify. In the notify handler, OS will clear USB3's PCI PME status to avoid wakeup
event flood. But in current code, acpi_ev_asynch_execute_gpe_method will start an asynchronous
execution of notify and return soon. Just before the return, acpi_ev_asynch_execute_gpe_method
will reenable GPE 0C. That means GPE is enabled before OS execute notification handler and USB3's
PCI PME status is cleared, and cause GPE flood. Ideally, I think we should delay GPE enable
of _L0c till notification handler is finished or simply the method _L0c is really finished.
What do you think?

Thanks,
Shaohua

        Method (_L0C, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB3, 0x02)
        }

        Method (_L0D, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB7, 0x02)
        }

        Method (_L0E, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB4, 0x02)
        }

Alexey Starikovskiy

2007-11-09 09:45 UTC #1 lore raw

Shaohua,
There is cond_resched() before the exit from deferred execution routine, specifically to let notify thread a chance to execute.
Thus by the time deferred execution is exited, notify should be already done, and we could safely enable _Lxx again.
Do you think it is not sufficient?

Regards,
Alex.

Quoted 46 lines · Shaohua

Bob & Ming,
I'm working on adding wakeup GPE support at system runtime, this capability can help us
Identify which device invokes a wakeup event at runtime, this is required for runtime device
Power management.
Below is the ASL code. For example, _L0c, USB3 will send a wakeup GPE and invoke
a notify. In the notify handler, OS will clear USB3's PCI PME status to avoid wakeup
event flood. But in current code, acpi_ev_asynch_execute_gpe_method will start an asynchronous
execution of notify and return soon. Just before the return, acpi_ev_asynch_execute_gpe_method
will reenable GPE 0C. That means GPE is enabled before OS execute notification handler and USB3's
PCI PME status is cleared, and cause GPE flood. Ideally, I think we should delay GPE enable
of _L0c till notification handler is finished or simply the method _L0c is really finished.
What do you think?
Thanks,
Shaohua
        Method (_L0C, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB3, 0x02)
        }

        Method (_L0D, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB7, 0x02)
        }

        Method (_L0E, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB4, 0x02)
        }
-
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

Shaohua Li

author

2007-11-12 00:48 UTC #2 lore raw

Quoted 7 lines · Alexey Starikovskiy

Shaohua,
There is cond_resched() before the exit from deferred execution routine, specifically to let notify thread a chance to execute.
Thus by the time deferred execution is exited, notify should be already done,
and we could safely enable _Lxx again.
Do you think it is not sufficient?

Certainly not. A resched() can help to give a chance for other threads to execute, but it's very unreliable. If it works, just because of luck. We should add a wait there to guarantee.

Thanks,
Shaohua

Quoted 53 lines

Quoted 41 lines · Shaohua
Bob & Ming,
I'm working on adding wakeup GPE support at system runtime, this capability can help us
Identify which device invokes a wakeup event at runtime, this is required for runtime device
Power management.
Below is the ASL code. For example, _L0c, USB3 will send a wakeup GPE and invoke
a notify. In the notify handler, OS will clear USB3's PCI PME status to avoid wakeup
event flood. But in current code, acpi_ev_asynch_execute_gpe_method will start an asynchronous
execution of notify and return soon. Just before the return, acpi_ev_asynch_execute_gpe_method
will reenable GPE 0C. That means GPE is enabled before OS execute notification handler and USB3's
PCI PME status is cleared, and cause GPE flood. Ideally, I think we should delay GPE enable
of _L0c till notification handler is finished or simply the method _L0c is really finished.
What do you think?
Thanks,
Shaohua
        Method (_L0C, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB3, 0x02)
        }

        Method (_L0D, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB7, 0x02)
        }

        Method (_L0E, 0, NotSerialized)
        {
            Notify (\_SB.PCI0.USB4, 0x02)
        }
Quoted 5 lines
-
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
-
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

2007-11-13 10:05 UTC #3 lore raw

PATCH 1/1 ACPI: Defer enabling of level GPE until all pending notifies done

Level GPE should not be enabled until all work caused by it is done, e.g. all Notify() methods are completed.
This could be accomplished by appending enable_gpe function to the end of notify queue.

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

---

 drivers/acpi/events/evgpe.c |   17 +++++++++++++----
 drivers/acpi/osl.c          |   42 ++++++++----------------------------------
 2 files changed, 21 insertions(+), 38 deletions(-)

drivers/acpi/events/evgpe.c +13 -4

index e22f4a9..b4509f9 100644
@@ -501,6 +501,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info * gpe_xrupt_list)
  *              an interrupt handler.
  *
  ******************************************************************************/
+static void acpi_ev_asynch_enable_gpe(void *context);
 
 static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 {

@@ -576,22 +577,30 @@ static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 					 method_node)));
 		}
 	}
+	/* Defer enabling of GPE until all notify handlers are done */
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_asynch_enable_gpe,
+				gpe_event_info);
+	return_VOID;
+}
 
-	if ((local_gpe_event_info.flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+static void acpi_ev_asynch_enable_gpe(void *context)
+{
+	struct acpi_gpe_event_info *gpe_event_info = context;
+	acpi_status status;
+	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
 	    ACPI_GPE_LEVEL_TRIGGERED) {
 		/*
 		 * GPE is level-triggered, we clear the GPE status bit after
 		 * handling the event.
 		 */
-		status = acpi_hw_clear_gpe(&local_gpe_event_info);
+		status = acpi_hw_clear_gpe(gpe_event_info);
 		if (ACPI_FAILURE(status)) {
 			return_VOID;
 		}
 	}
 
 	/* Enable this GPE */
-
-	(void)acpi_hw_write_gpe_enable_reg(&local_gpe_event_info);
+	(void)acpi_hw_write_gpe_enable_reg(gpe_event_info);
 	return_VOID;
 }

drivers/acpi/osl.c +8 ~~-34~~

60 lines not on this page · raw message

Shaohua Li

author

2007-11-15 05:51 UTC #4 lore raw

Re: [PATCH] ACPI: Defer enabling of level GPE until all pending notifiesdone

Quoted 4 lines · Alexey Starikovskiy

Level GPE should not be enabled until all work caused by it is done, e.g. all Notify() methods are completed.
This could be accomplished by appending enable_gpe function to the end of notify queue.

Patch looks good, but the method is a little tricky, I'd suggest adding some comments in the code to explain it.

Thanks,
Shaohua

Len Brown

reviewer

2007-12-07 02:55 UTC #5 lore raw

Re: [PATCH] ACPI: Defer enabling of level GPE until all pending notifies done

In reply to Alexey Starikovskiy

applied to acpi test branch.

thanks,
-len

Quoted 132 lines · Alexey Starikovskiy

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

---

 drivers/acpi/events/evgpe.c |   17 +++++++++++++----
 drivers/acpi/osl.c          |   42 ++++++++----------------------------------
 2 files changed, 21 insertions(+), 38 deletions(-)

drivers/acpi/events/evgpe.c +13 -4

index e22f4a9..b4509f9 100644
@@ -501,6 +501,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info * gpe_xrupt_list)
  *              an interrupt handler.
  *
  ******************************************************************************/
+static void acpi_ev_asynch_enable_gpe(void *context);
 
 static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 {

@@ -576,22 +577,30 @@ static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 					 method_node)));
 		}
 	}
+	/* Defer enabling of GPE until all notify handlers are done */
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_asynch_enable_gpe,
+				gpe_event_info);
+	return_VOID;
+}
 
-	if ((local_gpe_event_info.flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+static void acpi_ev_asynch_enable_gpe(void *context)
+{
+	struct acpi_gpe_event_info *gpe_event_info = context;
+	acpi_status status;
+	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
 	    ACPI_GPE_LEVEL_TRIGGERED) {
 		/*
 		 * GPE is level-triggered, we clear the GPE status bit after
 		 * handling the event.
 		 */
-		status = acpi_hw_clear_gpe(&local_gpe_event_info);
+		status = acpi_hw_clear_gpe(gpe_event_info);
 		if (ACPI_FAILURE(status)) {
 			return_VOID;
 		}
 	}
 
 	/* Enable this GPE */
-
-	(void)acpi_hw_write_gpe_enable_reg(&local_gpe_event_info);
+	(void)acpi_hw_write_gpe_enable_reg(gpe_event_info);
 	return_VOID;
 }

drivers/acpi/osl.c +8 ~~-35~~

index aabc6ca..6816ac6 100644
@@ -625,25 +625,6 @@ static void acpi_os_execute_deferred(struct work_struct *work)
 	dpc->function(dpc->context);
 	kfree(dpc);
 
-	/* Yield cpu to notify thread */
-	cond_resched();
-
-	return;
-}
-
-static void acpi_os_execute_notify(struct work_struct *work)
-{
-	struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
-
-	if (!dpc) {
-		printk(KERN_ERR PREFIX "Invalid (NULL) context\n");
-		return;
-	}
-
-	dpc->function(dpc->context);
-
-	kfree(dpc);
-
 	return;
 }

@@ -667,7 +648,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 {
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
-
+	struct workqueue_struct *queue;
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.\n",
 			  function, context));

@@ -691,20 +672,13 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 	dpc->function = function;
 	dpc->context = context;
 
-	if (type == OSL_NOTIFY_HANDLER) {
-		INIT_WORK(&dpc->work, acpi_os_execute_notify);
-		if (!queue_work(kacpi_notify_wq, &dpc->work)) {
-			status = AE_ERROR;
-			kfree(dpc);
-		}
-	} else {
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-		if (!queue_work(kacpid_wq, &dpc->work)) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				  "Call to queue_work() failed.\n"));
-			status = AE_ERROR;
-			kfree(dpc);
-		}
+	INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+	queue = (type == OSL_NOTIFY_HANDLER) ? kacpi_notify_wq : kacpid_wq;
+	if (!queue_work(queue, &dpc->work)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+			  "Call to queue_work() failed.\n"));
+		status = AE_ERROR;
+		kfree(dpc);
 	}
 	return_ACPI_STATUS(status);
 }
 
-

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

Len Brown

reviewer

2008-02-02 09:46 UTC #6 lore raw

Re: [PATCH] ACPI: Defer enabling of level GPE until all pending notifies done

In reply to Alexey Starikovskiy

Alexey, Rui,
this now conflicts with theacpi_os_execute() changes proposed in the battery hotplug series.

recommendations?

thanks,
-Len

Quoted 132 lines · Alexey Starikovskiy

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

---

 drivers/acpi/events/evgpe.c |   17 +++++++++++++----
 drivers/acpi/osl.c          |   42 ++++++++----------------------------------
 2 files changed, 21 insertions(+), 38 deletions(-)

drivers/acpi/events/evgpe.c +13 -4

index e22f4a9..b4509f9 100644
@@ -501,6 +501,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info * gpe_xrupt_list)
  *              an interrupt handler.
  *
  ******************************************************************************/
+static void acpi_ev_asynch_enable_gpe(void *context);
 
 static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 {

@@ -576,22 +577,30 @@ static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 					 method_node)));
 		}
 	}
+	/* Defer enabling of GPE until all notify handlers are done */
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_asynch_enable_gpe,
+				gpe_event_info);
+	return_VOID;
+}
 
-	if ((local_gpe_event_info.flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+static void acpi_ev_asynch_enable_gpe(void *context)
+{
+	struct acpi_gpe_event_info *gpe_event_info = context;
+	acpi_status status;
+	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
 	    ACPI_GPE_LEVEL_TRIGGERED) {
 		/*
 		 * GPE is level-triggered, we clear the GPE status bit after
 		 * handling the event.
 		 */
-		status = acpi_hw_clear_gpe(&local_gpe_event_info);
+		status = acpi_hw_clear_gpe(gpe_event_info);
 		if (ACPI_FAILURE(status)) {
 			return_VOID;
 		}
 	}
 
 	/* Enable this GPE */
-
-	(void)acpi_hw_write_gpe_enable_reg(&local_gpe_event_info);
+	(void)acpi_hw_write_gpe_enable_reg(gpe_event_info);
 	return_VOID;
 }

drivers/acpi/osl.c +8 ~~-35~~

index aabc6ca..6816ac6 100644
@@ -625,25 +625,6 @@ static void acpi_os_execute_deferred(struct work_struct *work)
 	dpc->function(dpc->context);
 	kfree(dpc);
 
-	/* Yield cpu to notify thread */
-	cond_resched();
-
-	return;
-}
-
-static void acpi_os_execute_notify(struct work_struct *work)
-{
-	struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
-
-	if (!dpc) {
-		printk(KERN_ERR PREFIX "Invalid (NULL) context\n");
-		return;
-	}
-
-	dpc->function(dpc->context);
-
-	kfree(dpc);
-
 	return;
 }

@@ -667,7 +648,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 {
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
-
+	struct workqueue_struct *queue;
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.\n",
 			  function, context));

@@ -691,20 +672,13 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 	dpc->function = function;
 	dpc->context = context;
 
-	if (type == OSL_NOTIFY_HANDLER) {
-		INIT_WORK(&dpc->work, acpi_os_execute_notify);
-		if (!queue_work(kacpi_notify_wq, &dpc->work)) {
-			status = AE_ERROR;
-			kfree(dpc);
-		}
-	} else {
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-		if (!queue_work(kacpid_wq, &dpc->work)) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				  "Call to queue_work() failed.\n"));
-			status = AE_ERROR;
-			kfree(dpc);
-		}
+	INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+	queue = (type == OSL_NOTIFY_HANDLER) ? kacpi_notify_wq : kacpid_wq;
+	if (!queue_work(queue, &dpc->work)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+			  "Call to queue_work() failed.\n"));
+		status = AE_ERROR;
+		kfree(dpc);
 	}
 	return_ACPI_STATUS(status);
 }
 
-

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

2008-02-02 11:05 UTC #7 lore raw

Re: [PATCH] ACPI: Defer enabling of level GPE until all pending notifies done

Quoted 3 lines · Len Brown

Alexey, Rui,
this now conflicts with theacpi_os_execute() changes proposed in the battery hotplug series.

Great! We already sold this patch to FreeBSD, but don't use it ourselves?

Quoted 2 lines

recommendations?

There are no fundamental conflicts, these patches just happen to change same function. I could adjust any of two patches to not conflict with the other.

Regards,
Alex.

Quoted 138 lines

thanks,
-Len

Quoted 132 lines · Alexey Starikovskiy

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

---

 drivers/acpi/events/evgpe.c |   17 +++++++++++++----
 drivers/acpi/osl.c          |   42 ++++++++----------------------------------
 2 files changed, 21 insertions(+), 38 deletions(-)

drivers/acpi/events/evgpe.c +13 -4

index e22f4a9..b4509f9 100644
@@ -501,6 +501,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info * gpe_xrupt_list)
  *              an interrupt handler.
  *
  ******************************************************************************/
+static void acpi_ev_asynch_enable_gpe(void *context);
 
 static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 {

@@ -576,22 +577,30 @@ static void ACPI_SYSTEM_XFACE acpi_ev_asynch_execute_gpe_method(void *context)
 					 method_node)));
 		}
 	}
+	/* Defer enabling of GPE until all notify handlers are done */
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_asynch_enable_gpe,
+				gpe_event_info);
+	return_VOID;
+}
 
-	if ((local_gpe_event_info.flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
+static void acpi_ev_asynch_enable_gpe(void *context)
+{
+	struct acpi_gpe_event_info *gpe_event_info = context;
+	acpi_status status;
+	if ((gpe_event_info->flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
 	    ACPI_GPE_LEVEL_TRIGGERED) {
 		/*
 		 * GPE is level-triggered, we clear the GPE status bit after
 		 * handling the event.
 		 */
-		status = acpi_hw_clear_gpe(&local_gpe_event_info);
+		status = acpi_hw_clear_gpe(gpe_event_info);
 		if (ACPI_FAILURE(status)) {
 			return_VOID;
 		}
 	}
 
 	/* Enable this GPE */
-
-	(void)acpi_hw_write_gpe_enable_reg(&local_gpe_event_info);
+	(void)acpi_hw_write_gpe_enable_reg(gpe_event_info);
 	return_VOID;
 }

drivers/acpi/osl.c +8 ~~-35~~

index aabc6ca..6816ac6 100644
@@ -625,25 +625,6 @@ static void acpi_os_execute_deferred(struct work_struct *work)
 	dpc->function(dpc->context);
 	kfree(dpc);
 
-	/* Yield cpu to notify thread */
-	cond_resched();
-
-	return;
-}
-
-static void acpi_os_execute_notify(struct work_struct *work)
-{
-	struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
-
-	if (!dpc) {
-		printk(KERN_ERR PREFIX "Invalid (NULL) context\n");
-		return;
-	}
-
-	dpc->function(dpc->context);
-
-	kfree(dpc);
-
 	return;
 }

@@ -667,7 +648,7 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 {
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
-
+	struct workqueue_struct *queue;
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.\n",
 			  function, context));

@@ -691,20 +672,13 @@ acpi_status acpi_os_execute(acpi_execute_type type,
 	dpc->function = function;
 	dpc->context = context;
 
-	if (type == OSL_NOTIFY_HANDLER) {
-		INIT_WORK(&dpc->work, acpi_os_execute_notify);
-		if (!queue_work(kacpi_notify_wq, &dpc->work)) {
-			status = AE_ERROR;
-			kfree(dpc);
-		}
-	} else {
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-		if (!queue_work(kacpid_wq, &dpc->work)) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-				  "Call to queue_work() failed.\n"));
-			status = AE_ERROR;
-			kfree(dpc);
-		}
+	INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+	queue = (type == OSL_NOTIFY_HANDLER) ? kacpi_notify_wq : kacpid_wq;
+	if (!queue_work(queue, &dpc->work)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+			  "Call to queue_work() failed.\n"));
+		status = AE_ERROR;
+		kfree(dpc);
 	}
 	return_ACPI_STATUS(status);
 }
 
-

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