Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Govindraj.R]

Hello,

During system_wide_suspend pm runtime is disabled.
I.e. __pm_runtime_disable is called from __device_suspend.
Now, if a wakeup interrupt is triggered and the wakeup device irq handler
is called even before device_resume and pm_runtime_enable happens,
the device irq_handler proceeds to enable clock with runtime API to
handle wakeup event.
 
Wouldn't this result in system wide abort since the pm_runtime is not enabled
yet from dpm_resume?
As we end up accessing regs after doing runtime get_sync.
 
Looks like this scenario is not handled currently.
Or Am I missing something here?

--
Thanks,
Govindraj.R

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Fri, 26 Aug 2011, Govindraj.R wrote:

> Hello,
> 
> During system_wide_suspend pm runtime is disabled.
> I.e. __pm_runtime_disable is called from __device_suspend.
> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
> is called even before device_resume and pm_runtime_enable happens,
> the device irq_handler proceeds to enable clock with runtime API to
> handle wakeup event.
>  
> Wouldn't this result in system wide abort since the pm_runtime is not enabled
> yet from dpm_resume?
> As we end up accessing regs after doing runtime get_sync.
>  
> Looks like this scenario is not handled currently.
> Or Am I missing something here?

I don't have the complete picture, but it seems that the IRQ handler 
needs to check the return code from pm_runtime_get_sync().  If the call 
fails then the handler shouldn't try to access the device registers.

In the case of a genuine wakeup event, the event should be handled 
later on as part of the resume or resume_noirq processing.

However, this does raise a potential problem.  What happens if the 
clocks are needed in order to turn off the IRQ source?  Runtime PM 
won't allow the clocks to be enabled until after interrupts have been 
enabled, and by then it will be too late -- the source will have caused 
an interrupt storm.

Alan Stern

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Santosh]

Alen,

On Saturday 27 August 2011 12:06 AM, Alan Stern wrote:
> On Fri, 26 Aug 2011, Govindraj.R wrote:
>
>> Hello,
>>
>> During system_wide_suspend pm runtime is disabled.
>> I.e. __pm_runtime_disable is called from __device_suspend.
>> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
>> is called even before device_resume and pm_runtime_enable happens,
>> the device irq_handler proceeds to enable clock with runtime API to
>> handle wakeup event.
>>
>> Wouldn't this result in system wide abort since the pm_runtime is not enabled
>> yet from dpm_resume?
>> As we end up accessing regs after doing runtime get_sync.
>>
>> Looks like this scenario is not handled currently.
>> Or Am I missing something here?
>
> I don't have the complete picture, but it seems that the IRQ handler
> needs to check the return code from pm_runtime_get_sync().  If the call
> fails then the handler shouldn't try to access the device registers.
>
> In the case of a genuine wakeup event, the event should be handled
> later on as part of the resume or resume_noirq processing.
>
> However, this does raise a potential problem.  What happens if the
> clocks are needed in order to turn off the IRQ source?  Runtime PM
> won't allow the clocks to be enabled until after interrupts have been
> enabled, and by then it will be too late -- the source will have caused
> an interrupt storm.
>
I might be wrong here, but after discussion with Govindraj on this
issue, it seems there is a flaw in the way OMAP chain handler
handling the child interrupts.

On OMAP, we have special interrupt wakeup source at PRCM level and
many devices can trigger wakeup from low power via this common
interrupt source. The common interrupt source upon wakeup from low
power state, decodes the source of interrupt and based on that
source, calls the respective device ISR directly.

The issue I see here is, the ISR on _a_ device (UART in this case)
is happening even before UART resume and DPM resume has been completed.
If this is the case, then it is surely asking for trouble. Because not
just clocks, but even driver state machine is already in suspend state
when the ISR is called.

Regards
Santosh

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Santosh]

Alen,

On Saturday 27 August 2011 12:06 AM, Alan Stern wrote:
> On Fri, 26 Aug 2011, Govindraj.R wrote:
>
>> Hello,
>>
>> During system_wide_suspend pm runtime is disabled.
>> I.e. __pm_runtime_disable is called from __device_suspend.
>> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
>> is called even before device_resume and pm_runtime_enable happens,
>> the device irq_handler proceeds to enable clock with runtime API to
>> handle wakeup event.
>>
>> Wouldn't this result in system wide abort since the pm_runtime is not enabled
>> yet from dpm_resume?
>> As we end up accessing regs after doing runtime get_sync.
>>
>> Looks like this scenario is not handled currently.
>> Or Am I missing something here?
>
> I don't have the complete picture, but it seems that the IRQ handler
> needs to check the return code from pm_runtime_get_sync().  If the call
> fails then the handler shouldn't try to access the device registers.
>
> In the case of a genuine wakeup event, the event should be handled
> later on as part of the resume or resume_noirq processing.
>
> However, this does raise a potential problem.  What happens if the
> clocks are needed in order to turn off the IRQ source?  Runtime PM
> won't allow the clocks to be enabled until after interrupts have been
> enabled, and by then it will be too late -- the source will have caused
> an interrupt storm.
>
I might be wrong here, but after discussion with Govindraj on this
issue, it seems there is a flaw in the way OMAP chain handler
handling the child interrupts.

On OMAP, we have special interrupt wakeup source at PRCM level and
many devices can trigger wakeup from low power via this common
interrupt source. The common interrupt source upon wakeup from low
power state, decodes the source of interrupt and based on that
source, calls the respective device ISR directly.

The issue I see here is, the ISR on _a_ device (UART in this case)
is happening even before UART resume and DPM resume has been completed.
If this is the case, then it is surely asking for trouble. Because not
just clocks, but even driver state machine is already in suspend state
when the ISR is called.

Regards
Santosh

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Sat, 27 Aug 2011, Santosh wrote:

> I might be wrong here, but after discussion with Govindraj on this
> issue, it seems there is a flaw in the way OMAP chain handler
> handling the child interrupts.
> 
> On OMAP, we have special interrupt wakeup source at PRCM level and
> many devices can trigger wakeup from low power via this common
> interrupt source. The common interrupt source upon wakeup from low
> power state, decodes the source of interrupt and based on that
> source, calls the respective device ISR directly.
> 
> The issue I see here is, the ISR on _a_ device (UART in this case)
> is happening even before UART resume and DPM resume has been completed.
> If this is the case, then it is surely asking for trouble. Because not
> just clocks, but even driver state machine is already in suspend state
> when the ISR is called.

If the driver state machine is in the suspend state when the ISR is
called, then the ISR should realize it is handling a wakeup event
instead of a normal I/O event.  All it needs to do is turn off the
interrupt source; the event itself will be taken care of during the
device's resume callback.

Alan Stern

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Santosh]

On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> On Sat, 27 Aug 2011, Santosh wrote:
>
>> I might be wrong here, but after discussion with Govindraj on this
>> issue, it seems there is a flaw in the way OMAP chain handler
>> handling the child interrupts.
>>
>> On OMAP, we have special interrupt wakeup source at PRCM level and
>> many devices can trigger wakeup from low power via this common
>> interrupt source. The common interrupt source upon wakeup from low
>> power state, decodes the source of interrupt and based on that
>> source, calls the respective device ISR directly.
>>
>> The issue I see here is, the ISR on _a_ device (UART in this case)
>> is happening even before UART resume and DPM resume has been completed.
>> If this is the case, then it is surely asking for trouble. Because not
>> just clocks, but even driver state machine is already in suspend state
>> when the ISR is called.
>
> If the driver state machine is in the suspend state when the ISR is
> called, then the ISR should realize it is handling a wakeup event
> instead of a normal I/O event.  All it needs to do is turn off the
> interrupt source; the event itself will be taken care of during the
> device's resume callback.
>
Good point but the ISR is called as a function call and not real
hardware event so no need to turn-off the source in the child
ISR. Parent ISR will clear the source anyways.

But the intention here is to record the event for the child.
I mean for UART wakeup, the received character should be
extracted. If not done, UART will loose that character because
the event is lost. So not sure how the event will be taken
care during resume callback. Could you elaborate bit more on
last comment please?

Regards
Santosh

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Sat, 27 Aug 2011, Santosh wrote:

> On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> > On Sat, 27 Aug 2011, Santosh wrote:
> >
> >> I might be wrong here, but after discussion with Govindraj on this
> >> issue, it seems there is a flaw in the way OMAP chain handler
> >> handling the child interrupts.
> >>
> >> On OMAP, we have special interrupt wakeup source at PRCM level and
> >> many devices can trigger wakeup from low power via this common
> >> interrupt source. The common interrupt source upon wakeup from low
> >> power state, decodes the source of interrupt and based on that
> >> source, calls the respective device ISR directly.
> >>
> >> The issue I see here is, the ISR on _a_ device (UART in this case)
> >> is happening even before UART resume and DPM resume has been completed.
> >> If this is the case, then it is surely asking for trouble. Because not
> >> just clocks, but even driver state machine is already in suspend state
> >> when the ISR is called.
> >
> > If the driver state machine is in the suspend state when the ISR is
> > called, then the ISR should realize it is handling a wakeup event
> > instead of a normal I/O event.  All it needs to do is turn off the
> > interrupt source; the event itself will be taken care of during the
> > device's resume callback.
> >
> Good point but the ISR is called as a function call and not real
> hardware event so no need to turn-off the source in the child
> ISR. Parent ISR will clear the source anyways.
> 
> But the intention here is to record the event for the child.

In that case the ISR only has to record the event.

> I mean for UART wakeup, the received character should be
> extracted. If not done, UART will loose that character because
> the event is lost. So not sure how the event will be taken
> care during resume callback. Could you elaborate bit more on
> last comment please?

The resume callback routine must check to see if an event was recorded.
If one was, the routine must see whether a character was received while 
the system was asleep and extract the character from the UART.  (It 
probably won't hurt to do this even if an event wasn't recorded.)

Alan Stern

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Tero Kristo]

Hi,

On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
> On Sat, 27 Aug 2011, Santosh wrote:
> 
> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> > > On Sat, 27 Aug 2011, Santosh wrote:
> > >
> > >> I might be wrong here, but after discussion with Govindraj on this
> > >> issue, it seems there is a flaw in the way OMAP chain handler
> > >> handling the child interrupts.
> > >>
> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
> > >> many devices can trigger wakeup from low power via this common
> > >> interrupt source. The common interrupt source upon wakeup from low
> > >> power state, decodes the source of interrupt and based on that
> > >> source, calls the respective device ISR directly.
> > >>
> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
> > >> is happening even before UART resume and DPM resume has been completed.
> > >> If this is the case, then it is surely asking for trouble. Because not
> > >> just clocks, but even driver state machine is already in suspend state
> > >> when the ISR is called.
> > >
> > > If the driver state machine is in the suspend state when the ISR is
> > > called, then the ISR should realize it is handling a wakeup event
> > > instead of a normal I/O event.  All it needs to do is turn off the
> > > interrupt source; the event itself will be taken care of during the
> > > device's resume callback.
> > >
> > Good point but the ISR is called as a function call and not real
> > hardware event so no need to turn-off the source in the child
> > ISR. Parent ISR will clear the source anyways.
> > 
> > But the intention here is to record the event for the child.
> 
> In that case the ISR only has to record the event.
> 
> > I mean for UART wakeup, the received character should be
> > extracted. If not done, UART will loose that character because
> > the event is lost. So not sure how the event will be taken
> > care during resume callback. Could you elaborate bit more on
> > last comment please?
> 
> The resume callback routine must check to see if an event was recorded.
> If one was, the routine must see whether a character was received while 
> the system was asleep and extract the character from the UART.  (It 
> probably won't hurt to do this even if an event wasn't recorded.)
> 
> Alan Stern
> 

After thinking about this problem and looking at possible ways to fix
it, I am planning to change the PRCM chain handler to be a driver, which
gets suspended along with the rest of the system. This means the PRCM
interrupt would get disabled also during this time, and it would be
enabled in the driver->complete() call, which should happen after rest
of the drivers have been able to enable their PM runtime in the
driver->resume() call chain. Do you see any problems with this approach?
The only issue I am seeing myself is if some driver decides to do
resume() in the complete() pm-op and potentially screwing the ordering
here...

-Tero



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Tero Kristo]

Hi,

On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
> On Sat, 27 Aug 2011, Santosh wrote:
> 
> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> > > On Sat, 27 Aug 2011, Santosh wrote:
> > >
> > >> I might be wrong here, but after discussion with Govindraj on this
> > >> issue, it seems there is a flaw in the way OMAP chain handler
> > >> handling the child interrupts.
> > >>
> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
> > >> many devices can trigger wakeup from low power via this common
> > >> interrupt source. The common interrupt source upon wakeup from low
> > >> power state, decodes the source of interrupt and based on that
> > >> source, calls the respective device ISR directly.
> > >>
> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
> > >> is happening even before UART resume and DPM resume has been completed.
> > >> If this is the case, then it is surely asking for trouble. Because not
> > >> just clocks, but even driver state machine is already in suspend state
> > >> when the ISR is called.
> > >
> > > If the driver state machine is in the suspend state when the ISR is
> > > called, then the ISR should realize it is handling a wakeup event
> > > instead of a normal I/O event.  All it needs to do is turn off the
> > > interrupt source; the event itself will be taken care of during the
> > > device's resume callback.
> > >
> > Good point but the ISR is called as a function call and not real
> > hardware event so no need to turn-off the source in the child
> > ISR. Parent ISR will clear the source anyways.
> > 
> > But the intention here is to record the event for the child.
> 
> In that case the ISR only has to record the event.
> 
> > I mean for UART wakeup, the received character should be
> > extracted. If not done, UART will loose that character because
> > the event is lost. So not sure how the event will be taken
> > care during resume callback. Could you elaborate bit more on
> > last comment please?
> 
> The resume callback routine must check to see if an event was recorded.
> If one was, the routine must see whether a character was received while 
> the system was asleep and extract the character from the UART.  (It 
> probably won't hurt to do this even if an event wasn't recorded.)
> 
> Alan Stern
> 

After thinking about this problem and looking at possible ways to fix
it, I am planning to change the PRCM chain handler to be a driver, which
gets suspended along with the rest of the system. This means the PRCM
interrupt would get disabled also during this time, and it would be
enabled in the driver->complete() call, which should happen after rest
of the drivers have been able to enable their PM runtime in the
driver->resume() call chain. Do you see any problems with this approach?
The only issue I am seeing myself is if some driver decides to do
resume() in the complete() pm-op and potentially screwing the ordering
here...

-Tero



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> Hi,
>
> On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
>> On Sat, 27 Aug 2011, Santosh wrote:
>> 
>> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
>> > > On Sat, 27 Aug 2011, Santosh wrote:
>> > >
>> > >> I might be wrong here, but after discussion with Govindraj on this
>> > >> issue, it seems there is a flaw in the way OMAP chain handler
>> > >> handling the child interrupts.
>> > >>
>> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
>> > >> many devices can trigger wakeup from low power via this common
>> > >> interrupt source. The common interrupt source upon wakeup from low
>> > >> power state, decodes the source of interrupt and based on that
>> > >> source, calls the respective device ISR directly.
>> > >>
>> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
>> > >> is happening even before UART resume and DPM resume has been completed.
>> > >> If this is the case, then it is surely asking for trouble. Because not
>> > >> just clocks, but even driver state machine is already in suspend state
>> > >> when the ISR is called.
>> > >
>> > > If the driver state machine is in the suspend state when the ISR is
>> > > called, then the ISR should realize it is handling a wakeup event
>> > > instead of a normal I/O event.  All it needs to do is turn off the
>> > > interrupt source; the event itself will be taken care of during the
>> > > device's resume callback.
>> > >
>> > Good point but the ISR is called as a function call and not real
>> > hardware event so no need to turn-off the source in the child
>> > ISR. Parent ISR will clear the source anyways.
>> > 
>> > But the intention here is to record the event for the child.
>> 
>> In that case the ISR only has to record the event.
>> 
>> > I mean for UART wakeup, the received character should be
>> > extracted. If not done, UART will loose that character because
>> > the event is lost. So not sure how the event will be taken
>> > care during resume callback. Could you elaborate bit more on
>> > last comment please?
>> 
>> The resume callback routine must check to see if an event was recorded.
>> If one was, the routine must see whether a character was received while 
>> the system was asleep and extract the character from the UART.  (It 
>> probably won't hurt to do this even if an event wasn't recorded.)
>> 
>> Alan Stern
>> 
>
> After thinking about this problem and looking at possible ways to fix
> it, I am planning to change the PRCM chain handler to be a driver, which
> gets suspended along with the rest of the system. This means the PRCM
> interrupt would get disabled also during this time, and it would be
> enabled in the driver->complete() call, which should happen after rest
> of the drivers have been able to enable their PM runtime in the
> driver->resume() call chain. Do you see any problems with this approach?

How will the system wakeup from retention or off-mode if the PRCM IRQ is
disabled?

Besides that, I don't like this approach because it leaves a rather long
window during which no PRCM-triggered wakeup events can happen.  This is
not good since we also want potential wakeup events that happen *during*
system suspend to be able to cancel the suspend.

> The only issue I am seeing myself is if some driver decides to do
> resume() in the complete() pm-op and potentially screwing the ordering
> here...

Personally, I think Alan's approach is the only scalable approach.  This
has to be handled by the drivers.

If the driver's ISR does a pm_runtime_get_sync() and it fails (in this
case, with -EACCESS since runtime PM is disabled), the driver should
handle that handle as Alan described above.

Kevin

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Tero Kristo]

On Wed, 2011-09-07 at 19:59 +0200, Hilman, Kevin wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > Hi,
> >
> > On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
> >> On Sat, 27 Aug 2011, Santosh wrote:
> >> 
> >> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> >> > > On Sat, 27 Aug 2011, Santosh wrote:
> >> > >
> >> > >> I might be wrong here, but after discussion with Govindraj on this
> >> > >> issue, it seems there is a flaw in the way OMAP chain handler
> >> > >> handling the child interrupts.
> >> > >>
> >> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
> >> > >> many devices can trigger wakeup from low power via this common
> >> > >> interrupt source. The common interrupt source upon wakeup from low
> >> > >> power state, decodes the source of interrupt and based on that
> >> > >> source, calls the respective device ISR directly.
> >> > >>
> >> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
> >> > >> is happening even before UART resume and DPM resume has been completed.
> >> > >> If this is the case, then it is surely asking for trouble. Because not
> >> > >> just clocks, but even driver state machine is already in suspend state
> >> > >> when the ISR is called.
> >> > >
> >> > > If the driver state machine is in the suspend state when the ISR is
> >> > > called, then the ISR should realize it is handling a wakeup event
> >> > > instead of a normal I/O event.  All it needs to do is turn off the
> >> > > interrupt source; the event itself will be taken care of during the
> >> > > device's resume callback.
> >> > >
> >> > Good point but the ISR is called as a function call and not real
> >> > hardware event so no need to turn-off the source in the child
> >> > ISR. Parent ISR will clear the source anyways.
> >> > 
> >> > But the intention here is to record the event for the child.
> >> 
> >> In that case the ISR only has to record the event.
> >> 
> >> > I mean for UART wakeup, the received character should be
> >> > extracted. If not done, UART will loose that character because
> >> > the event is lost. So not sure how the event will be taken
> >> > care during resume callback. Could you elaborate bit more on
> >> > last comment please?
> >> 
> >> The resume callback routine must check to see if an event was recorded.
> >> If one was, the routine must see whether a character was received while 
> >> the system was asleep and extract the character from the UART.  (It 
> >> probably won't hurt to do this even if an event wasn't recorded.)
> >> 
> >> Alan Stern
> >> 
> >
> > After thinking about this problem and looking at possible ways to fix
> > it, I am planning to change the PRCM chain handler to be a driver, which
> > gets suspended along with the rest of the system. This means the PRCM
> > interrupt would get disabled also during this time, and it would be
> > enabled in the driver->complete() call, which should happen after rest
> > of the drivers have been able to enable their PM runtime in the
> > driver->resume() call chain. Do you see any problems with this approach?
> 
> How will the system wakeup from retention or off-mode if the PRCM IRQ is
> disabled?

This is actually some sort of an issue with retention if we disable PRCM
irq completely, off works purely with wakeup signals as we come out of
reset. Anyway, I did some experimentation with this, and OMAP3 is able
to wake up if we leave WKUP irq up, but disable IO chain irq. IO chain
events seem to trigger a WKUP event also always, we just postpone the
processing of IO chain until later. I had to also split the wakeup
clearing for OMAP3 into 2 parts, one part handles wakeups and another IO
chain. Currently both IO chain and WKUP are acked by the same handler.

> Besides that, I don't like this approach because it leaves a rather long
> window during which no PRCM-triggered wakeup events can happen.  This is
> not good since we also want potential wakeup events that happen *during*
> system suspend to be able to cancel the suspend.

This should also be taken care of by the approach described above.

> 
> > The only issue I am seeing myself is if some driver decides to do
> > resume() in the complete() pm-op and potentially screwing the ordering
> > here...
> 
> Personally, I think Alan's approach is the only scalable approach.  This
> has to be handled by the drivers.
> 
> If the driver's ISR does a pm_runtime_get_sync() and it fails (in this
> case, with -EACCESS since runtime PM is disabled), the driver should
> handle that handle as Alan described above.

Yea I think this probably needs to be done anyway, at least on some
cases. The PRCM chain handler approach might be able to solve most of
the problems though. I think I will post what I have anyway for comments
hopefully later today, so you can have a look and say what you think.

-Tero



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Tero Kristo]

On Wed, 2011-09-07 at 19:59 +0200, Hilman, Kevin wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > Hi,
> >
> > On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
> >> On Sat, 27 Aug 2011, Santosh wrote:
> >> 
> >> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> >> > > On Sat, 27 Aug 2011, Santosh wrote:
> >> > >
> >> > >> I might be wrong here, but after discussion with Govindraj on this
> >> > >> issue, it seems there is a flaw in the way OMAP chain handler
> >> > >> handling the child interrupts.
> >> > >>
> >> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
> >> > >> many devices can trigger wakeup from low power via this common
> >> > >> interrupt source. The common interrupt source upon wakeup from low
> >> > >> power state, decodes the source of interrupt and based on that
> >> > >> source, calls the respective device ISR directly.
> >> > >>
> >> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
> >> > >> is happening even before UART resume and DPM resume has been completed.
> >> > >> If this is the case, then it is surely asking for trouble. Because not
> >> > >> just clocks, but even driver state machine is already in suspend state
> >> > >> when the ISR is called.
> >> > >
> >> > > If the driver state machine is in the suspend state when the ISR is
> >> > > called, then the ISR should realize it is handling a wakeup event
> >> > > instead of a normal I/O event.  All it needs to do is turn off the
> >> > > interrupt source; the event itself will be taken care of during the
> >> > > device's resume callback.
> >> > >
> >> > Good point but the ISR is called as a function call and not real
> >> > hardware event so no need to turn-off the source in the child
> >> > ISR. Parent ISR will clear the source anyways.
> >> > 
> >> > But the intention here is to record the event for the child.
> >> 
> >> In that case the ISR only has to record the event.
> >> 
> >> > I mean for UART wakeup, the received character should be
> >> > extracted. If not done, UART will loose that character because
> >> > the event is lost. So not sure how the event will be taken
> >> > care during resume callback. Could you elaborate bit more on
> >> > last comment please?
> >> 
> >> The resume callback routine must check to see if an event was recorded.
> >> If one was, the routine must see whether a character was received while 
> >> the system was asleep and extract the character from the UART.  (It 
> >> probably won't hurt to do this even if an event wasn't recorded.)
> >> 
> >> Alan Stern
> >> 
> >
> > After thinking about this problem and looking at possible ways to fix
> > it, I am planning to change the PRCM chain handler to be a driver, which
> > gets suspended along with the rest of the system. This means the PRCM
> > interrupt would get disabled also during this time, and it would be
> > enabled in the driver->complete() call, which should happen after rest
> > of the drivers have been able to enable their PM runtime in the
> > driver->resume() call chain. Do you see any problems with this approach?
> 
> How will the system wakeup from retention or off-mode if the PRCM IRQ is
> disabled?

This is actually some sort of an issue with retention if we disable PRCM
irq completely, off works purely with wakeup signals as we come out of
reset. Anyway, I did some experimentation with this, and OMAP3 is able
to wake up if we leave WKUP irq up, but disable IO chain irq. IO chain
events seem to trigger a WKUP event also always, we just postpone the
processing of IO chain until later. I had to also split the wakeup
clearing for OMAP3 into 2 parts, one part handles wakeups and another IO
chain. Currently both IO chain and WKUP are acked by the same handler.

> Besides that, I don't like this approach because it leaves a rather long
> window during which no PRCM-triggered wakeup events can happen.  This is
> not good since we also want potential wakeup events that happen *during*
> system suspend to be able to cancel the suspend.

This should also be taken care of by the approach described above.

> 
> > The only issue I am seeing myself is if some driver decides to do
> > resume() in the complete() pm-op and potentially screwing the ordering
> > here...
> 
> Personally, I think Alan's approach is the only scalable approach.  This
> has to be handled by the drivers.
> 
> If the driver's ISR does a pm_runtime_get_sync() and it fails (in this
> case, with -EACCESS since runtime PM is disabled), the driver should
> handle that handle as Alan described above.

Yea I think this probably needs to be done anyway, at least on some
cases. The PRCM chain handler approach might be able to solve most of
the problems though. I think I will post what I have anyway for comments
hopefully later today, so you can have a look and say what you think.

-Tero



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki
 

Re: [linux-pm] Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Kevin Hilman]

Hi Tero,

Tero Kristo <t-kristo@ti.com> writes:

> On Wed, 2011-09-07 at 19:59 +0200, Hilman, Kevin wrote:
>> Tero Kristo <t-kristo@ti.com> writes:

[...]

>> > After thinking about this problem and looking at possible ways to fix
>> > it, I am planning to change the PRCM chain handler to be a driver, which
>> > gets suspended along with the rest of the system. This means the PRCM
>> > interrupt would get disabled also during this time, and it would be
>> > enabled in the driver->complete() call, which should happen after rest
>> > of the drivers have been able to enable their PM runtime in the
>> > driver->resume() call chain. Do you see any problems with this approach?
>> 
>> How will the system wakeup from retention or off-mode if the PRCM IRQ is
>> disabled?
>
> This is actually some sort of an issue with retention if we disable PRCM
> irq completely, off works purely with wakeup signals as we come out of
> reset. Anyway, I did some experimentation with this, and OMAP3 is able
> to wake up if we leave WKUP irq up, but disable IO chain irq. IO chain
> events seem to trigger a WKUP event also always, we just postpone the
> processing of IO chain until later. I had to also split the wakeup
> clearing for OMAP3 into 2 parts, one part handles wakeups and another IO
> chain. Currently both IO chain and WKUP are acked by the same handler.

Here's another option, which is kind of a hybrid of what's been
discussed so far.

The PRCM driver would leave the IRQs enabled during suspend, but would
just delay delivering them to the drivers.  IOW, handle/clear the PRCM
IRQ normally, but delay delivery of the *device* IRQ until the
->complete callback of the PRCM driver.

Doing this ensures all the drivers are resumed before any device IRQ is
delivered, and doesn't require any additional queuing of events in the
drivers.

Kevin

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Kevin Hilman]

Hi Tero,

Tero Kristo <t-kristo@ti.com> writes:

> On Wed, 2011-09-07 at 19:59 +0200, Hilman, Kevin wrote:
>> Tero Kristo <t-kristo@ti.com> writes:

[...]

>> > After thinking about this problem and looking at possible ways to fix
>> > it, I am planning to change the PRCM chain handler to be a driver, which
>> > gets suspended along with the rest of the system. This means the PRCM
>> > interrupt would get disabled also during this time, and it would be
>> > enabled in the driver->complete() call, which should happen after rest
>> > of the drivers have been able to enable their PM runtime in the
>> > driver->resume() call chain. Do you see any problems with this approach?
>> 
>> How will the system wakeup from retention or off-mode if the PRCM IRQ is
>> disabled?
>
> This is actually some sort of an issue with retention if we disable PRCM
> irq completely, off works purely with wakeup signals as we come out of
> reset. Anyway, I did some experimentation with this, and OMAP3 is able
> to wake up if we leave WKUP irq up, but disable IO chain irq. IO chain
> events seem to trigger a WKUP event also always, we just postpone the
> processing of IO chain until later. I had to also split the wakeup
> clearing for OMAP3 into 2 parts, one part handles wakeups and another IO
> chain. Currently both IO chain and WKUP are acked by the same handler.

Here's another option, which is kind of a hybrid of what's been
discussed so far.

The PRCM driver would leave the IRQs enabled during suspend, but would
just delay delivering them to the drivers.  IOW, handle/clear the PRCM
IRQ normally, but delay delivery of the *device* IRQ until the
->complete callback of the PRCM driver.

Doing this ensures all the drivers are resumed before any device IRQ is
delivered, and doesn't require any additional queuing of events in the
drivers.

Kevin

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> Hi,
>
> On Sat, 2011-08-27 at 21:42 +0200, Alan Stern wrote:
>> On Sat, 27 Aug 2011, Santosh wrote:
>> 
>> > On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
>> > > On Sat, 27 Aug 2011, Santosh wrote:
>> > >
>> > >> I might be wrong here, but after discussion with Govindraj on this
>> > >> issue, it seems there is a flaw in the way OMAP chain handler
>> > >> handling the child interrupts.
>> > >>
>> > >> On OMAP, we have special interrupt wakeup source at PRCM level and
>> > >> many devices can trigger wakeup from low power via this common
>> > >> interrupt source. The common interrupt source upon wakeup from low
>> > >> power state, decodes the source of interrupt and based on that
>> > >> source, calls the respective device ISR directly.
>> > >>
>> > >> The issue I see here is, the ISR on _a_ device (UART in this case)
>> > >> is happening even before UART resume and DPM resume has been completed.
>> > >> If this is the case, then it is surely asking for trouble. Because not
>> > >> just clocks, but even driver state machine is already in suspend state
>> > >> when the ISR is called.
>> > >
>> > > If the driver state machine is in the suspend state when the ISR is
>> > > called, then the ISR should realize it is handling a wakeup event
>> > > instead of a normal I/O event.  All it needs to do is turn off the
>> > > interrupt source; the event itself will be taken care of during the
>> > > device's resume callback.
>> > >
>> > Good point but the ISR is called as a function call and not real
>> > hardware event so no need to turn-off the source in the child
>> > ISR. Parent ISR will clear the source anyways.
>> > 
>> > But the intention here is to record the event for the child.
>> 
>> In that case the ISR only has to record the event.
>> 
>> > I mean for UART wakeup, the received character should be
>> > extracted. If not done, UART will loose that character because
>> > the event is lost. So not sure how the event will be taken
>> > care during resume callback. Could you elaborate bit more on
>> > last comment please?
>> 
>> The resume callback routine must check to see if an event was recorded.
>> If one was, the routine must see whether a character was received while 
>> the system was asleep and extract the character from the UART.  (It 
>> probably won't hurt to do this even if an event wasn't recorded.)
>> 
>> Alan Stern
>> 
>
> After thinking about this problem and looking at possible ways to fix
> it, I am planning to change the PRCM chain handler to be a driver, which
> gets suspended along with the rest of the system. This means the PRCM
> interrupt would get disabled also during this time, and it would be
> enabled in the driver->complete() call, which should happen after rest
> of the drivers have been able to enable their PM runtime in the
> driver->resume() call chain. Do you see any problems with this approach?

How will the system wakeup from retention or off-mode if the PRCM IRQ is
disabled?

Besides that, I don't like this approach because it leaves a rather long
window during which no PRCM-triggered wakeup events can happen.  This is
not good since we also want potential wakeup events that happen *during*
system suspend to be able to cancel the suspend.

> The only issue I am seeing myself is if some driver decides to do
> resume() in the complete() pm-op and potentially screwing the ordering
> here...

Personally, I think Alan's approach is the only scalable approach.  This
has to be handled by the drivers.

If the driver's ISR does a pm_runtime_get_sync() and it fails (in this
case, with -EACCESS since runtime PM is disabled), the driver should
handle that handle as Alan described above.

Kevin

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Sat, 27 Aug 2011, Santosh wrote:

> On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> > On Sat, 27 Aug 2011, Santosh wrote:
> >
> >> I might be wrong here, but after discussion with Govindraj on this
> >> issue, it seems there is a flaw in the way OMAP chain handler
> >> handling the child interrupts.
> >>
> >> On OMAP, we have special interrupt wakeup source at PRCM level and
> >> many devices can trigger wakeup from low power via this common
> >> interrupt source. The common interrupt source upon wakeup from low
> >> power state, decodes the source of interrupt and based on that
> >> source, calls the respective device ISR directly.
> >>
> >> The issue I see here is, the ISR on _a_ device (UART in this case)
> >> is happening even before UART resume and DPM resume has been completed.
> >> If this is the case, then it is surely asking for trouble. Because not
> >> just clocks, but even driver state machine is already in suspend state
> >> when the ISR is called.
> >
> > If the driver state machine is in the suspend state when the ISR is
> > called, then the ISR should realize it is handling a wakeup event
> > instead of a normal I/O event.  All it needs to do is turn off the
> > interrupt source; the event itself will be taken care of during the
> > device's resume callback.
> >
> Good point but the ISR is called as a function call and not real
> hardware event so no need to turn-off the source in the child
> ISR. Parent ISR will clear the source anyways.
> 
> But the intention here is to record the event for the child.

In that case the ISR only has to record the event.

> I mean for UART wakeup, the received character should be
> extracted. If not done, UART will loose that character because
> the event is lost. So not sure how the event will be taken
> care during resume callback. Could you elaborate bit more on
> last comment please?

The resume callback routine must check to see if an event was recorded.
If one was, the routine must see whether a character was received while 
the system was asleep and extract the character from the UART.  (It 
probably won't hurt to do this even if an event wasn't recorded.)

Alan Stern

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Santosh]

On Saturday 27 August 2011 07:31 PM, Alan Stern wrote:
> On Sat, 27 Aug 2011, Santosh wrote:
>
>> I might be wrong here, but after discussion with Govindraj on this
>> issue, it seems there is a flaw in the way OMAP chain handler
>> handling the child interrupts.
>>
>> On OMAP, we have special interrupt wakeup source at PRCM level and
>> many devices can trigger wakeup from low power via this common
>> interrupt source. The common interrupt source upon wakeup from low
>> power state, decodes the source of interrupt and based on that
>> source, calls the respective device ISR directly.
>>
>> The issue I see here is, the ISR on _a_ device (UART in this case)
>> is happening even before UART resume and DPM resume has been completed.
>> If this is the case, then it is surely asking for trouble. Because not
>> just clocks, but even driver state machine is already in suspend state
>> when the ISR is called.
>
> If the driver state machine is in the suspend state when the ISR is
> called, then the ISR should realize it is handling a wakeup event
> instead of a normal I/O event.  All it needs to do is turn off the
> interrupt source; the event itself will be taken care of during the
> device's resume callback.
>
Good point but the ISR is called as a function call and not real
hardware event so no need to turn-off the source in the child
ISR. Parent ISR will clear the source anyways.

But the intention here is to record the event for the child.
I mean for UART wakeup, the received character should be
extracted. If not done, UART will loose that character because
the event is lost. So not sure how the event will be taken
care during resume callback. Could you elaborate bit more on
last comment please?

Regards
Santosh

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Sat, 27 Aug 2011, Santosh wrote:

> I might be wrong here, but after discussion with Govindraj on this
> issue, it seems there is a flaw in the way OMAP chain handler
> handling the child interrupts.
> 
> On OMAP, we have special interrupt wakeup source at PRCM level and
> many devices can trigger wakeup from low power via this common
> interrupt source. The common interrupt source upon wakeup from low
> power state, decodes the source of interrupt and based on that
> source, calls the respective device ISR directly.
> 
> The issue I see here is, the ISR on _a_ device (UART in this case)
> is happening even before UART resume and DPM resume has been completed.
> If this is the case, then it is surely asking for trouble. Because not
> just clocks, but even driver state machine is already in suspend state
> when the ISR is called.

If the driver state machine is in the suspend state when the ISR is
called, then the ISR should realize it is handling a wakeup event
instead of a normal I/O event.  All it needs to do is turn off the
interrupt source; the event itself will be taken care of during the
device's resume callback.

Alan Stern

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Alan Stern]

On Fri, 26 Aug 2011, Govindraj.R wrote:

> Hello,
> 
> During system_wide_suspend pm runtime is disabled.
> I.e. __pm_runtime_disable is called from __device_suspend.
> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
> is called even before device_resume and pm_runtime_enable happens,
> the device irq_handler proceeds to enable clock with runtime API to
> handle wakeup event.
>  
> Wouldn't this result in system wide abort since the pm_runtime is not enabled
> yet from dpm_resume?
> As we end up accessing regs after doing runtime get_sync.
>  
> Looks like this scenario is not handled currently.
> Or Am I missing something here?

I don't have the complete picture, but it seems that the IRQ handler 
needs to check the return code from pm_runtime_get_sync().  If the call 
fails then the handler shouldn't try to access the device registers.

In the case of a genuine wakeup event, the event should be handled 
later on as part of the resume or resume_noirq processing.

However, this does raise a potential problem.  What happens if the 
clocks are needed in order to turn off the IRQ source?  Runtime PM 
won't allow the clocks to be enabled until after interrupts have been 
enabled, and by then it will be too late -- the source will have caused 
an interrupt storm.

Alan Stern

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Rafael J. Wysocki]

On Friday, August 26, 2011, Govindraj.R wrote:
> Hello,
> 
> During system_wide_suspend pm runtime is disabled.
> I.e. __pm_runtime_disable is called from __device_suspend.
> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
> is called even before device_resume and pm_runtime_enable happens,
> the device irq_handler proceeds to enable clock with runtime API to
> handle wakeup event.
>  
> Wouldn't this result in system wide abort since the pm_runtime is not enabled
> yet from dpm_resume?
> As we end up accessing regs after doing runtime get_sync.
>  
> Looks like this scenario is not handled currently.
> Or Am I missing something here?

To be precise, what do you mean by "wakeup interrupt"?

Rafael

Re: Issue: Runtime API usage in wake-up device irq_handler during wakeup from system-wide-suspend.

[Rafael J. Wysocki]

On Friday, August 26, 2011, Govindraj.R wrote:
> Hello,
> 
> During system_wide_suspend pm runtime is disabled.
> I.e. __pm_runtime_disable is called from __device_suspend.
> Now, if a wakeup interrupt is triggered and the wakeup device irq handler
> is called even before device_resume and pm_runtime_enable happens,
> the device irq_handler proceeds to enable clock with runtime API to
> handle wakeup event.
>  
> Wouldn't this result in system wide abort since the pm_runtime is not enabled
> yet from dpm_resume?
> As we end up accessing regs after doing runtime get_sync.
>  
> Looks like this scenario is not handled currently.
> Or Am I missing something here?

To be precise, what do you mean by "wakeup interrupt"?

Rafael