I believe that this is the reason for disabling the GPE in the handler,
so that it will not generate additional interrupts (SCIs)
case ACPI_GPE_DISPATCH_METHOD:
/*
* Disable GPE, so it doesn't keep firing before the method has
a
* chance to run.
*/
Status = AcpiEvDisableGpe (GpeEventInfo);
> -----Original Message-----
> From: William Morrow [mailto:William.Morrow@xxxxxxx]
> Sent: Friday, August 18, 2006 3:01 PM
> To: Moore, Robert
> Cc: akpm@xxxxxxxx; Brown, Len; linux-acpi@xxxxxxxxxxxxxxx;
> jordan.crouse@xxxxxxx; Yu, Luming
> Subject: Re: [patch 12/14] ACPI: Clear GPE before disabling it
>
> Moore, Robert wrote:
>
> >Here is an explanation from one of the ACPI experts:
> >
> >
> >
> >>In most cases, it cannot be cleared until after the method is
> >>
> >>
> >executed.
> >
> >
> >>The method usually talks to the hardware that is holding the input
> >>signal low to tell it to "let go". If you try to clear before the
> >>
> >>
> >method
> >
> >
> >>is executed, it won't work because the GPE will still be held low.
> >>
> >>
> I still don't see how the background method can execute with an
interrupt
> pending. I suggest that the handler should disable the pending
event,
> schedule the background method to deal with the hardware, and clear
the
> event status. The event/interrupt wont be refired because it is
disabled.
> The background task will have to deal with the hardware to remove the
> event
> assertion. This would simplify the suggested fix. To that end, I
have
> created another patch which is attached.
>
> I notice that in all cases, it should be that the interrupt status
should
> be
> cleared or a storm may be possible. I suggest moving the
> acpi_hw_clear_gpe
> call in the dispatch handler to the end of the acpi_ev_gpe_dispatch
> routine.
> This gives the code more strength (is cleared in any case) and
provides
> good bracketing - although it is not clear why the status should be
> cleared
> later rather than sooner. It also sort of resembles the spec.
>
> This does alter the hw status presented in the ctrl when the
background
> hander runs. But since that is asynchronous, if that data is
important
> it should be captured in the isr and forwarded via the handler
parameter
> interface.
>
> morrow
>
> >
> >
> >
> >>-----Original Message-----
> >>From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-
> >>owner@xxxxxxxxxxxxxxx] On Behalf Of William Morrow
> >>Sent: Friday, August 18, 2006 11:15 AM
> >>To: Moore, Robert
> >>Cc: akpm@xxxxxxxx; Brown, Len; linux-acpi@xxxxxxxxxxxxxxx;
> >>jordan.crouse@xxxxxxx; Yu, Luming
> >>Subject: Re: [patch 12/14] ACPI: Clear GPE before disabling it
> >>
> >>Moore, Robert wrote:
> >>
> >>
> >>
> >>>Here is the required software behavior, from the ACPI spec version
> >>>
> >>>
> >3.0A,
> >
> >
> >>>page 138, section 5.6.2.2:
> >>>
> >>>When OSPM receives a general-purpose event (the event is from a
> >>>
> >>>
> >GPEx_BLK
> >
> >
> >>>STS bit), OSPM does the following:
> >>>1. Disables the interrupt source (GPEx_BLK EN bit).
> >>>2. If an edge event, clears the status bit.
> >>>3. Performs one of the following:
> >>>* Dispatches to an ACPI-aware device driver.
> >>>* Queues the matching control method for execution.
> >>>* Manages a wake event using device _PRW objects.
> >>>4. If a level event, clears the status bit.
> >>>5. Enables the interrupt source.
> >>>
> >>>
> >>>
> >>>
> >>I have to admit, I dont see how step 4 is supposed to be occurring.
> >>If any unclearable interrupt source is active here, then the storm
> >>
> >>
> >will
> >
> >
> >>ensue.
> >>The background execution of a method will never happen to clear it.
> >>
> >>I have to plead ignorance when it comes to the full understanding of
> >>the acpi system. On re-reading your email, I think I may have
missed
> >>your point on clearing the events in other cases. If you are
> >>
> >>
> >advocating
> >
> >
> >>that the fix should be conditioned to only clear the bit in the
level
> >>
> >>
> >case
> >
> >
> >>then I have to agree. Like I said, I frequently seem to miss the
> >>
> >>
> >point.
> >
> >
> >>morrow
> >>
> >>
> >>
> >>>This is essentially what is implemented today in ACPICA. (It
appears
> >>>that 1 and 2 are transposed in the code, this may or may not be a
> >>>problem.)
> >>>
> >>>Note that edge GPEs are cleared before dispatch, but level GPEs are
> >>>cleared after dispatch.
> >>>
> >>>Bob
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: William Morrow [mailto:William.Morrow@xxxxxxx]
> >>>>Sent: Thursday, August 17, 2006 9:39 AM
> >>>>To: Moore, Robert
> >>>>Cc: akpm@xxxxxxxx; Brown, Len; linux-acpi@xxxxxxxxxxxxxxx;
> >>>>jordan.crouse@xxxxxxx; Yu, Luming
> >>>>Subject: Re: [patch 12/14] ACPI: Clear GPE before disabling it
> >>>>
> >>>>Moore, Robert wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>>Worse, the GPE is already cleared in the edge case, at the start
of
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>the
> >>>
> >>>
> >>>
> >>>
> >>>>>GPE dispatch function:
> >>>>>
> >>>>> /*
> >>>>> * If edge-triggered, clear the GPE status bit now. Note that
> >>>>> * level-triggered events are cleared after the GPE is
serviced.
> >>>>> */
> >>>>> if ((GpeEventInfo->Flags & ACPI_GPE_XRUPT_TYPE_MASK) ==
> >>>>> ACPI_GPE_EDGE_TRIGGERED)
> >>>>> {
> >>>>> Status = AcpiHwClearGpe (GpeEventInfo);
> >>>>> if (ACPI_FAILURE (Status))
> >>>>> {
> >>>>> ACPI_EXCEPTION ((AE_INFO, Status,
> >>>>> "Unable to clear GPE[%2X]", GpeNumber));
> >>>>> return_UINT32 (ACPI_INTERRUPT_NOT_HANDLED);
> >>>>> }
> >>>>> }
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>Sorry for the late reply, but I have not looked at this for some
> >>>>
> >>>>
> >time.
> >
> >
> >>>>I had to re-examine this issue.
> >>>>
> >>>>The interrupt type is tagged as level triggered in this case.
> >>>>
> >>>>The dispatch handler is ACPI_GPE_DISPATCH_METHOD and so the
> >>>>method is queued, but the interrupt is not cleared (it is disabled
> >>>>instead).
> >>>>In this hw, that is all that happens. It is disabled but not
> >>>>
> >>>>
> >cleared.
> >
> >
> >>>>The original comment would lead me to think that the prevailing
> >>>>
> >>>>
> >>>>
> >>>>
> >>>thought
> >>>
> >>>
> >>>
> >>>
> >>>>was that disabling the gpe implied clearing the interrupt status.
> >>>>
> >>>>
> >>>>
> >>>>
> >>>That is
> >>>
> >>>
> >>>
> >>>
> >>>>not the case in this hw environment, and the result is the
interrupt
> >>>>storm.
> >>>>
> >>>>Although this handler is artfully done, the architecture of this
> >>>>handler is unusual in that most interrupt handlers field status
> >>>>and clear the offending event at once, then process the new input.
> >>>>This handler is coded in a way that seems to indicate that
interrupt
> >>>>status is possible when events are not enabled, but is not willing
> >>>>
> >>>>
> >to
> >
> >
> >>>>clear them - again indicating that the hw design is required to
> >>>>
> >>>>
> >block
> >
> >
> >>>>interrupts if the event has been disabled, but the interrupt
status
> >>>>
> >>>>
> >is
> >
> >
> >>>>not clear. If that is true - then this code has no effect, or is
> >>>>
> >>>>
> >>>>
> >>>>
> >>>another
> >>>
> >>>
> >>>
> >>>
> >>>>case which can result in an interrupt storm.
> >>>>
> >>>> /* Check if there is anything active at all in this register */
> >>>>
> >>>> enabled_status_byte = (u8) (status_reg & enable_reg);
> >>>> if (!enabled_status_byte) {
> >>>> /* No active GPEs in this register, move on */
> >>>> continue;
> >>>> }
> >>>>
> >>>>Additionally, any error return which prevents the interrupt
> >>>>source from being cleared will result in an interrupt storm.
> >>>>This occurs several times.
> >>>>
> >>>>The comment:
> >>>>
> >>>>/*
> >>>>* Execute the method associated with the GPE
> >>>>* NOTE: Level-triggered GPEs are cleared after the method
> >>>>
> >>>>
> >completes.
> >
> >
> >>>>*/
> >>>>Status = AcpiOsExecute (OSL_GPE_HANDLER,
> >>>> AcpiEvAsynchExecuteGpeMethod, GpeEventInfo);
> >>>>
> >>>>is alarming, since it documents that the interrupt is active and
> >>>>expects the OS to be able to schedule with pending interrupts.
> >>>>
> >>>>My personal preference is that the code be modified to work in
> >>>>the more traditional way, but since that would be a large change
> >>>>and this produced the desired result - I opted for the minimum
> >>>>coding distance change.
> >>>>
> >>>>
> >>>>If there are any other materials you need to evaluate this change,
> >>>>
> >>>>
> >let
> >
> >
> >>>>me know.
> >>>>I hope this addresses your point. I am sort of renowned for being
a
> >>>>little askew
> >>>>when trying to explain myself.
> >>>>
> >>>>Thanks for your attention!
> >>>>morrow
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>-----Original Message-----
> >>>>>>From: Moore, Robert
> >>>>>>Sent: Wednesday, August 16, 2006 2:50 PM
> >>>>>>To: 'akpm@xxxxxxxx'; Brown, Len
> >>>>>>Cc: linux-acpi@xxxxxxxxxxxxxxx; william.morrow@xxxxxxx;
> >>>>>>jordan.crouse@xxxxxxx; Yu, Luming
> >>>>>>Subject: RE: [patch 12/14] ACPI: Clear GPE before disabling it
> >>>>>>
> >>>>>>How does this patch relate to level-triggered GPEs, where we
have
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>the
> >>>
> >>>
> >>>
> >>>
> >>>>>>following comment in the code just after the patch:
> >>>>>>
> >>>>>> /*
> >>>>>> * Execute the method associated with the GPE
> >>>>>> * NOTE: Level-triggered GPEs are cleared after the method
> >>>>>>completes.
> >>>>>> */
> >>>>>> Status = AcpiOsExecute (OSL_GPE_HANDLER,
> >>>>>> AcpiEvAsynchExecuteGpeMethod, GpeEventInfo);
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>-----Original Message-----
> >>>>>>>From: akpm@xxxxxxxx [mailto:akpm@xxxxxxxx]
> >>>>>>>Sent: Monday, August 14, 2006 10:38 PM
> >>>>>>>To: Brown, Len
> >>>>>>>Cc: linux-acpi@xxxxxxxxxxxxxxx; akpm@xxxxxxxx;
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>william.morrow@xxxxxxx;
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>>jordan.crouse@xxxxxxx; Yu, Luming; Moore, Robert
> >>>>>>>Subject: [patch 12/14] ACPI: Clear GPE before disabling it
> >>>>>>>
> >>>>>>>From: William Morrow <william.morrow@xxxxxxx>
> >>>>>>>
> >>>>>>>On some BIOSen, the GPE bit will remain set even if it is
> >>>>>>>
> >>>>>>>
> >disabled,
> >
> >
> >>>>>>>resulting in a interrupt storm. This patch clears the bit
before
> >>>>>>>disabling
> >>>>>>>it.
> >>>>>>>
> >>>>>>>Signed-off-by: William Morrow <william.morrow@xxxxxxx>
> >>>>>>>Signed-off-by: Jordan Crouse <jordan.crouse@xxxxxxx>
> >>>>>>>Cc: "Yu, Luming" <luming.yu@xxxxxxxxx>
> >>>>>>>Cc: "Brown, Len" <len.brown@xxxxxxxxx>
> >>>>>>>Cc: "Moore, Robert" <robert.moore@xxxxxxxxx>
> >>>>>>>Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
> >>>>>>>---
> >>>>>>>
> >>>>>>>drivers/acpi/events/evgpe.c | 14 +++++++++++++-
> >>>>>>>1 file changed, 13 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>>diff -puN
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>drivers/acpi/events/evgpe.c~acpi-clear-gpe-before-disabling-it
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>>drivers/acpi/events/evgpe.c
> >>>>>>>---
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>a/drivers/acpi/events/evgpe.c~acpi-clear-gpe-before-disabling-it
> >>>
> >>>
> >>>
> >>>
> >>>>>>>+++ a/drivers/acpi/events/evgpe.c
> >>>>>>>@@ -677,10 +677,22 @@ acpi_ev_gpe_dispatch(struct acpi_gpe_eve
> >>>>>>> case ACPI_GPE_DISPATCH_METHOD:
> >>>>>>>
> >>>>>>> /*
> >>>>>>>- * Disable GPE, so it doesn't keep firing before
> >>>>>>>
> >>>>>>>
> >the
> >
> >
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>method
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>>has a
> >>>>>>>+ * Clear GPE, so it doesn't keep firing before
> >>>>>>>
> >>>>>>>
> >the
> >
> >
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>method has
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>>>a
> >>>>>>> * chance to run.
> >>>>>>> */
> >>>>>>>+ status = acpi_hw_clear_gpe(gpe_event_info);
> >>>>>>>+ if (ACPI_FAILURE(status)) {
> >>>>>>>+ ACPI_EXCEPTION((AE_INFO, status,
> >>>>>>>+ "Unable to clear
> >>>>>>>
> >>>>>>>
> >GPE[%2X]",
> >
> >
> >>>>>>>+ gpe_number));
> >>>>>>>+
> >>>>>>>
> >>>>>>>
> >return_UINT32(ACPI_INTERRUPT_NOT_HANDLED);
> >
> >
> >>>>>>>+ }
> >>>>>>>+ /*
> >>>>>>>+ * Disable GPE, so it doesn't keep happen again.
> >>>>>>>+ */
> >>>>>>>+
> >>>>>>> status = acpi_ev_disable_gpe(gpe_event_info);
> >>>>>>>+
> >>>>>>> if (ACPI_FAILURE(status)) {
> >>>>>>> ACPI_EXCEPTION((AE_INFO, status,
> >>>>>>> "Unable to disable
GPE[%2X]",
> >>>>>>>_
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>-
> >>To unsubscribe from this list: send the line "unsubscribe
linux-acpi"
> >>
> >>
> >in
> >
> >
> >>the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
