From patchwork Tue Jun 30 04:56:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Boichat X-Patchwork-Id: 6693341 Return-Path: X-Original-To: patchwork-alsa-devel@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 471DAC05AC for ; Tue, 30 Jun 2015 04:57:24 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 04D7C20612 for ; Tue, 30 Jun 2015 04:57:23 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id 12D6420520 for ; Tue, 30 Jun 2015 04:57:21 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id 7BA74265867; Tue, 30 Jun 2015 06:57:19 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=no version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id 3C6122606E1; Tue, 30 Jun 2015 06:57:11 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id AE91B2614E0; Tue, 30 Jun 2015 06:57:09 +0200 (CEST) Received: from mail-pa0-f45.google.com (mail-pa0-f45.google.com [209.85.220.45]) by alsa0.perex.cz (Postfix) with ESMTP id 463852606B0 for ; Tue, 30 Jun 2015 06:57:04 +0200 (CEST) Received: by pactm7 with SMTP id tm7so115804241pac.2 for ; Mon, 29 Jun 2015 21:57:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=K1BnbevlGGk5xs4qnZ1dqs51hHVQpoJhm8pSa2jYRmw=; b=lmMLHUuiDa5kBKkG99e/jyD8NEytIKWxGIm/iHeRjWsaafBttXk1AJMrUqEUhKtS+n rIZile5ajeE6epcUTsgPBeekb8CSWRWYuHgqJUgxjelgPiXYtC/5gOjzqo5y/xwCoyAp YRnfR0XqY55zyYXE1uc0N2b07Rv0EAcK/cnzA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=K1BnbevlGGk5xs4qnZ1dqs51hHVQpoJhm8pSa2jYRmw=; b=igST2VTBCXgxWTqVzNlh8O8hRrq4Dgzjqdkiost3RznnqSL4efPlkGdtc4Eu0TGtDW Mc5tMVVcjvUXmyGDKYm93/8uiq1rrq55yzvwC4xGv+ALXc3oKfEUI9FDk6j1D6+UnQJB s3yZrv3TSgiNom67C61ZaPdjvOcGPHeRVcKbU6jN0rH6pIIegsGVTpKVgWn8lCNhxINT ya2M/hsCXOMrYOs0JwMUq9rlTZS9hleqswAla3XwT9p3d89m9hXRdzWejjSexe8n7d+h Oedl9mzhvMul3CaHSUL8k5ke78rOxZwh4RCz0T+2pntV0pMKgFri+AF9D2mNTpXaP1uY XaVw== X-Gm-Message-State: ALoCoQlSoYu2pMogELAXkTV50NFdDYXtS1qnYgLX4ID0sYfujnI+/zJZyCxOQG1c3BvA8EGmo3ny X-Received: by 10.68.224.35 with SMTP id qz3mr39101669pbc.165.1435640222639; Mon, 29 Jun 2015 21:57:02 -0700 (PDT) Received: from google.com ([2401:fa00:1:b:1c47:b89d:32fb:9e63]) by mx.google.com with ESMTPSA id kh6sm43901388pbc.50.2015.06.29.21.56.59 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 29 Jun 2015 21:57:01 -0700 (PDT) Date: Tue, 30 Jun 2015 12:56:56 +0800 From: Nicolas Boichat To: Mark Brown Message-ID: <20150630045655.GA38694@google.com> References: <558C1824.8020204@metafoo.de> <20150625153325.GR14071@sirena.org.uk> <558C229D.4090409@metafoo.de> <20150625160817.GT14071@sirena.org.uk> <5591414D.6080802@metafoo.de> <559153A3.1000006@metafoo.de> <20150629153411.GG11162@sirena.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150629153411.GG11162@sirena.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Oder Chiou , alsa-devel@alsa-project.org, Lars-Peter Clausen , Anatol Pomozov , Mauro Carvalho Chehab , Takashi Iwai , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Liam Girdwood , Antti Palosaari , Bard Liao , Arjan van de Ven , Ingo Molnar Subject: Re: [alsa-devel] [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jun 29, 2015 at 04:34:11PM +0100, Mark Brown wrote: > On Mon, Jun 29, 2015 at 04:18:11PM +0200, Lars-Peter Clausen wrote: > > Leaves us pretty much with only two options. Either add a lock key pointer > > to regmap_config which needs to be manually initialized. Or wrap all > > regmap_init() variants to create a static lock key. I'd slightly prefer the > > later. We can avoid most of the boiler-plate code by using some helper > > macros to generate the wrappers. > > It's better to keep the bodges in the core, yes. Partial attempt below. Of course all other _init functions will need to be converted as well. I'd like to get feedback before I do the rest of the work. The macro part is quite repetitive and I don't think it can be simplified. Thanks! >8------------------------------------------------------8< Subject: [PATCH] regmap: Use different lockdep classes for each regmap init call Lockdep validator complains about recursive locking and deadlock when two different regmap instances are called in a nested order. That happens anytime a regmap read/write call needs to access another regmap. This is because, for performance reason, lockdep groups all locks initialized by the same mutex_init() in the same lock class. Therefore all regmap mutexes are in the same lock class, leading to lockdep "nested locking" warnings if a regmap accesses another regmap. In general, it is impossible to establish in advance the hierarchy of regmaps, so we make sure that each regmap init call initializes its own static lock_class_key. This is done by wrapping all regmap_init calls into macros. This also allows us to give meaningful names to the lock_class_key. For example, in rt5677 case, we have in /proc/lockdep_chains: irq_context: 0 [ffffffc0018d2198] &dev->mutex [ffffffc0018d2198] &dev->mutex [ffffffc001bd7f60] rt5677:5104:(&rt5677_regmap)->_lock [ffffffc001bd7f58] rt5677:5096:(&rt5677_regmap_physical)->_lock [ffffffc001b95448] &(&base->lock)->rlock The above would have resulted in a lockdep recursive warning previously. This is not the case anymore as the lockdep validator now clearly identifies the 2 locks as separate. Signed-off-by: Nicolas Boichat --- drivers/base/regmap/regmap-i2c.c | 22 ++++++---- drivers/base/regmap/regmap.c | 31 +++++++++----- include/linux/regmap.h | 91 ++++++++++++++++++++++++++++++++++------ 3 files changed, 113 insertions(+), 31 deletions(-) diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c index 053150a..c1f9396 100644 --- a/drivers/base/regmap/regmap-i2c.c +++ b/drivers/base/regmap/regmap-i2c.c @@ -198,17 +198,20 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, * The return value will be an ERR_PTR() on error or a valid pointer to * a struct regmap. */ -struct regmap *regmap_init_i2c(struct i2c_client *i2c, - const struct regmap_config *config) +struct regmap *__regmap_init_i2c(struct i2c_client *i2c, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) { const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config); if (IS_ERR(bus)) return ERR_CAST(bus); - return regmap_init(&i2c->dev, bus, &i2c->dev, config); + return __regmap_init(&i2c->dev, bus, &i2c->dev, config, + lock_key, lock_name); } -EXPORT_SYMBOL_GPL(regmap_init_i2c); +EXPORT_SYMBOL_GPL(__regmap_init_i2c); /** * devm_regmap_init_i2c(): Initialise managed register map @@ -220,16 +223,19 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c); * to a struct regmap. The regmap will be automatically freed by the * device management code. */ -struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c, - const struct regmap_config *config) +struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) { const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config); if (IS_ERR(bus)) return ERR_CAST(bus); - return devm_regmap_init(&i2c->dev, bus, &i2c->dev, config); + return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config, + lock_key, lock_name); } -EXPORT_SYMBOL_GPL(devm_regmap_init_i2c); +EXPORT_SYMBOL_GPL(__devm_regmap_init_i2c); MODULE_LICENSE("GPL"); diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index d2f8a81..b8e26af 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -526,10 +526,12 @@ static enum regmap_endian regmap_get_val_endian(struct device *dev, * a struct regmap. This function should generally not be called * directly, it should be called by bus-specific init functions. */ -struct regmap *regmap_init(struct device *dev, - const struct regmap_bus *bus, - void *bus_context, - const struct regmap_config *config) +struct regmap *__regmap_init(struct device *dev, + const struct regmap_bus *bus, + void *bus_context, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) { struct regmap *map; int ret = -EINVAL; @@ -555,10 +557,14 @@ struct regmap *regmap_init(struct device *dev, spin_lock_init(&map->spinlock); map->lock = regmap_lock_spinlock; map->unlock = regmap_unlock_spinlock; + lockdep_set_class_and_name(&map->spinlock, + lock_key, lock_name); } else { mutex_init(&map->mutex); map->lock = regmap_lock_mutex; map->unlock = regmap_unlock_mutex; + lockdep_set_class_and_name(&map->mutex, + lock_key, lock_name); } map->lock_arg = map; } @@ -898,7 +904,7 @@ err_map: err: return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(regmap_init); +EXPORT_SYMBOL_GPL(__regmap_init); static void devm_regmap_release(struct device *dev, void *res) { @@ -918,10 +924,12 @@ static void devm_regmap_release(struct device *dev, void *res) * directly, it should be called by bus-specific init functions. The * map will be automatically freed by the device management code. */ -struct regmap *devm_regmap_init(struct device *dev, - const struct regmap_bus *bus, - void *bus_context, - const struct regmap_config *config) +struct regmap *__devm_regmap_init(struct device *dev, + const struct regmap_bus *bus, + void *bus_context, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) { struct regmap **ptr, *regmap; @@ -929,7 +937,8 @@ struct regmap *devm_regmap_init(struct device *dev, if (!ptr) return ERR_PTR(-ENOMEM); - regmap = regmap_init(dev, bus, bus_context, config); + regmap = __regmap_init(dev, bus, bus_context, config, + lock_key, lock_name); if (!IS_ERR(regmap)) { *ptr = regmap; devres_add(dev, ptr); @@ -939,7 +948,7 @@ struct regmap *devm_regmap_init(struct device *dev, return regmap; } -EXPORT_SYMBOL_GPL(devm_regmap_init); +EXPORT_SYMBOL_GPL(__devm_regmap_init); static void regmap_field_init(struct regmap_field *rm_field, struct regmap *regmap, struct reg_field reg_field) diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 1318e935..9505bca 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -17,6 +17,7 @@ #include #include #include +#include struct module; struct device; @@ -323,14 +324,18 @@ struct regmap_bus { enum regmap_endian val_format_endian_default; }; -struct regmap *regmap_init(struct device *dev, - const struct regmap_bus *bus, - void *bus_context, - const struct regmap_config *config); +struct regmap *__regmap_init(struct device *dev, + const struct regmap_bus *bus, + void *bus_context, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); int regmap_attach_dev(struct device *dev, struct regmap *map, const struct regmap_config *config); -struct regmap *regmap_init_i2c(struct i2c_client *i2c, - const struct regmap_config *config); +struct regmap *__regmap_init_i2c(struct i2c_client *i2c, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); struct regmap *regmap_init_spi(struct spi_device *dev, const struct regmap_config *config); struct regmap *regmap_init_spmi_base(struct spmi_device *dev, @@ -341,12 +346,16 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id, void __iomem *regs, const struct regmap_config *config); -struct regmap *devm_regmap_init(struct device *dev, - const struct regmap_bus *bus, - void *bus_context, - const struct regmap_config *config); -struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c, - const struct regmap_config *config); +struct regmap *__devm_regmap_init(struct device *dev, + const struct regmap_bus *bus, + void *bus_context, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); +struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); struct regmap *devm_regmap_init_spi(struct spi_device *dev, const struct regmap_config *config); struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev, @@ -392,6 +401,64 @@ static inline struct regmap *devm_regmap_init_mmio(struct device *dev, return devm_regmap_init_mmio_clk(dev, NULL, regs, config); } +#ifdef CONFIG_LOCKDEP +#define regmap_init(dev, bus, bus_context, config) \ +( \ + ({ \ + static struct lock_class_key _key; \ + __regmap_init(dev, bus, bus_context, config, \ + &_key, \ + KBUILD_BASENAME ":" \ + __stringify(__LINE__) ":" \ + "(" #config ")->_lock"); \ + }) \ +) +#define regmap_init_i2c(i2c, config) \ +( \ + ({ \ + static struct lock_class_key _key; \ + __regmap_init_i2c(i2c, config, \ + &_key, \ + KBUILD_BASENAME ":" \ + __stringify(__LINE__) ":" \ + "(" #config ")->_lock"); \ + }) \ +) + +#define devm_regmap_init(dev, bus, bus_context, config) \ +( \ + ({ \ + static struct lock_class_key _key; \ + __devm_regmap_init(dev, bus, bus_context, config, \ + &_key, \ + KBUILD_BASENAME ":" \ + __stringify(__LINE__) ":" \ + "(" #config ")->_lock"); \ + }) \ +) +#define devm_regmap_init_i2c(i2c, config) \ +( \ + ({ \ + static struct lock_class_key _key; \ + __devm_regmap_init_i2c(i2c, config, \ + &_key, \ + KBUILD_BASENAME ":" \ + __stringify(__LINE__) ":" \ + "(" #config ")->_lock"); \ + }) \ +) +#else +#define regmap_init(dev, bus, bus_context, config) \ + __regmap_init(dev, bus, bus_context, config, NULL, NULL) +#define regmap_init_i2c(dev, bus, bus_context, config) \ + __regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL) + +#define devm_regmap_init(dev, bus, bus_context, config) \ + __devm_regmap_init(dev, bus, bus_context, config, NULL, NULL) +#define devm_regmap_init_i2c(dev, bus, bus_context, config) \ + __devm_regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL) +#endif + void regmap_exit(struct regmap *map); int regmap_reinit_cache(struct regmap *map, const struct regmap_config *config);