From patchwork Tue Oct 25 21:31:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulf Hansson X-Patchwork-Id: 79283 Delivered-To: patch@linaro.org Received: by 10.140.97.247 with SMTP id m110csp86933qge; Tue, 25 Oct 2016 14:31:38 -0700 (PDT) X-Received: by 10.98.9.147 with SMTP id 19mr43264180pfj.68.1477431098745; Tue, 25 Oct 2016 14:31:38 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t2si22760202pgn.168.2016.10.25.14.31.38; Tue, 25 Oct 2016 14:31:38 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756334AbcJYVbX (ORCPT + 14 others); Tue, 25 Oct 2016 17:31:23 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:36648 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756224AbcJYVbU (ORCPT ); Tue, 25 Oct 2016 17:31:20 -0400 Received: by mail-wm0-f42.google.com with SMTP id b80so188083475wme.1 for ; Tue, 25 Oct 2016 14:31:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4hGWL7UWd3hfG4mJ309Gv9GIGQR77gYHCddVMuHw55g=; b=WDhil5DSiECKy2KEsuzHxSTG7wqy3rORkkqSwQOxN/Oq/ieJk5SIungE0WwtVtyANw QMWa6cP7HxM/xnOuZJrk8IEY/OFk37RREUxBVLL2Y35LsF12uKrPOe9mp3PCyKYPd8mX FC8ip71nhKoxtamq4DiYNFMC+C9KdCiCSkU8A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4hGWL7UWd3hfG4mJ309Gv9GIGQR77gYHCddVMuHw55g=; b=hXvv3xJgH2/6oa/Ispf/m9hJEB8J/kiFPYcR47MTvmUNrODbo90LcWrVFq52S8BsHN SDCCi311WVsgYTfBeFTX1+cI6UZ85VWI1grxTZf9CK42GXirI79gTVY7lpmH/Rb4h4z2 uo+vzoE27Jcu/EV0ISzKRs9HLl5Xol9m6Okl4wizP9SWXVuQBou3HQO+Nxjf/+EMlrN0 u3XDx6nUhMyn11OYMdh4e84CG7zrs17HlDkUCtkC+FmPmkJ5zw7l5EWQWgXUI/rXTpJ6 k/rI+ljkPZprfinTR6vSk6ZbnK1AFfp6W7VZUZWwybBfU7fyzr0X6yCpa1v0iPGdkcoi Msqw== X-Gm-Message-State: ABUngvdu+T8ijY+4QKUSMuHRMl2BgW0b9zlfCkWKrluraF2SiuTEv6o7Dlh5wggPT0QYY6ED7d9IdvzHPfdhiSz3 X-Received: by 10.194.109.42 with SMTP id hp10mr17869839wjb.24.1477431078944; Tue, 25 Oct 2016 14:31:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.194.151.41 with HTTP; Tue, 25 Oct 2016 14:31:17 -0700 (PDT) In-Reply-To: References: <1476728221-26530-1-git-send-email-ulf.hansson@linaro.org> <1476728221-26530-5-git-send-email-ulf.hansson@linaro.org> From: Ulf Hansson Date: Tue, 25 Oct 2016 23:31:17 +0200 Message-ID: Subject: Re: [PATCH 4/4] PM / Runtime: Don't allow to suspend a device with an active child To: Geert Uytterhoeven Cc: "Rafael J. Wysocki" , Alan Stern , Linux PM list , Len Brown , Pavel Machek , Kevin Hilman , Lina Iyer , Jon Hunter , Marek Szyprowski , Linus Walleij , Linux-Renesas Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On 25 October 2016 at 15:59, Geert Uytterhoeven wrote: > Hi Ulf, > > On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson wrote: >> When resuming a device in __pm_runtime_set_status(), the prerequisite is >> that its parent must already be active, else an error code is returned and >> the device's status remains suspended. >> >> When suspending a device there is no similar constraints being validated. >> Let's change this to make the behaviour consistent, by not allowing to >> suspend a device with an active child, unless it has been explicitly set to >> ignore its children. >> >> Signed-off-by: Ulf Hansson > > This is now commit 8b1107b85efd78c in pm/linux-next. > > This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the > smsc911x Ethernet is connected to an external bus, whose clock is controlled > through Runtime PM and the simple-pm-bus driver. > > Reverting this commit fixes the issue. Geert, thanks again for testing and reporting. I believe this problem is directly related to what you reported for another patch [1] as well. Can you please try out this rather trivial patch to the Ethernet driver (smsc911x), to see if that solves the problem(s). From: Ulf Hansson Date: Tue, 25 Oct 2016 23:11:51 +0200 Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during system suspend Signed-off-by: Ulf Hansson --- drivers/net/ethernet/smsc/smsc911x.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 1.9.1 Kind regards Uffe [1] https://patchwork.kernel.org/patch/9375061/ > >> --- >> drivers/base/power/runtime.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c >> index f47a345..6f946a3 100644 >> --- a/drivers/base/power/runtime.c >> +++ b/drivers/base/power/runtime.c >> @@ -1028,7 +1028,17 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status) >> goto out_set; >> >> if (status == RPM_SUSPENDED) { >> - /* It always is possible to set the status to 'suspended'. */ >> + /* >> + * It is invalid to suspend a device with an active child, >> + * unless it has been set to ignore its children. >> + */ >> + if (!dev->power.ignore_children && >> + atomic_read(&dev->power.child_count)) { >> + dev_err(dev, "runtime PM trying to suspend device but active child\n"); >> + error = -EBUSY; >> + goto out; >> + } >> + >> if (parent) { >> atomic_add_unless(&parent->power.child_count, -1, 0); >> notify_parent = !parent->power.ignore_children; > > Kernel output difference, with some additional debug info: > > | --- GOOD 2016-10-25 15:46:19.669597124 +0200 > | +++ BAD 2016-10-25 15:48:15.201596869 +0200 > | @@ -5,24 +5,71 @@ Freezing remaining freezable tasks ... ( > | PM: Suspending system (mem) > | cpg_div6_clock_disable: sdhi1ck > | cpg_div6_clock_disable: sdhi0ck > | -PM: suspend of devices complete after 22.777 msecs > | -PM: late suspend of devices complete after 7.302 msecs > | +PM: suspend of devices complete after 22.932 msecs > | +PM: late suspend of devices complete after 7.311 msecs > | cpg_div6_clock_disable: mmc0 > | cpg_div6_clock_disable: zb > > pm_clk disables the clock of fec10000.bus. > > | +simple-pm-bus fec10000.bus: runtime PM trying to suspend device but > active child > > Suspend is aborted with -EBUSY, but zb stays disabled. > > | rmobile_pd_power_down: a3sp > | -PM: noirq suspend of devices complete after 18.424 msecs > | +PM: noirq suspend of devices complete after 26.937 msecs > | Disabling non-boot CPUs ... > | -Suspended for 7.196 seconds > | +Suspended for 2.579 seconds > | rmobile_pd_power_up: a3sp > | -cpg_div6_clock_enable: zb > > pm_clk doesn't enable the clock of fec10000.bus. > (since it thinks it's still suspended?) > > | cpg_div6_clock_enable: sdhi0ck > | cpg_div6_clock_enable: sdhi1ck > | cpg_div6_clock_enable: mmc0 > | -PM: noirq resume of devices complete after 134.580 msecs > | -PM: early resume of devices complete after 6.084 msecs > | -PM: resume of devices complete after 21.846 msecs > | -PM: Finishing wakeup. > | -Restarting tasks ... cpg_div6_clock_disable: mmc0 > | -done. > | +PM: noirq resume of devices complete after 128.014 msecs > | +PM: early resume of devices complete after 6.121 msecs > | +Unhandled fault: imprecise external abort (0x1406) at 0x00000000 > | +pgd = ee748000 > | +[00000000] *pgd=7fc42835 > | +Internal error: : 1406 [#1] SMP ARM > | +CPU: 0 PID: 1087 Comm: s2ram Not tainted > 4.9.0-rc2-ape6evm-01959-g90550a414a6a6cd3-dirty #505 > | +Hardware name: Generic R8A73A4 (Flattened Device Tree) > | +task: ee47a340 task.stack: ee686000 > | +PC is at smsc911x_resume+0x6c/0xbc > | +LR is at smsc911x_resume+0x6c/0xbc > > smsc911x_resume() tries to access the smsc911x while the bus clock zb is > not enabled. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Tested-by: Geert Uytterhoeven diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index e9b8579..65fca9c 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev) PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ | PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_); + pm_runtime_disable(dev); + pm_runtime_set_suspended(dev); + return 0; } @@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev) struct smsc911x_data *pdata = netdev_priv(ndev); unsigned int to = 100; + pm_runtime_enable(dev); + pm_runtime_resume(dev); + /* Note 3.11 from the datasheet: * "When the LAN9220 is in a power saving state, a write of any * data to the BYTE_TEST register will wake-up the device."