[patch 08/26] arm: Replace various irq_desc accesses

[patch 08/26] arm: Replace various irq_desc accesses

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: arm-replace-various-irq-to-desc-accesses.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140223/b40ca264/attachment.ksh>

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: arm-mmp-remove-idiotic-fiddling-with-irq-internals.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140223/ef9b3db6/attachment.ksh>

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Uwe Kleine-König]

Hi Thomas,

On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> callbacks fiddle pointlessly with the irq actions for no reason except
> for lack of understanding how the wakeup mechanism works.
> 
> On supsend the core disables all interrupts lazily, i.e. it does not
> mask them at the irq controller level. So any interrupt which is
> firing during supsend will mark the corresponding interrupt line as
s/supsend/suspend/ twice
> pending. Just before the core powers down it checks whether there are
> interrupts pending from interrupt lines which are marked as wakeup
> sources and if so it aborts the resume and resends the interrupts.
It's the suspend that is aborted, not the resume.

Other than that your change looks fine.

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[patch 08/26] arm: Replace various irq_desc accesses

[Shawn Guo]

On Sun, Feb 23, 2014 at 09:40:12PM -0000, Thomas Gleixner wrote:
> Use the proper functions. There is no need to fiddle with irq_desc.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
...
>  arch/arm/mach-imx/pm-imx6q.c        |    7 +++----

Acked-by: Shawn Guo <shawn.guo@linaro.org>

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Chao Xie]

On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Hi Thomas,
>
> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>> callbacks fiddle pointlessly with the irq actions for no reason except
>> for lack of understanding how the wakeup mechanism works.
>>
>> On supsend the core disables all interrupts lazily, i.e. it does not
>> mask them at the irq controller level. So any interrupt which is
>> firing during supsend will mark the corresponding interrupt line as
> s/supsend/suspend/ twice
>> pending. Just before the core powers down it checks whether there are
>> interrupts pending from interrupt lines which are marked as wakeup
>> sources and if so it aborts the resume and resends the interrupts.
> It's the suspend that is aborted, not the resume.
>
> Other than that your change looks fine.
>
For pxa910 and MMP2, wake up source only wake up the AP subsystem.
The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
Now the core is still powered down. APMU will check the interrupt
lines, and find
that there are interrupt pending, it will power on the cores.
So if the irq is disabled, even wake up source can wake up AP subsystem, but the
core is still powered down. It will not be powered up by APMU.


> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Haojian Zhuang]

On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
>> Hi Thomas,
>>
>> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>>> callbacks fiddle pointlessly with the irq actions for no reason except
>>> for lack of understanding how the wakeup mechanism works.
>>>
>>> On supsend the core disables all interrupts lazily, i.e. it does not
>>> mask them at the irq controller level. So any interrupt which is
>>> firing during supsend will mark the corresponding interrupt line as
>> s/supsend/suspend/ twice
>>> pending. Just before the core powers down it checks whether there are
>>> interrupts pending from interrupt lines which are marked as wakeup
>>> sources and if so it aborts the resume and resends the interrupts.
>> It's the suspend that is aborted, not the resume.
>>
>> Other than that your change looks fine.
>>
> For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> Now the core is still powered down. APMU will check the interrupt
> lines, and find
> that there are interrupt pending, it will power on the cores.
> So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> core is still powered down. It will not be powered up by APMU.
>

Yes, suspend/resume can't work if the above code is removed.

Interrupt source (logic AND with interrupt mask register) is connected
to MPMU as
wakeup source. If the interrupt is disabled, there's no wakeup source event.

And APMU is waken up by MPMU.

So please don't remove the above code. We must keep these interrupt lines active
to wake up the whole system.

Regards
Haojian

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Thomas Gleixner]

On Mon, 24 Feb 2014, Chao Xie wrote:

> On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hi Thomas,
> >
> > On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> >> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> >> callbacks fiddle pointlessly with the irq actions for no reason except
> >> for lack of understanding how the wakeup mechanism works.
> >>
> >> On supsend the core disables all interrupts lazily, i.e. it does not
> >> mask them at the irq controller level. So any interrupt which is
> >> firing during supsend will mark the corresponding interrupt line as
> > s/supsend/suspend/ twice
> >> pending. Just before the core powers down it checks whether there are
> >> interrupts pending from interrupt lines which are marked as wakeup
> >> sources and if so it aborts the resume and resends the interrupts.
> > It's the suspend that is aborted, not the resume.
> >
> > Other than that your change looks fine.
> >
> For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> Now the core is still powered down. APMU will check the interrupt
> lines, and find
> that there are interrupt pending, it will power on the cores.
> So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> core is still powered down. It will not be powered up by APMU.

The interrupt is NOT disabled at the interrupt chip level. The core does:

suspend_device_irqs() {
  __disable_irq() {
    if (!desc->depth++)
      irq_disable(desc) {
        irq_state_set_disabled(desc);
        if (desc->irq_data.chip->irq_disable) {
          desc->irq_data.chip->irq_disable(&desc->irq_data);
          irq_state_set_masked(desc);
        }

Your chip does not have a chip->irq_disable() callback installed, so
the interrupt is not masked at the controller level. So APMU sees the
interrupt enabled.

APMU does not care about the irq_desc->depth counter and the irq_data
IRQD_DISABLED state bit.

So this hackery is completely pointless.

Thanks,

	tglx

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Thomas Gleixner]

On Mon, 24 Feb 2014, Haojian Zhuang wrote:

> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
> > <u.kleine-koenig@pengutronix.de> wrote:
> >> Hi Thomas,
> >>
> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
> >>> callbacks fiddle pointlessly with the irq actions for no reason except
> >>> for lack of understanding how the wakeup mechanism works.
> >>>
> >>> On supsend the core disables all interrupts lazily, i.e. it does not
> >>> mask them at the irq controller level. So any interrupt which is
> >>> firing during supsend will mark the corresponding interrupt line as
> >> s/supsend/suspend/ twice
> >>> pending. Just before the core powers down it checks whether there are
> >>> interrupts pending from interrupt lines which are marked as wakeup
> >>> sources and if so it aborts the resume and resends the interrupts.
> >> It's the suspend that is aborted, not the resume.
> >>
> >> Other than that your change looks fine.
> >>
> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
> > Now the core is still powered down. APMU will check the interrupt
> > lines, and find
> > that there are interrupt pending, it will power on the cores.
> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
> > core is still powered down. It will not be powered up by APMU.
> >
> 
> Yes, suspend/resume can't work if the above code is removed.
> 
> Interrupt source (logic AND with interrupt mask register) is connected
> to MPMU as
> wakeup source. If the interrupt is disabled, there's no wakeup source event.
> 
> And APMU is waken up by MPMU.
> 
> So please don't remove the above code. We must keep these interrupt lines active
> to wake up the whole system.

They are kept active at the interrupt controller level. You just
refuse to understand how the internals of the interrupt subsystem
work.

And even if you would need this flag, then fiddling with the irq desc
internals is a big NONO. There is a proper way to hand that in.

Thanks,

	tglx

[patch 08/26] arm: Replace various irq_desc accesses

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [140223 18:58]:
> On Sun, Feb 23, 2014 at 09:40:12PM -0000, Thomas Gleixner wrote:
> > Use the proper functions. There is no need to fiddle with irq_desc.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> ...
> >  arch/arm/mach-imx/pm-imx6q.c        |    7 +++----
> 
> Acked-by: Shawn Guo <shawn.guo@linaro.org>

Acked-by: Tony Lindgren <tony@atomide.com> 

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Chao Xie]

On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 24 Feb 2014, Haojian Zhuang wrote:
>
>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
>> > <u.kleine-koenig@pengutronix.de> wrote:
>> >> Hi Thomas,
>> >>
>> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>> >>> callbacks fiddle pointlessly with the irq actions for no reason except
>> >>> for lack of understanding how the wakeup mechanism works.
>> >>>
>> >>> On supsend the core disables all interrupts lazily, i.e. it does not
>> >>> mask them at the irq controller level. So any interrupt which is
>> >>> firing during supsend will mark the corresponding interrupt line as
>> >> s/supsend/suspend/ twice
>> >>> pending. Just before the core powers down it checks whether there are
>> >>> interrupts pending from interrupt lines which are marked as wakeup
>> >>> sources and if so it aborts the resume and resends the interrupts.
>> >> It's the suspend that is aborted, not the resume.
>> >>
>> >> Other than that your change looks fine.
>> >>
>> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
>> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
>> > Now the core is still powered down. APMU will check the interrupt
>> > lines, and find
>> > that there are interrupt pending, it will power on the cores.
>> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
>> > core is still powered down. It will not be powered up by APMU.
>> >
>>
>> Yes, suspend/resume can't work if the above code is removed.
>>
>> Interrupt source (logic AND with interrupt mask register) is connected
>> to MPMU as
>> wakeup source. If the interrupt is disabled, there's no wakeup source event.
>>
>> And APMU is waken up by MPMU.
>>
>> So please don't remove the above code. We must keep these interrupt lines active
>> to wake up the whole system.
>
> They are kept active at the interrupt controller level. You just
> refuse to understand how the internals of the interrupt subsystem
> work.
>
If no irq_disable callback is hooked, when do irq_disable, it will not
actually disable
the interrupt, it will depend on next time when the irq happens, the
handler will first mask
the interrupt as this interrupt never happens.
So after system suspended, the interrupt happens, but the device
driver will not recieve this interrupt
because it is masked.
It results in that the device driver miss a important interrupt which
related to something need to be
handled. If user application for example android has power managment
daemon. It will find that nothing
to handle, it will make the system enter suspend again.

> And even if you would need this flag, then fiddling with the irq desc
> internals is a big NONO. There is a proper way to hand that in.
>

So can you suggest the proper way to handle it? Thanks.

> Thanks,
>
>         tglx
>

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Haojian Zhuang]

On Thu, Feb 27, 2014 at 9:37 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Mon, Feb 24, 2014 at 7:31 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Mon, 24 Feb 2014, Haojian Zhuang wrote:
>>
>>> On Mon, Feb 24, 2014 at 2:07 PM, Chao Xie <xiechao.mail@gmail.com> wrote:
>>> > On Mon, Feb 24, 2014 at 7:17 AM, Uwe Kleine-K?nig
>>> > <u.kleine-koenig@pengutronix.de> wrote:
>>> >> Hi Thomas,
>>> >>
>>> >> On Sun, Feb 23, 2014 at 09:40:13PM -0000, Thomas Gleixner wrote:
>>> >>> The pm-mmp2 and pm-pxa910 power management related irq_set_wake
>>> >>> callbacks fiddle pointlessly with the irq actions for no reason except
>>> >>> for lack of understanding how the wakeup mechanism works.
>>> >>>
>>> >>> On supsend the core disables all interrupts lazily, i.e. it does not
>>> >>> mask them at the irq controller level. So any interrupt which is
>>> >>> firing during supsend will mark the corresponding interrupt line as
>>> >> s/supsend/suspend/ twice
>>> >>> pending. Just before the core powers down it checks whether there are
>>> >>> interrupts pending from interrupt lines which are marked as wakeup
>>> >>> sources and if so it aborts the resume and resends the interrupts.
>>> >> It's the suspend that is aborted, not the resume.
>>> >>
>>> >> Other than that your change looks fine.
>>> >>
>>> > For pxa910 and MMP2, wake up source only wake up the AP subsystem.
>>> > The AP subsystem includes the APMU(AP Power Mangament Unit) and cores.
>>> > Now the core is still powered down. APMU will check the interrupt
>>> > lines, and find
>>> > that there are interrupt pending, it will power on the cores.
>>> > So if the irq is disabled, even wake up source can wake up AP subsystem, but the
>>> > core is still powered down. It will not be powered up by APMU.
>>> >
>>>
>>> Yes, suspend/resume can't work if the above code is removed.
>>>
>>> Interrupt source (logic AND with interrupt mask register) is connected
>>> to MPMU as
>>> wakeup source. If the interrupt is disabled, there's no wakeup source event.
>>>
>>> And APMU is waken up by MPMU.
>>>
>>> So please don't remove the above code. We must keep these interrupt lines active
>>> to wake up the whole system.
>>
>> They are kept active at the interrupt controller level. You just
>> refuse to understand how the internals of the interrupt subsystem
>> work.
>>
> If no irq_disable callback is hooked, when do irq_disable, it will not
> actually disable
> the interrupt, it will depend on next time when the irq happens, the
> handler will first mask
> the interrupt as this interrupt never happens.
> So after system suspended, the interrupt happens, but the device
> driver will not recieve this interrupt
> because it is masked.
> It results in that the device driver miss a important interrupt which
> related to something need to be
> handled. If user application for example android has power managment
> daemon. It will find that nothing
> to handle, it will make the system enter suspend again.
>
Let me list the logic to make it easier to understand.

suspend_enter()
  --> dpm_suspend_end()
           --> dpm_suspend_noirq()
                    --> suspend_device_irqs()
                             --> __disable_irq()
                                     --> set IRQS_SUSPENDED && call
irq_disable() if necessary
  --> syscore_suspend()
          --> check_wakeup_irqs()
               If there's no pending irq in suspend process &&
IRQS_SUSPENDED is set,
               then mask the irq.

Yes, we didn't implement disable_irq(). But we must implement mask_irq().

So system suspends. Then system will never be waken up by this irq any
more since
it's masked.


>> And even if you would need this flag, then fiddling with the irq desc
>> internals is a big NONO. There is a proper way to hand that in.
>>
>
> So can you suggest the proper way to handle it? Thanks.
>
>> Thanks,
>>
>>         tglx
>>

[patch 09/26] arm: mmp: Remove pointless fiddling with irq internals

[Thomas Gleixner]

On Thu, 27 Feb 2014, Haojian Zhuang wrote:
> Let me list the logic to make it easier to understand.
> 
> suspend_enter()
>   --> dpm_suspend_end()
>            --> dpm_suspend_noirq()
>                     --> suspend_device_irqs()
>                              --> __disable_irq()
>                                      --> set IRQS_SUSPENDED && call
> irq_disable() if necessary
>   --> syscore_suspend()
>           --> check_wakeup_irqs()
>                If there's no pending irq in suspend process &&
> IRQS_SUSPENDED is set,
>                then mask the irq.
> 
> Yes, we didn't implement disable_irq(). But we must implement mask_irq().
> 
> So system suspends. Then system will never be waken up by this irq any
> more since
> it's masked.

This is so wrong, it's not even funny anymore.

check_wakeup_irqs()
{
	for_each_irq_desc(irq, desc) {
                if (irqd_is_wakeup_set(&desc->irq_data)) {
                        if (desc->depth == 1 && desc->istate & IRQS_PENDING)
                                return -EBUSY;
                        continue;
                }

So all interrupt lines which have been marked as wakeup sources are
not masked. And we only mask the other lines if the irq chip has the
IRQCHIP_MASK_ON_SUSPEND flag set.

                if (desc->istate & IRQS_SUSPENDED &&
                    irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
                        mask_irq(desc);

}

The interrupts which can wake up your system fall into the
irqd_is_wakeup_set() clause. So nothing masks the interrupts at these
interrupt controller level. Your chip does not have the
IRQCHIP_MASK_ON_SUSPEND flag set either, so not a single interrupt
line gets masked.

The only thing you do with your hackery is to avoid that the interrupt
is marked disabled. And that means that you violate the rules of the
suspend logic.

We lazy disable the interrupts when we go into suspend in order to
abort the suspend when a wakeup interrupt happens between the
suspend_device_irqs() and check_wakeup_irqs(). Your hackery allows the
system to handle the interrupt, so we have no indication that the
suspend should be aborted.

You insist, that the interrupt line is masked at the irq chip level on
suspend, but you completely fail to explain how this should
happen. All you came up with so far is handwaving and a proper proof
of incompetence.

I'm really start to get grumpy about this utter waste of time.

Thanks,

	tglx

[patch 08/26] arm: Replace various irq_desc accesses

[Arnd Bergmann]

On Sunday 23 February 2014, Thomas Gleixner wrote:
> --- tip.orig/arch/arm/mach-omap1/ams-delta-fiq.c
> +++ tip/arch/arm/mach-omap1/ams-delta-fiq.c
> @@ -44,13 +44,10 @@ static unsigned int irq_counter[16];
>  
>  static irqreturn_t deferred_fiq(int irq, void *dev_id)
>  {
> -       struct irq_desc *irq_desc;
> -       struct irq_chip *irq_chip = NULL;
>         int gpio, irq_num, fiq_count;
> +       struct irq_chip *irq_chip;
>  
> -       irq_desc = irq_to_desc(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK));
> -       if (irq_desc)
> -               irq_chip = irq_desc->irq_data.chip;
> +       irq_chip = irq_get_irq_chip(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK));

I got a compile error because irq_get_irq_chip() doesn't exist. I suppose
you meant irq_get_chip.

	Arnd