From patchwork Wed Nov 18 10:48:39 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ulf Hansson X-Patchwork-Id: 56937 Delivered-To: patches@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp2485800lbb; Wed, 18 Nov 2015 02:48:56 -0800 (PST) X-Received: by 10.25.30.5 with SMTP id e5mr304100lfe.48.1447843736743; Wed, 18 Nov 2015 02:48:56 -0800 (PST) Return-Path: Received: from mail-lf0-x22a.google.com (mail-lf0-x22a.google.com. [2a00:1450:4010:c07::22a]) by mx.google.com with ESMTPS id u8si1437888lbb.149.2015.11.18.02.48.56 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 18 Nov 2015 02:48:56 -0800 (PST) Received-SPF: pass (google.com: domain of ulf.hansson@linaro.org designates 2a00:1450:4010:c07::22a as permitted sender) client-ip=2a00:1450:4010:c07::22a; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ulf.hansson@linaro.org designates 2a00:1450:4010:c07::22a as permitted sender) smtp.mailfrom=ulf.hansson@linaro.org; dkim=pass header.i=@linaro-org.20150623.gappssmtp.com Received: by lfaz4 with SMTP id z4so23669005lfa.0 for ; Wed, 18 Nov 2015 02:48:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro-org.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id; bh=FAwekRxrkgi62xz+l1sGMWev1f07gY4pndnjOvjY8fo=; b=PAVz8r9W1fkqdImKZtirmf+jNR/L/A9Q8tDM/QVVR+KEGuf+P3ayXOFrAn3brPX0cw UFWv1Kd+DuNT5c77jbEYyT4y+7dJ9XieanqbXbQMMvx4G30Ho8A87FohQusx5bBhHA57 Z4+vVPNwe6sX4cdTmdQNrY62/zfBJ5eYsqB9uj9+3KmvWKB9LBrX0iidBUOGi6By93L6 QzfZYOzu4OadCjP4p2Xjr63H1bnzX69jmw0PNvy+7RXPpzEJm/XGE4Pi09D7jIAW4Os8 LW0J5KOQXxfp7AQuI++VADOcBIuVZSmByj69RLWXrT1mQgq+6yb1jdLjwVl3U+4GOXZe dc3A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=FAwekRxrkgi62xz+l1sGMWev1f07gY4pndnjOvjY8fo=; b=QHhpCtKsjluWEv4LUK4CZuywWr1PJM9JhJzfCK3omATsq0T3lNgtzD0995IJ8yRdII EwgtoRO+g27m0x45r6iJT0X6v+6dmPftKTady1NF/vy0Bh5JsLxcbAZUGx6Br3uTIp2v sQs/BNjhT3M6vgDIZlylr7LRrQ3EJIhfXkQYA03NBIO0CVl4/fdyorN48KoMyZNbTCFa GFJlcLa1vAqqPVGzrvb7o2BORbToCr0h8TB7t80NGChWpmhIysadiyDSgAEQHMQNEj6O fGh5xb6VsTZGGNtXFP5ec9cCyH5IcR4OkujMO0rkb/QKiKnNepXtREnFQlvldH3Fvcff v3RA== X-Gm-Message-State: ALoCoQmrzDvXxCy+mtH42foz68dQtqJPZhAV6XodflDTsV3qMbTt7iD1ChLwzKPL7yUTwHQo2CfB X-Received: by 10.25.142.84 with SMTP id q81mr303158lfd.77.1447843736409; Wed, 18 Nov 2015 02:48:56 -0800 (PST) Return-Path: Received: from uffe-Latitude-E6430s.ideon.se ([85.235.10.227]) by smtp.gmail.com with ESMTPSA id 194sm343471lfd.4.2015.11.18.02.48.55 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 18 Nov 2015 02:48:55 -0800 (PST) From: Ulf Hansson To: "Rafael J. Wysocki" , Greg Kroah-Hartman , Alan Stern , linux-pm@vger.kernel.org Cc: Kevin Hilman , Len Brown , Pavel Machek , Geert Uytterhoeven , Kuninori Morimoto , Eduardo Valentin , Ulf Hansson Subject: [PATCH v2] Driver core: Re-init runtime PM states at probe error and driver unbind Date: Wed, 18 Nov 2015 11:48:39 +0100 Message-Id: <1447843719-9501-1-git-send-email-ulf.hansson@linaro.org> X-Mailer: git-send-email 1.9.1 There are two common expectations among several subsystems/drivers that deploys runtime PM support, but which isn't met by the driver core. Expectation 1) At ->probe() the subsystem/driver expects the runtime PM status of the device to be RPM_SUSPENDED, which is the initial status being assigned at device registration. This expectation is especially common among some of those subsystems/ drivers that manages devices with an attached PM domain, as those requires the ->runtime_resume() callback at the PM domain level to be invoked during ->probe(). Moreover these subsystems/drivers entirely relies on runtime PM resources being managed at the PM domain level, thus don't implement their own set of runtime PM callbacks. These are two scenarios that suffers from this unmet expectation. i) A failed ->probe() sequence requests probe deferral: ->probe() ... pm_runtime_enable() pm_runtime_get_sync() ... err: pm_runtime_put() pm_runtime_disable() ... As there are no guarantees that such sequence turns the runtime PM status of the device into RPM_SUSPENDED, the re-trying ->probe() may start with the status in RPM_ACTIVE. In such case the runtime PM core won't invoke the ->runtime_resume() callback because of a pm_runtime_get_sync(), as it considers the device to be already runtime resumed. ii) A driver re-bind sequence: At driver unbind, the subsystem/driver's >remove() callback invokes a sequence of runtime PM APIs, to undo actions during ->probe() and to put the device into low power state. ->remove() ... pm_runtime_put() pm_runtime_disable() ... Similar as in the failing ->probe() case, this sequence don't guarantee the runtime PM status of the device to turn into RPM_SUSPENDED. Trying to re-bind the driver thus causes the same issue as when re-trying ->probe(), in the probe deferral scenario. Expectation 2) Drivers that invokes the pm_runtime_irq_safe() API during ->probe(), triggers the runtime PM core to increase the usage count for the device's parent and permanently make it runtime resumed. The usage count is only dropped at device removal, which also allows it to be runtime suspended again. A re-trying ->probe() repeats the call to pm_runtime_irq_safe() and thus once more triggers the usage count of the device's parent to be increased. This leads to not only an imbalance issue of the usage count of the device's parent, but also to keep it runtime resumed permanently even if ->probe() fails. To address these issues, let's change the policy of the driver core to meet these expectations. More precisely, at ->probe() failures and driver unbind, restore the initial states of runtime PM. Although to still allow subsystem's to control PM for devices that doesn't ->probe() successfully, don't restore the initial states unless runtime PM is disabled. Signed-off-by: Ulf Hansson --- Changes in v2: - Folded patches into one patch, according to Rafael's request. - Add pm_runtime_reinit() and updated code from review by Alan. - Updated commit message header. --- drivers/base/dd.c | 2 ++ drivers/base/power/power.h | 2 ++ drivers/base/power/runtime.c | 26 ++++++++++++++++++++------ 3 files changed, 24 insertions(+), 6 deletions(-) -- 1.9.1 diff --git a/drivers/base/dd.c b/drivers/base/dd.c index a641cf3..cd2d79b 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -340,6 +340,7 @@ probe_failed: dev_set_drvdata(dev, NULL); if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); + pm_runtime_reinit(dev); switch (ret) { case -EPROBE_DEFER: @@ -695,6 +696,7 @@ static void __device_release_driver(struct device *dev) dev_set_drvdata(dev, NULL); if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); + pm_runtime_reinit(dev); klist_remove(&dev->p->knode_driver); if (dev->bus) diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h index 998fa6b..8b06193 100644 --- a/drivers/base/power/power.h +++ b/drivers/base/power/power.h @@ -18,6 +18,7 @@ static inline void pm_runtime_early_init(struct device *dev) } extern void pm_runtime_init(struct device *dev); +extern void pm_runtime_reinit(struct device *dev); extern void pm_runtime_remove(struct device *dev); struct wake_irq { @@ -84,6 +85,7 @@ static inline void pm_runtime_early_init(struct device *dev) } static inline void pm_runtime_init(struct device *dev) {} +static inline void pm_runtime_reinit(struct device *dev) {} static inline void pm_runtime_remove(struct device *dev) {} static inline int dpm_sysfs_add(struct device *dev) { return 0; } diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index e1a10a0..ab3fcd9 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1390,18 +1390,32 @@ void pm_runtime_init(struct device *dev) } /** + * pm_runtime_reinit - Re-initialize runtime PM fields in given device object. + * @dev: Device object to re-initialize. + */ +void pm_runtime_reinit(struct device *dev) +{ + if (!pm_runtime_enabled(dev)) { + if (dev->power.runtime_status == RPM_ACTIVE) + pm_runtime_set_suspended(dev); + if (dev->power.irq_safe) { + spin_lock_irq(&dev->power.lock); + dev->power.irq_safe = 0; + spin_unlock_irq(&dev->power.lock); + if (dev->parent) + pm_runtime_put(dev->parent); + } + } +} + +/** * pm_runtime_remove - Prepare for removing a device from device hierarchy. * @dev: Device object being removed from device hierarchy. */ void pm_runtime_remove(struct device *dev) { __pm_runtime_disable(dev, false); - - /* Change the status back to 'suspended' to match the initial status. */ - if (dev->power.runtime_status == RPM_ACTIVE) - pm_runtime_set_suspended(dev); - if (dev->power.irq_safe && dev->parent) - pm_runtime_put(dev->parent); + pm_runtime_reinit(dev); } /**