From patchwork Thu Aug 6 14:11:42 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomeu Vizoso X-Patchwork-Id: 6959661 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 3BBCC9F38B for ; Thu, 6 Aug 2015 14:23:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 13ABE202EB for ; Thu, 6 Aug 2015 14:22:58 +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 B40AD20274 for ; Thu, 6 Aug 2015 14:22:56 +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 1ZNM2L-0004ug-8x; Thu, 06 Aug 2015 14:20:49 +0000 Received: from mail-wi0-x22a.google.com ([2a00:1450:400c:c05::22a]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZNM0q-0002rB-5a for linux-arm-kernel@lists.infradead.org; Thu, 06 Aug 2015 14:19:21 +0000 Received: by wicne3 with SMTP id ne3so24672595wic.1 for ; Thu, 06 Aug 2015 07:18:55 -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=GMmIgxQwn9793tlKzjAa8Gu9FMvRTtGepMGpZtmckRM=; b=hwkPsQuD2qKs/EEYz3691+CT/r8t1VH01dMHKjCDZu0kT0Fk8u4rFo3PRkI3LYymqu p/ntW6Zv2daxlQS6MTQB88Viv5WldsXd4BTF9VANMmvGpuSziS+ibX8jKiFMWp3FrFUS 7JUZ7jvWEQ/ML9eu7YhKEGdGiWwfsWoMaSNVO+pqE/ED58NtR7QecRLETU8+HphSnxLs 6cDIU9uK4Yc6JXqHIoul9nCdgiKigSF7FSBmHfI9hSHH7dcCiIVzalX0mM4/LJNMjkBQ LtoX1k+O2gsXtmxl8RNOfZWP49TRcNj5LC6fGsDbeYpCjDHCiGfrpcB0E+6hCXaK6kU9 9Tbg== X-Received: by 10.194.58.109 with SMTP id p13mr4311206wjq.36.1438870735655; Thu, 06 Aug 2015 07:18:55 -0700 (PDT) Received: from cizrna.lan ([109.72.12.154]) by smtp.gmail.com with ESMTPSA id z5sm9727282wjr.42.2015.08.06.07.18.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Aug 2015 07:18:55 -0700 (PDT) From: Tomeu Vizoso To: linux-kernel@vger.kernel.org Subject: [PATCH v3 05/18] regulator: core: Reduce critical area in _regulator_get Date: Thu, 6 Aug 2015 16:11:42 +0200 Message-Id: <1438870315-18689-6-git-send-email-tomeu.vizoso@collabora.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1438870315-18689-1-git-send-email-tomeu.vizoso@collabora.com> References: <1438870315-18689-1-git-send-email-tomeu.vizoso@collabora.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150806_071916_709837_30E0DD76 X-CRM114-Status: GOOD ( 21.43 ) 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: Tomeu Vizoso , devicetree@vger.kernel.org, linux-acpi@vger.kernel.org, Arnd Bergmann , Stephen Warren , Linus Walleij , Dmitry Torokhov , "Rafael J. Wysocki" , Rob Herring , 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=-4.2 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 v3: - Avoid unlocking the regulator device's mutex if we don't have a device Changes in v2: - Acquire regulator device lock before returning from regulator_dev_lookup() drivers/regulator/core.c | 104 +++++++++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 45 deletions(-) diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 613034667b93..5f6c76142b08 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; @@ -1464,7 +1476,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id, * succeed, so, quit with appropriate error value */ if (ret && ret != -ENODEV) - goto out; + return regulator; if (!devname) devname = "deviceless"; @@ -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; } @@ -1986,12 +1997,24 @@ static int _regulator_do_enable(struct regulator_dev *rdev) return 0; } -/* 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); + else + lockdep_assert_held_once(&rdev->mutex); /* check voltage and requested load before enabling */ if (rdev->constraints && @@ -2002,23 +2025,33 @@ 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 */ + ret = 0; } 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 +2067,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);