From patchwork Tue Jul 28 13:19:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomeu Vizoso X-Patchwork-Id: 6884971 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DA3CA9F380 for ; Tue, 28 Jul 2015 13:25:48 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0558C206CE for ; Tue, 28 Jul 2015 13:25:44 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CD82A206BE for ; Tue, 28 Jul 2015 13:25:42 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZK4qs-0006fq-7I; Tue, 28 Jul 2015 13:23:26 +0000 Received: from mail-wi0-x229.google.com ([2a00:1450:400c:c05::229]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZK4oi-0004w7-24 for linux-arm-kernel@lists.infradead.org; Tue, 28 Jul 2015 13:21:16 +0000 Received: by wicgb10 with SMTP id gb10so156498615wic.1 for ; Tue, 28 Jul 2015 06:20:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=64duXqqE+vzGu2/fDjl+pEUE4qlsjZNygGQxWp+h3Mc=; b=GYbiIRIJCJrTZRk88cjVR/26ncgijZsGwXTxKc7kMWVAqMPeGTHGDt1DOXcwvEB4rI nRB2TvidFYV7KRcy/vAfomcWfmqLgeug92SJS5u+nUdxRzaAWmeIU0triwpyAhFgdoH2 I5LLn2ktAcPrOOG7efBXX4aF934Bnc3h+1865kXkP6ZF7gfM3WCVjAro1Pq45pYWuOv/ 0re9VC8T74H2F0ygMkeOvKwjwHWukk1trPCE1N7Nwd9w7tke9DcUUMiD39Y1Igebnyab npRZQ+0JHu9zZZqKigFfA6CaFUuC0HVaRaLNEcOtd99RiqDHkgHZ+diFoE+5T2/tT+FP FX8w== X-Received: by 10.194.91.232 with SMTP id ch8mr62642738wjb.19.1438089650258; Tue, 28 Jul 2015 06:20:50 -0700 (PDT) Received: from cizrna.lan ([109.72.12.179]) by smtp.gmail.com with ESMTPSA id h9sm33265696wjx.20.2015.07.28.06.20.48 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Jul 2015 06:20:49 -0700 (PDT) From: Tomeu Vizoso To: linux-kernel@vger.kernel.org Subject: [PATCH v2 09/22] regulator: core: Reduce critical area in _regulator_get Date: Tue, 28 Jul 2015 15:19:40 +0200 Message-Id: <1438089593-7696-10-git-send-email-tomeu.vizoso@collabora.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1438089593-7696-1-git-send-email-tomeu.vizoso@collabora.com> References: <1438089593-7696-1-git-send-email-tomeu.vizoso@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150728_062112_472885_D3780040 X-CRM114-Status: GOOD ( 20.90 ) X-Spam-Score: -2.4 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, linux-acpi@vger.kernel.org, Arnd Bergmann , Stephen Warren , Linus Walleij , Dmitry Torokhov , "Rafael J. Wysocki" , Liam Girdwood , Tomeu Vizoso , Javier Martinez Canillas , Mark Brown , Thierry Reding , linux-arm-kernel@lists.infradead.org MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,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 ...by moving the locking of regulator_list_mutex into regulator_dev_lookup(), where it is iterated over. The regulator device lock gets acquired before returning, and the caller is responsible for releasing it after it's done with the regulator device. In _regulator_get() the regulator_list_mutex mutex is held for most of the function, but is only strictly needed to protect the list lookups. This change would be useful if for example regulator devices could be registered on demand when a driver requests them. regulator_register() could end up being called from within _regulator_get while the lock on regulator_list_mutex is being held, causing a deadlock. This backtrace illustrates the situation described above: (regulator_register) from [] (devm_regulator_register+0x48/0x84) (devm_regulator_register) from [] (reg_fixed_voltage_probe+0x214/0x35c) (reg_fixed_voltage_probe) from [] (platform_drv_probe+0x54/0xbc) (platform_drv_probe) from [] (driver_probe_device+0x184/0x2c4) (driver_probe_device) from [] (__device_attach+0x50/0x54) (__device_attach) from [] (bus_for_each_drv+0x70/0xa4) (bus_for_each_drv) from [] (device_attach+0x90/0xa4) (device_attach) from [] (bus_probe_device+0x94/0xb8) (bus_probe_device) from [] (device_add+0x384/0x580) (device_add) from [] (of_device_add+0x44/0x4c) (of_device_add) from [] ... (regulator_dev_lookup) from [] (_regulator_get+0x8c/0x26c) (_regulator_get) from [] (regulator_get+0x20/0x24) (regulator_get) from [] (_devm_regulator_get+0xa4/0xc8) (_devm_regulator_get) from [] (devm_regulator_get+0x1c/0x20) (devm_regulator_get) from [] (tegra_hdmi_probe+0xe0/0x278) (tegra_hdmi_probe) from [] (platform_drv_probe+0x54/0xbc) Signed-off-by: Tomeu Vizoso --- Changes in v2: - Acquire regulator device lock before returning from regulator_dev_lookup() drivers/regulator/core.c | 98 +++++++++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 43 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 613034667b93..65a3f28d452c 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -110,6 +110,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev, struct device *dev, const char *supply_name); static void _regulator_put(struct regulator *regulator); +static int _regulator_enable(struct regulator *regulator, bool do_lock); static const char *rdev_get_name(struct regulator_dev *rdev) { @@ -1212,6 +1213,7 @@ static void unset_regulator_supplies(struct regulator_dev *rdev) #define REG_STR_SIZE 64 +/* rdev->mutex held by caller */ static struct regulator *create_regulator(struct regulator_dev *rdev, struct device *dev, const char *supply_name) @@ -1224,7 +1226,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev, if (regulator == NULL) return NULL; - mutex_lock(&rdev->mutex); + lockdep_assert_held_once(&rdev->mutex); regulator->rdev = rdev; list_add(®ulator->list, &rdev->consumer_list); @@ -1276,12 +1278,10 @@ static struct regulator *create_regulator(struct regulator_dev *rdev, _regulator_is_enabled(rdev)) regulator->always_on = true; - mutex_unlock(&rdev->mutex); return regulator; overflow_err: list_del(®ulator->list); kfree(regulator); - mutex_unlock(&rdev->mutex); return NULL; } @@ -1320,6 +1320,7 @@ static void regulator_supply_alias(struct device **dev, const char **supply) } } +/* Caller has to release r->mutex once done with r */ static struct regulator_dev *regulator_dev_lookup(struct device *dev, const char *supply, int *ret) @@ -1335,10 +1336,15 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, if (dev && dev->of_node) { node = of_get_regulator(dev, supply); if (node) { + mutex_lock(®ulator_list_mutex); list_for_each_entry(r, ®ulator_list, list) if (r->dev.parent && - node == r->dev.of_node) + node == r->dev.of_node) { + mutex_lock(&r->mutex); + mutex_unlock(®ulator_list_mutex); return r; + } + mutex_unlock(®ulator_list_mutex); *ret = -EPROBE_DEFER; return NULL; } else { @@ -1356,9 +1362,14 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, if (dev) devname = dev_name(dev); + mutex_lock(®ulator_list_mutex); list_for_each_entry(r, ®ulator_list, list) - if (strcmp(rdev_get_name(r), supply) == 0) + if (strcmp(rdev_get_name(r), supply) == 0) { + mutex_lock(&r->mutex); + mutex_unlock(®ulator_list_mutex); return r; + } + mutex_unlock(®ulator_list_mutex); list_for_each_entry(map, ®ulator_map_list, list) { /* If the mapping has a device set up it must match */ @@ -1400,6 +1411,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) if (!r) { if (have_full_constraints()) { r = dummy_regulator_rdev; + mutex_lock(&r->mutex); } else { dev_err(dev, "Failed to resolve %s-supply for %s\n", rdev->supply_name, rdev->desc->name); @@ -1410,23 +1422,25 @@ static int regulator_resolve_supply(struct regulator_dev *rdev) /* Recursively resolve the supply of the supply */ ret = regulator_resolve_supply(r); if (ret < 0) - return ret; + goto out; ret = set_supply(rdev, r); if (ret < 0) - return ret; + goto out; /* Cascade always-on state to supply */ if (_regulator_is_enabled(rdev)) { - ret = regulator_enable(rdev->supply); + ret = _regulator_enable(rdev->supply, false); if (ret < 0) { if (rdev->supply) _regulator_put(rdev->supply); - return ret; + goto out; } } - return 0; +out: + mutex_unlock(&r->mutex); + return ret; } /* Internal regulator request function */ @@ -1451,8 +1465,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, else ret = -EPROBE_DEFER; - mutex_lock(®ulator_list_mutex); - rdev = regulator_dev_lookup(dev, id, &ret); if (rdev) goto found; @@ -1478,13 +1490,13 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, devname, id); rdev = dummy_regulator_rdev; + mutex_lock(&rdev->mutex); goto found; /* Don't log an error when called from regulator_get_optional() */ } else if (!have_full_constraints() || exclusive) { dev_warn(dev, "dummy supplies not allowed\n"); } - mutex_unlock(®ulator_list_mutex); return regulator; found: @@ -1526,8 +1538,7 @@ found: } out: - mutex_unlock(®ulator_list_mutex); - + mutex_unlock(&rdev->mutex); return regulator; } @@ -1987,11 +1998,22 @@ static int _regulator_do_enable(struct regulator_dev *rdev) } /* locks held by regulator_enable() */ -static int _regulator_enable(struct regulator_dev *rdev) +static int _regulator_enable(struct regulator *regulator, bool do_lock) { - int ret; + struct regulator_dev *rdev = regulator->rdev; + int ret = 0; - lockdep_assert_held_once(&rdev->mutex); + if (regulator->always_on) + return 0; + + if (rdev->supply) { + ret = regulator_enable(rdev->supply); + if (ret != 0) + return ret; + } + + if (do_lock) + mutex_lock(&rdev->mutex); /* check voltage and requested load before enabling */ if (rdev->constraints && @@ -2002,23 +2024,32 @@ static int _regulator_enable(struct regulator_dev *rdev) /* The regulator may on if it's not switchable or left on */ ret = _regulator_is_enabled(rdev); if (ret == -EINVAL || ret == 0) { - if (!_regulator_can_change_status(rdev)) - return -EPERM; + if (!_regulator_can_change_status(rdev)) { + ret = -EPERM; + goto out; + } ret = _regulator_do_enable(rdev); if (ret < 0) - return ret; + goto out; } else if (ret < 0) { rdev_err(rdev, "is_enabled() failed: %d\n", ret); - return ret; + goto out; } /* Fallthrough on positive return values - already enabled */ } rdev->use_count++; - return 0; +out: + if (do_lock) + mutex_unlock(&rdev->mutex); + + if (ret != 0 && rdev->supply) + regulator_disable(rdev->supply); + + return ret; } /** @@ -2034,26 +2065,7 @@ static int _regulator_enable(struct regulator_dev *rdev) */ int regulator_enable(struct regulator *regulator) { - struct regulator_dev *rdev = regulator->rdev; - int ret = 0; - - if (regulator->always_on) - return 0; - - if (rdev->supply) { - ret = regulator_enable(rdev->supply); - if (ret != 0) - return ret; - } - - mutex_lock(&rdev->mutex); - ret = _regulator_enable(rdev); - mutex_unlock(&rdev->mutex); - - if (ret != 0 && rdev->supply) - regulator_disable(rdev->supply); - - return ret; + return _regulator_enable(regulator, true); } EXPORT_SYMBOL_GPL(regulator_enable);