From patchwork Thu Sep 10 10:19:03 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thierry Reding X-Patchwork-Id: 7153211 Return-Path: X-Original-To: patchwork-linux-pm@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 1A657BEEC1 for ; Thu, 10 Sep 2015 10:19:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DD28B20869 for ; Thu, 10 Sep 2015 10:19:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CA0362085D for ; Thu, 10 Sep 2015 10:19:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752868AbbIJKTL (ORCPT ); Thu, 10 Sep 2015 06:19:11 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:35450 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752241AbbIJKTI (ORCPT ); Thu, 10 Sep 2015 06:19:08 -0400 Received: by pacfv12 with SMTP id fv12so40097802pac.2; Thu, 10 Sep 2015 03:19:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=dGqsHfNKUViZsuehKr44RB/jCQ3ZfvMqfzLhmUpsZLQ=; b=vkKWGiqnF6O3K/qnRpxTBjv2JYAS0McHsN3rD8IRCUXwAa55w1eeqXBW2iX4hl1vYh XqVg2WlCQbmuGvGtiyXRG57ogvoR4OiT2XXwc0EIFsVCvoIz6bleEk5L1tIDSIxlX32M fSwUcB4Ws8noO0yLY55CM5nhVOjwGYriWnjS2mvrPFxPAaL+HQ4711kqVAWnnSO38moU nWC7LBoroIcu9ZZeEmtqikmq0QAMB30qN3Pyvpmrpc0pT/TY1SlCcrkZG6P6OkVZQ4aU u6pzmcsZasOcWOZjqbuwLiz7d0mcYBYg5mehU8FN/Zi8MknV6mo8Z+q1WrY0q/EGfuBK j4lQ== X-Received: by 10.66.102.71 with SMTP id fm7mr71687400pab.5.1441880347657; Thu, 10 Sep 2015 03:19:07 -0700 (PDT) Received: from localhost (port-54762.pppoe.wtnet.de. [46.59.214.153]) by smtp.gmail.com with ESMTPSA id pi9sm11075857pbb.96.2015.09.10.03.19.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Sep 2015 03:19:07 -0700 (PDT) From: Thierry Reding To: Greg Kroah-Hartman Cc: "Rafael J. Wysocki" , Grygorii Strashko , Tomeu Vizoso , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] driver core: Ensure proper suspend/resume ordering Date: Thu, 10 Sep 2015 12:19:03 +0200 Message-Id: <1441880343-30852-1-git-send-email-thierry.reding@gmail.com> X-Mailer: git-send-email 2.5.0 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, 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 From: Thierry Reding Deferred probe can lead to strange situations where a device that is a dependency for others will be moved to the end of the dpm_list. At the same time the dependers may not be moved because at the time they will be probed the dependee may already have been successfully reprobed and they will not have to defer the probe themselves. One example where this happens is the Jetson TK1 board (Tegra124). The gpio-keys driver exposes the power key of the board as an input device that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM: tegra: Add gpio-ranges property") results in the gpio-tegra driver deferring probe because one of its dependencies, the pinctrl-tegra driver, has not successfully completed probing. Currently the deferred probe code will move the corresponding gpio-tegra device to the end of the dpm_list, but by the time the gpio-keys device, depending on the gpio-tegra device, is probed, gpio-tegra has already been reprobed, so the gpio-keys device is not moved to the end of dpm_list itself. As a result, the suspend ordering becomes pinctrl-tegra -> gpio-keys -> gpio-tegra. That's problematic because the gpio-keys driver requests the power key to be a wakeup source. However, the programming of the wakeup interrupt registers happens in the gpio-tegra driver's suspend callback, which is now called before that of the gpio-keys driver. The result is that the wrong values are programmed and leaves the system unable to be resumed using the power key. To fix this situation, always move devices to the end of the dpm_list before probing them. Technically this should only be done for devices that have been successfully probed, but that won't work for recursive probing of devices (think an I2C master that instantiates children in its ->probe()). Effectively the dpm_list will end up ordered the same way that devices were probed, hence taking care of dependencies. Signed-off-by: Thierry Reding --- Note that this commit is kind of the PM equivalent of 52cdbdd49853 ("driver core: correct device's shutdown order) and that we have two lists that are essentially the same (dpm_list and devices_kset). I'm wondering if it would be worth looking into getting rid of one of them? I don't see any reason why the ordering for shutdown and suspend/resume should be different, and having a single list would help keep this in sync. drivers/base/dd.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index be0eb4639128..56291b11049b 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work) */ mutex_unlock(&deferred_probe_mutex); - /* - * Force the device to the end of the dpm_list since - * the PM code assumes that the order we add things to - * the list is a good order for suspend but deferred - * probe makes that very unsafe. - */ - device_pm_lock(); - device_pm_move_last(dev); - device_pm_unlock(); - dev_dbg(dev, "Retrying from deferred list\n"); bus_probe_device(dev); @@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv) */ devices_kset_move_last(dev); + /* + * Force the device to the end of the dpm_list since the PM code + * assumes that the order we add things to the list is a good order + * for suspend but deferred probe makes that very unsafe. + * + * Deferred probe can also cause situations in which a device that is + * a dependency for others gets moved further down the dpm_list as a + * result of probe deferral. In that case the dependee will end up + * getting suspended before any of its dependers. + * + * To ensure proper ordering of suspend/resume, move every device that + * is being probed to the end of the dpm_list. Note that technically + * only successfully probed devices need to be moved, but that breaks + * for recursively added devices because they would end up in the list + * in reverse of the desired order, so we simply do it unconditionally + * for all devices before they are being probed. In the worst case the + * list will be reordered a couple more times than necessary, which + * should be an insignificant amount of work. + */ + device_pm_lock(); + device_pm_move_last(dev); + device_pm_unlock(); + if (dev->bus->probe) { ret = dev->bus->probe(dev); if (ret)