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: 9395561 X-Patchwork-Delegate: geert@linux-m68k.org Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1E47160236 for ; Tue, 25 Oct 2016 21:31:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EE7EE2955E for ; Tue, 25 Oct 2016 21:31:43 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E2F9C2976F; Tue, 25 Oct 2016 21:31:43 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7D44929749 for ; Tue, 25 Oct 2016 21:31:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938636AbcJYVb2 (ORCPT ); Tue, 25 Oct 2016 17:31:28 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:37580 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756359AbcJYVbZ (ORCPT ); Tue, 25 Oct 2016 17:31:25 -0400 Received: by mail-wm0-f43.google.com with SMTP id c78so48485975wme.0 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=evEM2hRd6zFn+aZMkRTQq9vv6TZwIv/iIyS92YbO8LY/tFfoOWWLFqRqABkBrDPEx0 TVDdCp4/tkpyWO01Jn/MFW9JIpQQNP+quKIqeRSqSa75XJ8YtGyx55P4PzXm/BzAuG3s Oc4rs3vhZmsmwDC44o9oMQZZHQD8AvNRCyRxfVGP2GobPV6xLn4F4JkrFagNRFwYoF/b dTyGVcI/TVvvhTU1v+HOCTLpZ/2WcFDOxe995ClERHEaRdhqc/rYenMlS1w2bcQlRC/J KBreKtLM6KL7surdDqQFQKXlepsKtd/8hIs4np8FP/Xcjx4JOaI3uHXOKnQ1uWDMsCFV KpaA== X-Gm-Message-State: ABUngveJ90N1qW+qsCgmt+1CJOkbbx/jTAmNUVYg+fq7xVsso4RAdZOGQiTs/F7f8rSB28YqCnjRhlYYRbEtpF6i 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-renesas-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-renesas-soc@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Tested-by: Geert Uytterhoeven --- 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 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."