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

On Sat, Jul 28, 2012 at 12:53:11AM +0200, Rafael J. Wysocki wrote:

Unfortunately, your commit

commit ca5481c68e9fbcea62bb3c78ae6cccf99ca8fb73
Author: Paul Mundt [off-list ref]
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 [off-list ref]
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 [off-list ref]
    Reported-by: Kuninori Morimoto [off-list ref]
    Signed-off-by: Paul Mundt [off-list ref]

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);