From patchwork Tue Feb 2 10:07:00 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulf Hansson X-Patchwork-Id: 8187761 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 33742BEEE5 for ; Tue, 2 Feb 2016 10:07:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2CD09202E9 for ; Tue, 2 Feb 2016 10:07:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 96E2F2013A for ; Tue, 2 Feb 2016 10:07:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754404AbcBBKHM (ORCPT ); Tue, 2 Feb 2016 05:07:12 -0500 Received: from mail-yk0-f178.google.com ([209.85.160.178]:34739 "EHLO mail-yk0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754381AbcBBKHB (ORCPT ); Tue, 2 Feb 2016 05:07:01 -0500 Received: by mail-yk0-f178.google.com with SMTP id u9so47463356ykd.1 for ; Tue, 02 Feb 2016 02:07:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=mx8iGoIbFdGwXtSEg8PjTzyTPTONrdHt7nL0pjY6HMY=; b=Kyf59MNs7iZKA/i+EozRWDBVttDyoc0ui2cGAXPdKIUoXHxLrQyGMB2nFHHI6d4Xvu BL+w4rXdFZFPWvTY4i5fUcAb5se6clUa43lAiw41kMJP7+m0YLLVaMhqQYLR3AOPSht9 JbIMBX/ArTBiUQssSekrTJ+I3HbP91OtP72gc= 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:date :message-id:subject:from:to:cc:content-type; bh=mx8iGoIbFdGwXtSEg8PjTzyTPTONrdHt7nL0pjY6HMY=; b=NIbG9FPdtEXf05fpQDX4shiVt9sMwSgQw5RLLE33I6E0uLS9dD20NBQxAdEj5Y8hvh moIJJRy/CZQ0nvZg1B+zzOgBFR1Yh7CPKAwgwO0LmBMAMqD8tVl6PMd1G/ial2hFJsBF bBLoU4AZGpVnhwBK7fdp0RnfI5B45NS9qwoZ4nmxYOYr75HjEjg2+GuHQbdz7we20Rxd YEhBX235/zhlbiDqJlwMaM3yUJ6wpIOJ0gOzASNwIJDQ3U+O15vDiInqp4HcV+S808d/ FHfUbaE4MZ80yImArusEqh8Eefx7eActfELHASGopvHcIjmZJKl3+kqi4Qo07zrJaBSp uHIA== X-Gm-Message-State: AG10YOSjFObty0odDeUiWfheVcNIo6SklSNZ4s9w5C1f1tylZMJ0WLkpOhjTzYsr3iP4JhnoR+Qvg29H3/dKofew MIME-Version: 1.0 X-Received: by 10.37.33.5 with SMTP id h5mr18147374ybh.170.1454407620639; Tue, 02 Feb 2016 02:07:00 -0800 (PST) Received: by 10.129.114.84 with HTTP; Tue, 2 Feb 2016 02:07:00 -0800 (PST) In-Reply-To: <20160202030533.GT19432@atomide.com> References: <20160201232833.GR19432@atomide.com> <20160202030533.GT19432@atomide.com> Date: Tue, 2 Feb 2016 11:07:00 +0100 Message-ID: Subject: Re: PM regression with commit 5de85b9d57ab PM runtime re-init in v4.5-rc1 From: Ulf Hansson To: Tony Lindgren , Alan Stern , "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Kevin Hilman , "linux-pm@vger.kernel.org" , Linux OMAP Mailing List , "linux-arm-kernel@lists.infradead.org" Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2 February 2016 at 04:05, Tony Lindgren wrote: > Hi, > > * Alan Stern [160201 15:50]: >> >> You get here only if runtime PM is disabled, right? So the rpm_idle >> call won't do anything -- "disabled" means don't make any callbacks. > > Hmm sorry yes I'm confused again. Yeah it looks like calling rpm_idle > just has a side effect that makes a difference here. > >> Tony, exactly what are you trying to do here? Do you want this to >> invoke a runtime-PM callback in the subsystem, power domain, or driver? >> (Is there even a driver bound to the device when this function runs?) > > I guess I need to add more printks to figure out what's going on here. > But yeah, I'm not seeing the callback happening at the interconnect > level so hardware and PM runtime states won't match on the following > probe after -EPROBE_DEFER. > >> The function's name suggests that it merely resets the data stored in >> dev->power without actually touching the hardware. Is that what you >> really want? > > I guess you mean pm_runtime_set_suspended() above? I'm seeing a state > where we now set pm_runtime_set_suspended() between failed device > probes and the device is still active in hardware. > > The patch below also helps with the problem and leaves out the > rpm_suspend() call from loop so it might give more hints. > > The difference here from what Rafael suggested earlier is calling > __pm_runtime_use_autosuspend() and then not calling > pm_runtime_set_suspended(). > > However, it seems the below patch keeps hardware active in the > autoidle case though, so chances are there is more that needs to > be done here. Anyways, I'll try to debug it more tomorrow. > Your observations is correct. The hardware will be kept active in-between the probe attempts (and thus also if probing fails). Although, that's not a regression as that's the behaviour you get from runtime PM, when drivers are implemented like omap_hsmmc. Instead of the suggested approaches, I think the regression should be fixed at the PM domain level (omap hwmod). I have attached a patch below, please give it try as it's untested. To solve the other problem (allowing devices to become inactive in-between at probe failures), I see two options (not treated as regressions). 1) Change the behaviour of pm_runtime_put_sync(), to *not* respect the autosuspend mode. I think I prefer this option, as it seems like autosuspend should be respected only via the asynchronous runtime PM APIs. 2) Change the failing drivers, to before calling pm_runtime_put_sync() also invoke pm_runtime_dont_use_autosusend(). Or other similar approach. Kind regards Uffe [...] From: Ulf Hansson Date: Tue, 2 Feb 2016 10:05:39 +0100 Subject: [PATCH] ARM: OMAP2+: omap-device: Allow devices to be pre-enabled Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") may re-initialize the runtime PM status of the device to RPM_SUSPENDED at driver probe failures. For the omap_hsmmc and likely also other omap drivers, which needs more than one attempt to ->probe() (returning -EPROBE_DEFER), this commit causes a regression at the PM domain level (omap hwmod). The reason is that the drivers don't put back the device into low power state while bailing out in ->probe to return -EPROBE_DEFER. This leads to that pm_runtime_reinit() in driver core, is re-initializing the runtime PM status from RPM_ACTIVE to RPM_SUSPENDED. The next ->probe() attempt then triggers the ->runtime_resume() callback to be invoked, which means this happens two times in a row. At the PM domain level (omap hwmod) this is being treated as an error and thus the runtime PM status of the device isn't correctly synchronized with the runtime PM core. In the end, ->probe() anyway succeeds (as the driver don't checks the error code from the runtime PM APIs), but results in that the PM domain always stays powered on. This because of the runtime PM core believes the device is RPM_SUSPENDED. Fix this regression by allowing devices to be pre-enabled when the PM domain's (omap hwmod) ->runtime_resume() callback is requested to enable the device. In such cases, return zero to synchronize the runtime PM status with the runtime PM core. Fixes: 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") Signed-off-by: Ulf Hansson --- arch/arm/mach-omap2/omap_device.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c index 0437537..851767f 100644 --- a/arch/arm/mach-omap2/omap_device.c +++ b/arch/arm/mach-omap2/omap_device.c @@ -717,7 +717,7 @@ int omap_device_register(struct platform_device *pdev) * to be accessible and ready to operate. This generally involves * enabling clocks, setting SYSCONFIG registers; and in the future may * involve remuxing pins. Device drivers should call this function - * indirectly via pm_runtime_get*(). Returns -EINVAL if called when + * indirectly via pm_runtime_get*(). Returns zero if called when * the omap_device is already enabled, or passes along the return * value of _omap_device_enable_hwmods(). */ @@ -728,12 +728,13 @@ int omap_device_enable(struct platform_device *pdev) od = to_omap_device(pdev); - if (od->_state == OMAP_DEVICE_STATE_ENABLED) { - dev_warn(&pdev->dev, - "omap_device: %s() called from invalid state %d\n", - __func__, od->_state); - return -EINVAL; - } + /* + * From the PM domain perspective the device may already be enabled. + * In such case, let's return zero to synchronize the state with the + * runtime PM core. + */ + if (od->_state == OMAP_DEVICE_STATE_ENABLED) + return 0; ret = _omap_device_enable_hwmods(od);