diff mbox

[Regression,post-3.5] System suspend broken on the Mackerel board

Message ID 20120801073004.GE15380@linux-sh.org (mailing list archive)
State Rejected
Headers show

Commit Message

Paul Mundt Aug. 1, 2012, 7:30 a.m. UTC
On Sat, Jul 28, 2012 at 12:53:11AM +0200, Rafael J. Wysocki wrote:
> Unfortunately, your commit
> 
> commit ca5481c68e9fbcea62bb3c78ae6cccf99ca8fb73
> Author: Paul Mundt <lethal@linux-sh.org>
> Date:   Tue Jul 10 12:08:14 2012 +0900
> 
>     sh: pfc: Rudimentary pinctrl-backed GPIO support.
> 
> breaks system suspend on the Mackerel board (.config attached).  The system
> simply doesn't suspend and instead it hangs somewhere while suspending
> devices (apparently before running the "late" callbacks).
> 
> If the above commit is reverted, system suspend works normally.

On Tue, Jul 31, 2012 at 08:57:02PM -0700, kuninori.morimoto.gx@renesas.com wrote:
> gpio: sh7724_pfc handling gpio 0 -> 486
> core: sh7724_pfc support registered
> HW Breakpoints: SH-4A UBC support registered
> autorequest GPIO-53
> ------------[ cut here ]------------
> WARNING: at /opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/drivers/gpio/gpiolib.3
> Modules linked in:
> 
> Pid : 1, Comm:          swapper
> CPU : 0                 Not tainted  (3.5.0-rc6+ #1407)
> 
> PC is at gpio_ensure_requested+0x30/0x78
> PR is at gpio_ensure_requested+0x30/0x78

Morimoto-san's logs off-list made it clear what happened. Both of these
platforms are going gpio_request() calls at arch_initcall() time which
completely screwed up the ordering of the pfc core. We seem to -ENODEV
out in one place due to missing a pfc pointer initialization elsewhere
resulting in -EPROBE_DEFER from gpiolib.

Turns out we can just collapse the probe/init stuff anyways, so this
ought to fix it. I've verified that it fixes Morimoto-san's issue, my
expectation is that the mackerel case is likewise getting tripped up but
no one bothered implementing any error detecting logic for gpio_request()
failing, so it doesn't fail gracefully.

I'll be pushing this out to Linus shortly:

---

commit 1e32dfe323d156d5d7b25b9feffe015d19713db2
Author: Paul Mundt <lethal@linux-sh.org>
Date:   Wed Aug 1 16:27:38 2012 +0900

    sh: pfc: Fix up init ordering mess.
    
    Commit ca5481c68e9fbcea62bb3c78ae6cccf99ca8fb73 ("sh: pfc: Rudimentary
    pinctrl-backed GPIO support.") introduced a regression for platforms that
    were doing early GPIO API calls (from arch_initcall() or earlier),
    leading to a situation where our two-stage registration logic would trip
    itself up and we'd -ENODEV out of the pinctrl registration path,
    resulting in endless -EPROBE_DEFER errors. Further lack of checking any
    sort of errors from gpio_request() resulted in boot time warnings,
    tripping on the FLAG_REQUESTED test-and-set in gpio_ensure_requested().
    
    As it turns out there's no particular need to bother with the two-stage
    registration, as the platform bus is already available at the point that
    we have to start caring. As such, it's easiest to simply fold these
    together in to a single init path, the ordering of which is ensured
    through the platform's mux registration, as usual.
    
    Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
    Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
    Signed-off-by: Paul Mundt <lethal@linux-sh.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael Wysocki Aug. 4, 2012, 10:02 p.m. UTC | #1
On Wednesday, August 01, 2012, Paul Mundt wrote:
> On Sat, Jul 28, 2012 at 12:53:11AM +0200, Rafael J. Wysocki wrote:
> > Unfortunately, your commit
> > 
> > commit ca5481c68e9fbcea62bb3c78ae6cccf99ca8fb73
> > Author: Paul Mundt <lethal@linux-sh.org>
> > Date:   Tue Jul 10 12:08:14 2012 +0900
> > 
> >     sh: pfc: Rudimentary pinctrl-backed GPIO support.
> > 
> > breaks system suspend on the Mackerel board (.config attached).  The system
> > simply doesn't suspend and instead it hangs somewhere while suspending
> > devices (apparently before running the "late" callbacks).
> > 
> > If the above commit is reverted, system suspend works normally.
> 
> On Tue, Jul 31, 2012 at 08:57:02PM -0700, kuninori.morimoto.gx@renesas.com wrote:
> > gpio: sh7724_pfc handling gpio 0 -> 486
> > core: sh7724_pfc support registered
> > HW Breakpoints: SH-4A UBC support registered
> > autorequest GPIO-53
> > ------------[ cut here ]------------
> > WARNING: at /opt/usr/src/WORK/morimoto/gitlinux/linux-2.6/drivers/gpio/gpiolib.3
> > Modules linked in:
> > 
> > Pid : 1, Comm:          swapper
> > CPU : 0                 Not tainted  (3.5.0-rc6+ #1407)
> > 
> > PC is at gpio_ensure_requested+0x30/0x78
> > PR is at gpio_ensure_requested+0x30/0x78
> 
> Morimoto-san's logs off-list made it clear what happened. Both of these
> platforms are going gpio_request() calls at arch_initcall() time which
> completely screwed up the ordering of the pfc core. We seem to -ENODEV
> out in one place due to missing a pfc pointer initialization elsewhere
> resulting in -EPROBE_DEFER from gpiolib.
> 
> Turns out we can just collapse the probe/init stuff anyways, so this
> ought to fix it. I've verified that it fixes Morimoto-san's issue, my
> expectation is that the mackerel case is likewise getting tripped up but
> no one bothered implementing any error detecting logic for gpio_request()
> failing, so it doesn't fail gracefully.
> 
> I'll be pushing this out to Linus shortly:

Thanks, this helped.  Resume works correctly on Mackerel with 3.6-rc1.

However, I'm now seeing a different problem related to system suspend on that,
board which is that sh7372_enter_a3sm_common() returns immediately, as though
at least one of the wakeup signals was permanently asserted.  This hadn't
happened before your last pull request was merged, so I suspect that one
of the irqdomain patches might introduce this behavior.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mundt Aug. 7, 2012, 1:55 a.m. UTC | #2
On Sun, Aug 05, 2012 at 12:02:43AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, August 01, 2012, Paul Mundt wrote:
> > Turns out we can just collapse the probe/init stuff anyways, so this
> > ought to fix it. I've verified that it fixes Morimoto-san's issue, my
> > expectation is that the mackerel case is likewise getting tripped up but
> > no one bothered implementing any error detecting logic for gpio_request()
> > failing, so it doesn't fail gracefully.
> > 
> > I'll be pushing this out to Linus shortly:
> 
> Thanks, this helped.  Resume works correctly on Mackerel with 3.6-rc1.
> 
> However, I'm now seeing a different problem related to system suspend on that,
> board which is that sh7372_enter_a3sm_common() returns immediately, as though
> at least one of the wakeup signals was permanently asserted.  This hadn't
> happened before your last pull request was merged, so I suspect that one
> of the irqdomain patches might introduce this behavior.
> 
Ok, I'll back off the irqdomain selection until we have a chance to test
it more on the other platforms.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Aug. 8, 2012, 9:23 a.m. UTC | #3
On Tuesday, August 07, 2012, Paul Mundt wrote:
> On Sun, Aug 05, 2012 at 12:02:43AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, August 01, 2012, Paul Mundt wrote:
> > > Turns out we can just collapse the probe/init stuff anyways, so this
> > > ought to fix it. I've verified that it fixes Morimoto-san's issue, my
> > > expectation is that the mackerel case is likewise getting tripped up but
> > > no one bothered implementing any error detecting logic for gpio_request()
> > > failing, so it doesn't fail gracefully.
> > > 
> > > I'll be pushing this out to Linus shortly:
> > 
> > Thanks, this helped.  Resume works correctly on Mackerel with 3.6-rc1.
> > 
> > However, I'm now seeing a different problem related to system suspend on that,
> > board which is that sh7372_enter_a3sm_common() returns immediately, as though
> > at least one of the wakeup signals was permanently asserted.  This hadn't
> > happened before your last pull request was merged, so I suspect that one
> > of the irqdomain patches might introduce this behavior.
> > 
> Ok, I'll back off the irqdomain selection until we have a chance to test
> it more on the other platforms.

If you mean removing the select from drivers/sh/intc/Kconfig, that alone doesn't
help.  Reverting commit 1d6a21b0a672fb29b01ccf397d478e0541e17716
(sh: intc: initial irqdomain support.) helps, though.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mundt Aug. 9, 2012, 4:40 a.m. UTC | #4
On Wed, Aug 08, 2012 at 11:23:04AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, August 07, 2012, Paul Mundt wrote:
> > On Sun, Aug 05, 2012 at 12:02:43AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, August 01, 2012, Paul Mundt wrote:
> > > > Turns out we can just collapse the probe/init stuff anyways, so this
> > > > ought to fix it. I've verified that it fixes Morimoto-san's issue, my
> > > > expectation is that the mackerel case is likewise getting tripped up but
> > > > no one bothered implementing any error detecting logic for gpio_request()
> > > > failing, so it doesn't fail gracefully.
> > > > 
> > > > I'll be pushing this out to Linus shortly:
> > > 
> > > Thanks, this helped.  Resume works correctly on Mackerel with 3.6-rc1.
> > > 
> > > However, I'm now seeing a different problem related to system suspend on that,
> > > board which is that sh7372_enter_a3sm_common() returns immediately, as though
> > > at least one of the wakeup signals was permanently asserted.  This hadn't
> > > happened before your last pull request was merged, so I suspect that one
> > > of the irqdomain patches might introduce this behavior.
> > > 
> > Ok, I'll back off the irqdomain selection until we have a chance to test
> > it more on the other platforms.
> 
> If you mean removing the select from drivers/sh/intc/Kconfig, that alone doesn't
> help.  Reverting commit 1d6a21b0a672fb29b01ccf397d478e0541e17716
> (sh: intc: initial irqdomain support.) helps, though.
> 
Yeah, I see what happened now. I inadvertently trampled the -EEXIST
behaviour. I've taken care of it now, so hopefully you'll have better
luck with -rc2!
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Aug. 9, 2012, 9:20 p.m. UTC | #5
On Thursday, August 09, 2012, Paul Mundt wrote:
> On Wed, Aug 08, 2012 at 11:23:04AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, August 07, 2012, Paul Mundt wrote:
> > > On Sun, Aug 05, 2012 at 12:02:43AM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, August 01, 2012, Paul Mundt wrote:
> > > > > Turns out we can just collapse the probe/init stuff anyways, so this
> > > > > ought to fix it. I've verified that it fixes Morimoto-san's issue, my
> > > > > expectation is that the mackerel case is likewise getting tripped up but
> > > > > no one bothered implementing any error detecting logic for gpio_request()
> > > > > failing, so it doesn't fail gracefully.
> > > > > 
> > > > > I'll be pushing this out to Linus shortly:
> > > > 
> > > > Thanks, this helped.  Resume works correctly on Mackerel with 3.6-rc1.
> > > > 
> > > > However, I'm now seeing a different problem related to system suspend on that,
> > > > board which is that sh7372_enter_a3sm_common() returns immediately, as though
> > > > at least one of the wakeup signals was permanently asserted.  This hadn't
> > > > happened before your last pull request was merged, so I suspect that one
> > > > of the irqdomain patches might introduce this behavior.
> > > > 
> > > Ok, I'll back off the irqdomain selection until we have a chance to test
> > > it more on the other platforms.
> > 
> > If you mean removing the select from drivers/sh/intc/Kconfig, that alone doesn't
> > help.  Reverting commit 1d6a21b0a672fb29b01ccf397d478e0541e17716
> > (sh: intc: initial irqdomain support.) helps, though.
> > 
> Yeah, I see what happened now. I inadvertently trampled the -EEXIST
> behaviour. I've taken care of it now, so hopefully you'll have better
> luck with -rc2!

I tested your "sh: intc: Handle domain association for sparseirq pre-allocated
vectors" patch in the meantime and it does fix the issue.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/sh/pfc/pinctrl.c b/drivers/sh/pfc/pinctrl.c
index 814b292..2804eaa 100644
--- a/drivers/sh/pfc/pinctrl.c
+++ b/drivers/sh/pfc/pinctrl.c
@@ -325,20 +325,6 @@  static struct pinctrl_desc sh_pfc_pinctrl_desc = {
 	.confops	= &sh_pfc_pinconf_ops,
 };
 
-int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
-{
-	sh_pfc_pmx = kzalloc(sizeof(struct sh_pfc_pinctrl), GFP_KERNEL);
-	if (unlikely(!sh_pfc_pmx))
-		return -ENOMEM;
-
-	spin_lock_init(&sh_pfc_pmx->lock);
-
-	sh_pfc_pmx->pfc = pfc;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(sh_pfc_register_pinctrl);
-
 static inline void __devinit sh_pfc_map_one_gpio(struct sh_pfc *pfc,
 						 struct sh_pfc_pinctrl *pmx,
 						 struct pinmux_gpio *gpio,
@@ -505,7 +491,7 @@  static struct platform_device sh_pfc_pinctrl_device = {
 	.id		= -1,
 };
 
-static int __init sh_pfc_pinctrl_init(void)
+static int sh_pfc_pinctrl_init(void)
 {
 	int rc;
 
@@ -519,10 +505,22 @@  static int __init sh_pfc_pinctrl_init(void)
 	return rc;
 }
 
+int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
+{
+	sh_pfc_pmx = kzalloc(sizeof(struct sh_pfc_pinctrl), GFP_KERNEL);
+	if (unlikely(!sh_pfc_pmx))
+		return -ENOMEM;
+
+	spin_lock_init(&sh_pfc_pmx->lock);
+
+	sh_pfc_pmx->pfc = pfc;
+
+	return sh_pfc_pinctrl_init();
+}
+EXPORT_SYMBOL_GPL(sh_pfc_register_pinctrl);
+
 static void __exit sh_pfc_pinctrl_exit(void)
 {
 	platform_driver_unregister(&sh_pfc_pinctrl_driver);
 }
-
-subsys_initcall(sh_pfc_pinctrl_init);
 module_exit(sh_pfc_pinctrl_exit);