resume regression in 2.6.37

resume regression in 2.6.37

[Sven Neumann]

Hi,

we are using devices based on an PXA300 and are successfully using the
2.6.36.2 kernel (vanilla, basically unpatched). You can find the
platform-specific code that describes our devices
in /arch/arm/mach-pxa/raumfeld.c.

Now I've tried to update the kernel to 2.6.37. After fixing the already
reported NULL pointer dereference on bootup in pxa3xx_nand_probe(), the
device seems to work fine until I suspend it and then try to wake it up
from suspend. Suspend seems to work fine, no warnings whatsoever.
However there is absolutely no sign of resume.

I've used git bisect to track this down to the following commit:


commit 2f7e99bb9be6a2d8d7b808dc86037710cc8b7bf1
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Mon Sep 27 12:45:50 2010 +0000

    genirq: Provide compat handling for chip->set_wake()
    
    Wrap the old chip function set_wake() until the migration is
    complete and the old chip functions are removed.
    
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Cc: Peter Zijlstra <peterz@infradead.org>
    LKML-Reference: <20100927121842.927527393@linutronix.de>
    Reviewed-by: H. Peter Anvin <hpa@zytor.com>
    Reviewed-by: Ingo Molnar <mingo@elte.hu>


Any idea on how to proceed from here? I don't think I can just revert
this commit and expect things to work, right?


Regards,
Sven


-- 
Sven Neumann
Head of RAUMFELD Software Development

Lautsprecher Teufel GmbH | Bülowstr. 66 | 10783 Berlin | Germany
Tel: +49 (0)30-300 930 153 | s.neumann@raumfeld.com

Re: resume regression in 2.6.37

[Thomas Gleixner]

On Thu, 13 Jan 2011, Sven Neumann wrote:
> Now I've tried to update the kernel to 2.6.37. After fixing the already
> reported NULL pointer dereference on bootup in pxa3xx_nand_probe(), the
> device seems to work fine until I suspend it and then try to wake it up
> from suspend. Suspend seems to work fine, no warnings whatsoever.
> However there is absolutely no sign of resume.
> 
> I've used git bisect to track this down to the following commit:
> 
> 
> commit 2f7e99bb9be6a2d8d7b808dc86037710cc8b7bf1
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Mon Sep 27 12:45:50 2010 +0000
> 
>     genirq: Provide compat handling for chip->set_wake()
>     
>     Wrap the old chip function set_wake() until the migration is
>     complete and the old chip functions are removed.
>     
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     LKML-Reference: <20100927121842.927527393@linutronix.de>
>     Reviewed-by: H. Peter Anvin <hpa@zytor.com>
>     Reviewed-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> Any idea on how to proceed from here? I don't think I can just revert
> this commit and expect things to work, right?

Well, I somehow doubt, that this is the real culprit. The patch does
not change anything of the set_wake() functionality. It's a functional
equivivalent wrapper, nothing else.

Can you revert it on top of 2.6.37 and verify that it helps ?

Thanks,

	tglx

Re: resume regression in 2.6.37

[Sven Neumann]

On Thu, 2011-01-13 at 17:28 +0100, Thomas Gleixner wrote:
> On Thu, 13 Jan 2011, Sven Neumann wrote:
> > Now I've tried to update the kernel to 2.6.37. After fixing the already
> > reported NULL pointer dereference on bootup in pxa3xx_nand_probe(), the
> > device seems to work fine until I suspend it and then try to wake it up
> > from suspend. Suspend seems to work fine, no warnings whatsoever.
> > However there is absolutely no sign of resume.
> > 
> > I've used git bisect to track this down to the following commit:
> > 
> > 
> > commit 2f7e99bb9be6a2d8d7b808dc86037710cc8b7bf1
> > Author: Thomas Gleixner <tglx@linutronix.de>
> > Date:   Mon Sep 27 12:45:50 2010 +0000
> > 
> >     genirq: Provide compat handling for chip->set_wake()
> >     
> >     Wrap the old chip function set_wake() until the migration is
> >     complete and the old chip functions are removed.
> >     
> >     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     LKML-Reference: <20100927121842.927527393@linutronix.de>
> >     Reviewed-by: H. Peter Anvin <hpa@zytor.com>
> >     Reviewed-by: Ingo Molnar <mingo@elte.hu>
> > 
> > 
> > Any idea on how to proceed from here? I don't think I can just revert
> > this commit and expect things to work, right?
> 
> Well, I somehow doubt, that this is the real culprit. The patch does
> not change anything of the set_wake() functionality. It's a functional
> equivivalent wrapper, nothing else.
> 
> Can you revert it on top of 2.6.37 and verify that it helps ?

I've done that now and while it doesn't solve the problem completely, I
can definitely say that reverting the change does make a difference. 

Without the revert there is absolutely no sign of resume. With the
change reverted I can see that the PXA powers up again, there's an LED
showing that the USB controller has power again. Unfortunately there's
seems to be another problem and the resume doesn't complete. I've tried
to get console output by using no_console_suspend, but there's just
gibberish on the console after resume :(

Any idea on how to proceed from here? I could perhaps ask our hardware
engineer to try find out where exactly we are stuck in the resume. But
I'd like to avoid that if possible.


Thanks,
Sven

Re: resume regression in 2.6.37

[Thomas Gleixner]

On Thu, 13 Jan 2011, Sven Neumann wrote:
> On Thu, 2011-01-13 at 17:28 +0100, Thomas Gleixner wrote:
> > On Thu, 13 Jan 2011, Sven Neumann wrote:
> > > Any idea on how to proceed from here? I don't think I can just revert
> > > this commit and expect things to work, right?
> > 
> > Well, I somehow doubt, that this is the real culprit. The patch does
> > not change anything of the set_wake() functionality. It's a functional
> > equivivalent wrapper, nothing else.
> > 
> > Can you revert it on top of 2.6.37 and verify that it helps ?
> 
> I've done that now and while it doesn't solve the problem completely, I
> can definitely say that reverting the change does make a difference. 

Hmm. I have to admit that this confuses the hell out of me.

> Without the revert there is absolutely no sign of resume. With the
> change reverted I can see that the PXA powers up again, there's an LED
> showing that the USB controller has power again. Unfortunately there's
> seems to be another problem and the resume doesn't complete. I've tried
> to get console output by using no_console_suspend, but there's just
> gibberish on the console after resume :(
> 
> Any idea on how to proceed from here? I could perhaps ask our hardware
> engineer to try find out where exactly we are stuck in the resume. But
> I'd like to avoid that if possible.

Can you stick a printk into the set_wake() function of that irq chip
and print the irq and on arguments and the return value . Run that
with both stock 2.6.37 and the patch reverted.

Thanks,

	tglx

Re: resume regression in 2.6.37

[Sven Neumann]

On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:

> > Without the revert there is absolutely no sign of resume. With the
> > change reverted I can see that the PXA powers up again, there's an LED
> > showing that the USB controller has power again. Unfortunately there's
> > seems to be another problem and the resume doesn't complete. I've tried
> > to get console output by using no_console_suspend, but there's just
> > gibberish on the console after resume :(
> > 
> > Any idea on how to proceed from here? I could perhaps ask our hardware
> > engineer to try find out where exactly we are stuck in the resume. But
> > I'd like to avoid that if possible.
> 
> Can you stick a printk into the set_wake() function of that irq chip
> and print the irq and on arguments and the return value . Run that
> with both stock 2.6.37 and the patch reverted.

pxa3xx_set_wake(31, 1) returns 0

Same result with stock 2.6.37 and the patch reverted. Except that
without the patch reverted, the device doesn't power up again.


Sven

Re: resume regression in 2.6.37

[Thomas Gleixner]

On Mon, 17 Jan 2011, Sven Neumann wrote:

> On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:
> 
> > > Without the revert there is absolutely no sign of resume. With the
> > > change reverted I can see that the PXA powers up again, there's an LED
> > > showing that the USB controller has power again. Unfortunately there's
> > > seems to be another problem and the resume doesn't complete. I've tried
> > > to get console output by using no_console_suspend, but there's just
> > > gibberish on the console after resume :(
> > > 
> > > Any idea on how to proceed from here? I could perhaps ask our hardware
> > > engineer to try find out where exactly we are stuck in the resume. But
> > > I'd like to avoid that if possible.
> > 
> > Can you stick a printk into the set_wake() function of that irq chip
> > and print the irq and on arguments and the return value . Run that
> > with both stock 2.6.37 and the patch reverted.
> 
> pxa3xx_set_wake(31, 1) returns 0
> 
> Same result with stock 2.6.37 and the patch reverted. Except that
> without the patch reverted, the device doesn't power up again.

Ok, can we agree that the patch has no functional impact on the
set_wake function? And I don't see a reason why reverting that patch
results in a working resume. That does not make sense at all. Which
compiler version are you using ?

Thanks,

	tglx

Re: resume regression in 2.6.37

[Sven Neumann]

On Mon, 2011-01-17 at 13:40 +0100, Thomas Gleixner wrote:
> On Mon, 17 Jan 2011, Sven Neumann wrote:
> 
> > On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:
> > 
> > > > Without the revert there is absolutely no sign of resume. With the
> > > > change reverted I can see that the PXA powers up again, there's an LED
> > > > showing that the USB controller has power again. Unfortunately there's
> > > > seems to be another problem and the resume doesn't complete. I've tried
> > > > to get console output by using no_console_suspend, but there's just
> > > > gibberish on the console after resume :(
> > > > 
> > > > Any idea on how to proceed from here? I could perhaps ask our hardware
> > > > engineer to try find out where exactly we are stuck in the resume. But
> > > > I'd like to avoid that if possible.
> > > 
> > > Can you stick a printk into the set_wake() function of that irq chip
> > > and print the irq and on arguments and the return value . Run that
> > > with both stock 2.6.37 and the patch reverted.
> > 
> > pxa3xx_set_wake(31, 1) returns 0
> > 
> > Same result with stock 2.6.37 and the patch reverted. Except that
> > without the patch reverted, the device doesn't power up again.
> 
> Ok, can we agree that the patch has no functional impact on the
> set_wake function? And I don't see a reason why reverting that patch
> results in a working resume. That does not make sense at all. Which
> compiler version are you using ?

I agree that this is all very weird and as far as I understand the patch
it should have no functional impact. Unfortunately reverting the patch
does not result in a working resume, but at least it makes a difference
and at the moment it's the only trace I have.

My cross-compile tool-chain uses GCC 4.3.5.


Sven

Re: resume regression in 2.6.37

[Thomas Gleixner]

On Mon, 17 Jan 2011, Sven Neumann wrote:
> On Mon, 2011-01-17 at 13:40 +0100, Thomas Gleixner wrote:
> > On Mon, 17 Jan 2011, Sven Neumann wrote:
> > 
> > > On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:
> > > 
> > > > > Without the revert there is absolutely no sign of resume. With the
> > > > > change reverted I can see that the PXA powers up again, there's an LED
> > > > > showing that the USB controller has power again. Unfortunately there's
> > > > > seems to be another problem and the resume doesn't complete. I've tried
> > > > > to get console output by using no_console_suspend, but there's just
> > > > > gibberish on the console after resume :(
> > > > > 
> > > > > Any idea on how to proceed from here? I could perhaps ask our hardware
> > > > > engineer to try find out where exactly we are stuck in the resume. But
> > > > > I'd like to avoid that if possible.
> > > > 
> > > > Can you stick a printk into the set_wake() function of that irq chip
> > > > and print the irq and on arguments and the return value . Run that
> > > > with both stock 2.6.37 and the patch reverted.
> > > 
> > > pxa3xx_set_wake(31, 1) returns 0
> > > 
> > > Same result with stock 2.6.37 and the patch reverted. Except that
> > > without the patch reverted, the device doesn't power up again.
> > 
> > Ok, can we agree that the patch has no functional impact on the
> > set_wake function? And I don't see a reason why reverting that patch
> > results in a working resume. That does not make sense at all. Which
> > compiler version are you using ?
> 
> I agree that this is all very weird and as far as I understand the patch
> it should have no functional impact. Unfortunately reverting the patch
> does not result in a working resume, but at least it makes a difference
> and at the moment it's the only trace I have.
> 
> My cross-compile tool-chain uses GCC 4.3.5.

Can you try a different compiler ?

Thanks,

	tglx

Re: resume regression in 2.6.37

[Lars-Peter Clausen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/17/2011 01:40 PM, Thomas Gleixner wrote:
> On Mon, 17 Jan 2011, Sven Neumann wrote:
>
>> On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:
>>
>>>> Without the revert there is absolutely no sign of resume. With the
>>>> change reverted I can see that the PXA powers up again, there's an LED
>>>> showing that the USB controller has power again. Unfortunately there's
>>>> seems to be another problem and the resume doesn't complete. I've tried
>>>> to get console output by using no_console_suspend, but there's just
>>>> gibberish on the console after resume :(
>>>>
>>>> Any idea on how to proceed from here? I could perhaps ask our hardware
>>>> engineer to try find out where exactly we are stuck in the resume. But
>>>> I'd like to avoid that if possible.
>>>
>>> Can you stick a printk into the set_wake() function of that irq chip
>>> and print the irq and on arguments and the return value . Run that
>>> with both stock 2.6.37 and the patch reverted.
>>
>> pxa3xx_set_wake(31, 1) returns 0
>>
>> Same result with stock 2.6.37 and the patch reverted. Except that
>> without the patch reverted, the device doesn't power up again.
>
> Ok, can we agree that the patch has no functional impact on the
> set_wake function? And I don't see a reason why reverting that patch
> results in a working resume. That does not make sense at all. Which
> compiler version are you using ?
>
> Thanks,
>
> 	tglx
>

Hi

If you take a look at the pxa gpio irq handling you'll see that it sets the
"set_wake" field of it's irq_chip after calling set_irq_chip for all of the irqs, so
the compat handler gets never installed.

So I guess the following patch fixes it:

diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
- --- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
 	GRER0 &= ~0x3;
 	GEDR0 = 0x3;

+	pxa_low_gpio_chip.set_wake = fn;
+
 	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
 		set_irq_chip(irq, &pxa_low_gpio_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
- -
- -	pxa_low_gpio_chip.set_wake = fn;
 }

 void __init pxa_init_irq(int irq_nr, set_wake_t fn)
@@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
 	/* only unmasked interrupts kick us out of idle */
 	ICCR = 1;

+	pxa_internal_irq_chip.set_wake = fn;
+
 	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
 		set_irq_chip(irq, &pxa_internal_irq_chip);
 		set_irq_handler(irq, handle_level_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}

- -	pxa_internal_irq_chip.set_wake = fn;
 	pxa_init_low_gpio_irq(fn);
 }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk00l4cACgkQBX4mSR26RiMTMQCdGCiu3ALAIEe0dUkX3u5xeHmz
dggAnj2cePanr6HFi+6KQ4tF7cT6xoq0
=yxJs
-----END PGP SIGNATURE-----

Re: resume regression in 2.6.37

[Lars-Peter Clausen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/17/2011 08:24 PM, Lars-Peter Clausen wrote:
> On 01/17/2011 01:40 PM, Thomas Gleixner wrote:
>> On Mon, 17 Jan 2011, Sven Neumann wrote:
> 
>>> On Thu, 2011-01-13 at 18:33 +0100, Thomas Gleixner wrote:
>>>
>>>>> Without the revert there is absolutely no sign of resume. With the
>>>>> change reverted I can see that the PXA powers up again, there's an LED
>>>>> showing that the USB controller has power again. Unfortunately there's
>>>>> seems to be another problem and the resume doesn't complete. I've tried
>>>>> to get console output by using no_console_suspend, but there's just
>>>>> gibberish on the console after resume :(
>>>>>
>>>>> Any idea on how to proceed from here? I could perhaps ask our hardware
>>>>> engineer to try find out where exactly we are stuck in the resume. But
>>>>> I'd like to avoid that if possible.
>>>>
>>>> Can you stick a printk into the set_wake() function of that irq chip
>>>> and print the irq and on arguments and the return value . Run that
>>>> with both stock 2.6.37 and the patch reverted.
>>>
>>> pxa3xx_set_wake(31, 1) returns 0
>>>
>>> Same result with stock 2.6.37 and the patch reverted. Except that
>>> without the patch reverted, the device doesn't power up again.
> 
>> Ok, can we agree that the patch has no functional impact on the
>> set_wake function? And I don't see a reason why reverting that patch
>> results in a working resume. That does not make sense at all. Which
>> compiler version are you using ?
> 
>> Thanks,
> 
>> 	tglx
> 
> 
> Hi
> 
> If you take a look at the pxa gpio irq handling you'll see that it sets the
> "set_wake" field of it's irq_chip after calling set_irq_chip for all of the irqs, so
> the compat handler gets never installed.
> 
> So I guess the following patch fixes it:
> 
> diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
> --- a/arch/arm/mach-pxa/irq.c
> +++ b/arch/arm/mach-pxa/irq.c
> @@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
>  	GRER0 &= ~0x3;
>  	GEDR0 = 0x3;
> 
> +	pxa_low_gpio_chip.set_wake = fn;
> +
>  	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
>  		set_irq_chip(irq, &pxa_low_gpio_chip);
>  		set_irq_handler(irq, handle_edge_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
> -
> -	pxa_low_gpio_chip.set_wake = fn;
>  }
> 
>  void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> @@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
>  	/* only unmasked interrupts kick us out of idle */
>  	ICCR = 1;
> 
> +	pxa_internal_irq_chip.set_wake = fn;
> +
>  	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
>  		set_irq_chip(irq, &pxa_internal_irq_chip);
>  		set_irq_handler(irq, handle_level_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
> 
> -	pxa_internal_irq_chip.set_wake = fn;
>  	pxa_init_low_gpio_irq(fn);
>  }
> 
and

diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index d1c747c..6fc8281 100644
- --- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -558,13 +558,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
 {
 	int irq;

+	pxa_ext_wakeup_chip.set_wake = fn;
+
 	for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
 		set_irq_chip(irq, &pxa_ext_wakeup_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
- -
- -	pxa_ext_wakeup_chip.set_wake = fn;
 }

 void __init pxa3xx_init_irq(void)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk00n7wACgkQBX4mSR26RiO1lgCfRblXeDhxuXHe7sKzQEApzyS7
wjgAniUvOc9RdV5jms1SU5ivQ0T3NogK
=heYm
-----END PGP SIGNATURE-----

Re: resume regression in 2.6.37

[Sven Neumann]

Hi,

On Mon, 2011-01-17 at 20:59 +0100, Lars-Peter Clausen wrote:

> > If you take a look at the pxa gpio irq handling you'll see that it sets the
> > "set_wake" field of it's irq_chip after calling set_irq_chip for all of the irqs, so
> > the compat handler gets never installed.
> > 
> > So I guess the following patch fixes it:
> > 
> > diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
> > --- a/arch/arm/mach-pxa/irq.c
> > +++ b/arch/arm/mach-pxa/irq.c
> > @@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
> >  	GRER0 &= ~0x3;
> >  	GEDR0 = 0x3;
> > 
> > +	pxa_low_gpio_chip.set_wake = fn;
> > +
> >  	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
> >  		set_irq_chip(irq, &pxa_low_gpio_chip);
> >  		set_irq_handler(irq, handle_edge_irq);
> >  		set_irq_flags(irq, IRQF_VALID);
> >  	}
> > -
> > -	pxa_low_gpio_chip.set_wake = fn;
> >  }
> > 
> >  void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> > @@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> >  	/* only unmasked interrupts kick us out of idle */
> >  	ICCR = 1;
> > 
> > +	pxa_internal_irq_chip.set_wake = fn;
> > +
> >  	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
> >  		set_irq_chip(irq, &pxa_internal_irq_chip);
> >  		set_irq_handler(irq, handle_level_irq);
> >  		set_irq_flags(irq, IRQF_VALID);
> >  	}
> > 
> > -	pxa_internal_irq_chip.set_wake = fn;
> >  	pxa_init_low_gpio_irq(fn);
> >  }
> > 
> and
> 
> diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
> index d1c747c..6fc8281 100644
> - --- a/arch/arm/mach-pxa/pxa3xx.c
> +++ b/arch/arm/mach-pxa/pxa3xx.c
> @@ -558,13 +558,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
>  {
>  	int irq;
> 
> +	pxa_ext_wakeup_chip.set_wake = fn;
> +
>  	for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
>  		set_irq_chip(irq, &pxa_ext_wakeup_chip);
>  		set_irq_handler(irq, handle_edge_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
> - -
> - -	pxa_ext_wakeup_chip.set_wake = fn;
>  }
> 
>  void __init pxa3xx_init_irq(void)

Indeed, that does fix it. It doesn't fix all my problems, but at least
the device shows the same behavior now as with commit 2f7e99bb9be
reverted. It powers up again and then outputs gibberish to the console.
I will try another bisect to find out what else is broken.

I will send another mail with your two patches combined into a single
commit. Please consider to sign-off that patch and send it upstream and
to stable.


Thanks a lot for your help,
Sven

[PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Sven Neumann]

From: Lars-Peter Clausen <lars@metafoo.de>

Set the "set_wake" field of the irq_chip struct before calling
set_irq_chip() for all of the IRQs, so that the compat handler
actually gets installed.

Tested-by: Sven Neumann <s.neumann@raumfeld.com>
---
 arch/arm/mach-pxa/irq.c    |    7 ++++---
 arch/arm/mach-pxa/pxa3xx.c |    4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
index 1beb40f..a5b42a7 100644
--- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
 	GRER0 &= ~0x3;
 	GEDR0 = 0x3;
 
+	pxa_low_gpio_chip.set_wake = fn;
+
 	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
 		set_irq_chip(irq, &pxa_low_gpio_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
-
-	pxa_low_gpio_chip.set_wake = fn;
 }
 
 void __init pxa_init_irq(int irq_nr, set_wake_t fn)
@@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
 	/* only unmasked interrupts kick us out of idle */
 	ICCR = 1;
 
+	pxa_internal_irq_chip.set_wake = fn;
+
 	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
 		set_irq_chip(irq, &pxa_internal_irq_chip);
 		set_irq_handler(irq, handle_level_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
 
-	pxa_internal_irq_chip.set_wake = fn;
 	pxa_init_low_gpio_irq(fn);
 }
 
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 276e8fb..5a8fe5c 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -567,13 +567,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
 {
 	int irq;
 
+	pxa_ext_wakeup_chip.set_wake = fn;
+
 	for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
 		set_irq_chip(irq, &pxa_ext_wakeup_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
-
-	pxa_ext_wakeup_chip.set_wake = fn;
 }
 
 void __init pxa3xx_init_irq(void)
-- 
1.7.1

Re: resume regression in 2.6.37

[Thomas Gleixner]

B1;2401;0cOn Mon, 17 Jan 2011, Lars-Peter Clausen wrote:
> If you take a look at the pxa gpio irq handling you'll see that it sets the
> "set_wake" field of it's irq_chip after calling set_irq_chip for all of the irqs, so
> the compat handler gets never installed.

Good catch. Can you please resend with a proper changelog and SOB?
Please add 

Cc: stable@kernel.org

after the Signed-off-by line.

Also feel free to add:

Acked-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Uwe Kleine-König]

On Tue, Jan 18, 2011 at 09:18:58AM +0100, Sven Neumann wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Set the "set_wake" field of the irq_chip struct before calling
> set_irq_chip() for all of the IRQs, so that the compat handler
> actually gets installed.
> 
> Tested-by: Sven Neumann <s.neumann@raumfeld.com>
If you send the patch out it needs your S-o-b, too.

Best regards
Uwe

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

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Lars-Peter Clausen]

On 01/18/2011 09:18 AM, Sven Neumann wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Set the "set_wake" field of the irq_chip struct before calling
> set_irq_chip() for all of the IRQs, so that the compat handler
> actually gets installed.
> 
> Tested-by: Sven Neumann <s.neumann@raumfeld.com>

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>  arch/arm/mach-pxa/irq.c    |    7 ++++---
>  arch/arm/mach-pxa/pxa3xx.c |    4 ++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
> index 1beb40f..a5b42a7 100644
> --- a/arch/arm/mach-pxa/irq.c
> +++ b/arch/arm/mach-pxa/irq.c
> @@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
>  	GRER0 &= ~0x3;
>  	GEDR0 = 0x3;
>  
> +	pxa_low_gpio_chip.set_wake = fn;
> +
>  	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
>  		set_irq_chip(irq, &pxa_low_gpio_chip);
>  		set_irq_handler(irq, handle_edge_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
> -
> -	pxa_low_gpio_chip.set_wake = fn;
>  }
>  
>  void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> @@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
>  	/* only unmasked interrupts kick us out of idle */
>  	ICCR = 1;
>  
> +	pxa_internal_irq_chip.set_wake = fn;
> +
>  	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
>  		set_irq_chip(irq, &pxa_internal_irq_chip);
>  		set_irq_handler(irq, handle_level_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
>  
> -	pxa_internal_irq_chip.set_wake = fn;
>  	pxa_init_low_gpio_irq(fn);
>  }
>  
> diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
> index 276e8fb..5a8fe5c 100644
> --- a/arch/arm/mach-pxa/pxa3xx.c
> +++ b/arch/arm/mach-pxa/pxa3xx.c
> @@ -567,13 +567,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
>  {
>  	int irq;
>  
> +	pxa_ext_wakeup_chip.set_wake = fn;
> +
>  	for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
>  		set_irq_chip(irq, &pxa_ext_wakeup_chip);
>  		set_irq_handler(irq, handle_edge_irq);
>  		set_irq_flags(irq, IRQF_VALID);
>  	}
> -
> -	pxa_ext_wakeup_chip.set_wake = fn;
>  }
>  
>  void __init pxa3xx_init_irq(void)

[PATCH] ARM: pxa: fix GPIO IRQ handling

[Sven Neumann]

From: Lars-Peter Clausen <lars@metafoo.de>

Set the "set_wake" field of the irq_chip struct before calling
set_irq_chip() for all of the IRQs, so that the compat handler
actually gets installed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@kernel.org (for 2.6.37 only)
---
 arch/arm/mach-pxa/irq.c    |    7 ++++---
 arch/arm/mach-pxa/pxa3xx.c |    4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
index 1beb40f..a5b42a7 100644
--- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
 	GRER0 &= ~0x3;
 	GEDR0 = 0x3;
 
+	pxa_low_gpio_chip.set_wake = fn;
+
 	for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
 		set_irq_chip(irq, &pxa_low_gpio_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
-
-	pxa_low_gpio_chip.set_wake = fn;
 }
 
 void __init pxa_init_irq(int irq_nr, set_wake_t fn)
@@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
 	/* only unmasked interrupts kick us out of idle */
 	ICCR = 1;
 
+	pxa_internal_irq_chip.set_wake = fn;
+
 	for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
 		set_irq_chip(irq, &pxa_internal_irq_chip);
 		set_irq_handler(irq, handle_level_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
 
-	pxa_internal_irq_chip.set_wake = fn;
 	pxa_init_low_gpio_irq(fn);
 }
 
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 276e8fb..5a8fe5c 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -567,13 +567,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
 {
 	int irq;
 
+	pxa_ext_wakeup_chip.set_wake = fn;
+
 	for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
 		set_irq_chip(irq, &pxa_ext_wakeup_chip);
 		set_irq_handler(irq, handle_edge_irq);
 		set_irq_flags(irq, IRQF_VALID);
 	}
-
-	pxa_ext_wakeup_chip.set_wake = fn;
 }
 
 void __init pxa3xx_init_irq(void)
-- 
1.7.1

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Eric Miao]

On Tue, Jan 18, 2011 at 3:47 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 01/18/2011 09:18 AM, Sven Neumann wrote:
>> From: Lars-Peter Clausen <lars@metafoo.de>
>>
>> Set the "set_wake" field of the irq_chip struct before calling
>> set_irq_chip() for all of the IRQs, so that the compat handler
>> actually gets installed.

Not really sure if this is necessary. Lennert has done the transition to
irq_data to those _set_wake() functions as well, so it should be OK
that compat handler doesn't get installed.

Now I'm not against moving this above, just wondering if there were
any consequences/errors coming out?

>>
>> Tested-by: Sven Neumann <s.neumann@raumfeld.com>
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>
>> ---
>>  arch/arm/mach-pxa/irq.c    |    7 ++++---
>>  arch/arm/mach-pxa/pxa3xx.c |    4 ++--
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
>> index 1beb40f..a5b42a7 100644
>> --- a/arch/arm/mach-pxa/irq.c
>> +++ b/arch/arm/mach-pxa/irq.c
>> @@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
>>       GRER0 &= ~0x3;
>>       GEDR0 = 0x3;
>>
>> +     pxa_low_gpio_chip.set_wake = fn;
>> +
>>       for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
>>               set_irq_chip(irq, &pxa_low_gpio_chip);
>>               set_irq_handler(irq, handle_edge_irq);
>>               set_irq_flags(irq, IRQF_VALID);
>>       }
>> -
>> -     pxa_low_gpio_chip.set_wake = fn;
>>  }
>>
>>  void __init pxa_init_irq(int irq_nr, set_wake_t fn)
>> @@ -142,13 +142,14 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
>>       /* only unmasked interrupts kick us out of idle */
>>       ICCR = 1;
>>
>> +     pxa_internal_irq_chip.set_wake = fn;
>> +
>>       for (irq = PXA_IRQ(0); irq < PXA_IRQ(irq_nr); irq++) {
>>               set_irq_chip(irq, &pxa_internal_irq_chip);
>>               set_irq_handler(irq, handle_level_irq);
>>               set_irq_flags(irq, IRQF_VALID);
>>       }
>>
>> -     pxa_internal_irq_chip.set_wake = fn;
>>       pxa_init_low_gpio_irq(fn);
>>  }
>>
>> diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
>> index 276e8fb..5a8fe5c 100644
>> --- a/arch/arm/mach-pxa/pxa3xx.c
>> +++ b/arch/arm/mach-pxa/pxa3xx.c
>> @@ -567,13 +567,13 @@ static void __init pxa_init_ext_wakeup_irq(set_wake_t fn)
>>  {
>>       int irq;
>>
>> +     pxa_ext_wakeup_chip.set_wake = fn;
>> +
>>       for (irq = IRQ_WAKEUP0; irq <= IRQ_WAKEUP1; irq++) {
>>               set_irq_chip(irq, &pxa_ext_wakeup_chip);
>>               set_irq_handler(irq, handle_edge_irq);
>>               set_irq_flags(irq, IRQF_VALID);
>>       }
>> -
>> -     pxa_ext_wakeup_chip.set_wake = fn;
>>  }
>>
>>  void __init pxa3xx_init_irq(void)
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Lennert Buijtenhek]

On Tue, Jan 18, 2011 at 08:15:22AM -0800, Eric Miao wrote:

> >> Set the "set_wake" field of the irq_chip struct before calling
> >> set_irq_chip() for all of the IRQs, so that the compat handler
> >> actually gets installed.
> 
> Not really sure if this is necessary. Lennert has done the transition to
> irq_data to those _set_wake() functions as well, so it should be OK
> that compat handler doesn't get installed.

Yeah, this has been dealt with upstream, and this patch won't apply.


> >> diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
> >> index 1beb40f..a5b42a7 100644
> >> --- a/arch/arm/mach-pxa/irq.c
> >> +++ b/arch/arm/mach-pxa/irq.c
> >> @@ -111,13 +111,13 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
> >>       GRER0 &= ~0x3;
> >>       GEDR0 = 0x3;
> >>
> >> +     pxa_low_gpio_chip.set_wake = fn;
> >> +
> >>       for (irq = IRQ_GPIO0; irq <= IRQ_GPIO1; irq++) {
> >>               set_irq_chip(irq, &pxa_low_gpio_chip);
> >>               set_irq_handler(irq, handle_edge_irq);
> >>               set_irq_flags(irq, IRQF_VALID);
> >>       }
> >> -
> >> -     pxa_low_gpio_chip.set_wake = fn;

Upstream has:

        pxa_low_gpio_chip.irq_set_wake = fn;

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Sven Neumann]

On Tue, 2011-01-18 at 08:15 -0800, Eric Miao wrote:
> On Tue, Jan 18, 2011 at 3:47 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> > On 01/18/2011 09:18 AM, Sven Neumann wrote:
> >> From: Lars-Peter Clausen <lars@metafoo.de>
> >>
> >> Set the "set_wake" field of the irq_chip struct before calling
> >> set_irq_chip() for all of the IRQs, so that the compat handler
> >> actually gets installed.
> 
> Not really sure if this is necessary. Lennert has done the transition to
> irq_data to those _set_wake() functions as well, so it should be OK
> that compat handler doesn't get installed.
> 
> Now I'm not against moving this above, just wondering if there were
> any consequences/errors coming out?

Well, our PXA300 device doesn't power up again when it should resume
from suspend. Fine with me if this has been fixed upstream, but I would
still like to get this patch into 2.6.37. I'll try to test upstream as
soon as Linus releases 2.6.38-rc1...


Sven

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Eric Miao]

On Tue, Jan 18, 2011 at 4:49 PM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> On Tue, 2011-01-18 at 08:15 -0800, Eric Miao wrote:
>> On Tue, Jan 18, 2011 at 3:47 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> > On 01/18/2011 09:18 AM, Sven Neumann wrote:
>> >> From: Lars-Peter Clausen <lars@metafoo.de>
>> >>
>> >> Set the "set_wake" field of the irq_chip struct before calling
>> >> set_irq_chip() for all of the IRQs, so that the compat handler
>> >> actually gets installed.
>>
>> Not really sure if this is necessary. Lennert has done the transition to
>> irq_data to those _set_wake() functions as well, so it should be OK
>> that compat handler doesn't get installed.
>>
>> Now I'm not against moving this above, just wondering if there were
>> any consequences/errors coming out?
>
> Well, our PXA300 device doesn't power up again when it should resume
> from suspend. Fine with me if this has been fixed upstream,

Yeah, that's true. Mind re-post with your SOB?

> but I would
> still like to get this patch into 2.6.37. I'll try to test upstream as
> soon as Linus releases 2.6.38-rc1...
>
>
> Sven
>
>
>

Re: [PATCH] pxa3xx: fix PXA GPIO IRQ handling

[Eric Miao]

On Thu, Jan 20, 2011 at 1:48 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Tue, Jan 18, 2011 at 4:49 PM, Sven Neumann <s.neumann@raumfeld.com> wrote:
>> On Tue, 2011-01-18 at 08:15 -0800, Eric Miao wrote:
>>> On Tue, Jan 18, 2011 at 3:47 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> > On 01/18/2011 09:18 AM, Sven Neumann wrote:
>>> >> From: Lars-Peter Clausen <lars@metafoo.de>
>>> >>
>>> >> Set the "set_wake" field of the irq_chip struct before calling
>>> >> set_irq_chip() for all of the IRQs, so that the compat handler
>>> >> actually gets installed.
>>>
>>> Not really sure if this is necessary. Lennert has done the transition to
>>> irq_data to those _set_wake() functions as well, so it should be OK
>>> that compat handler doesn't get installed.
>>>
>>> Now I'm not against moving this above, just wondering if there were
>>> any consequences/errors coming out?
>>
>> Well, our PXA300 device doesn't power up again when it should resume
>> from suspend. Fine with me if this has been fixed upstream,
>
> Yeah, that's true. Mind re-post with your SOB?

And Cc linux-stable.

>
>> but I would
>> still like to get this patch into 2.6.37. I'll try to test upstream as
>> soon as Linus releases 2.6.38-rc1...

That's will be great help.

>>
>>
>> Sven
>>
>>
>>
>