From patchwork Mon Sep 12 15:16:14 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bartosz Golaszewski X-Patchwork-Id: 76008 Delivered-To: patch@linaro.org Received: by 10.140.106.72 with SMTP id d66csp901287qgf; Mon, 12 Sep 2016 08:16:19 -0700 (PDT) X-Received: by 10.66.131.111 with SMTP id ol15mr34383581pab.105.1473693379242; Mon, 12 Sep 2016 08:16:19 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id bi6si22073612pad.256.2016.09.12.08.16.19; Mon, 12 Sep 2016 08:16:19 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-gpio-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@baylibre-com.20150623.gappssmtp.com; spf=pass (google.com: best guess record for domain of linux-gpio-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-gpio-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932861AbcILPQS (ORCPT + 4 others); Mon, 12 Sep 2016 11:16:18 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:36141 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932172AbcILPQQ (ORCPT ); Mon, 12 Sep 2016 11:16:16 -0400 Received: by mail-oi0-f41.google.com with SMTP id q188so202951649oia.3 for ; Mon, 12 Sep 2016 08:16:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=UjfiEYlbBzuigc+ROD9bx72+CLxgzqGn0V1AOUfyW4Y=; b=KNbR1TRsQt54wG4a5hvRO3dDOlkVmthtLwXEPBjqg1U4SeAxxXDV+T/XeBrzezAiKw wz6TFG6IEIUZOLR9J6kTTVn6iFrm6P+A5szYaE6MwWey6m0mEaVYAQPMwrY0daTMFQn9 2I2Clq3VW4w7VEcmUb3jQdlNdPi+86nupSNYLyyYGuqeWu38bXvcy6vz42GEgQJy8L3K reGZuMUJQjz1SC5K2Vc2RV+HnY1pWHlTNI8ekHcggVP4kgEmIJQ2Q2Oo71/1O8PIJs8j 14cZMPEr9khKfQfV+tUvkHBgq7aXdkMu0W27HszUnXdMQkYaw7RQBwr15bHxkUZGNxJK upsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=UjfiEYlbBzuigc+ROD9bx72+CLxgzqGn0V1AOUfyW4Y=; b=f9b/+lp9suOEMJVPg9arm3nXZz11umARMeRqL4yNUOD4FVyfHRzojsrdjMEMlOgnvn saEQYI3gNi0H7anuYD4v2mYd19Wg1CcI5rRbP6h6a8wScldE/qdvjJr1+woF7D6SkWhr MCC3ynJEMJeGhdyNKUgMhAIpq9ByqVzLOE7cZFpjeZIBJml9Zg1vMfRMKD30TSSU+Th+ QjSyIeuPVAiXVhsiQ9ViLg44UkImt2q1pidIC6AR6oqFOBShn5Y5hueHYbVJOyHiiGdg LpZs+es3Pj29/Uy6ujY+fK+etdRQeeofVbZTTtLIufjtYw0yFFRoluL4ouNdjYrMfWLD CGYA== X-Gm-Message-State: AE9vXwN/eL62DM3UqlzGh+IR9sV3aiVjboSR7s1lJovVzdeen/6NP1W93AumP1UOxeWi6gt6xMTxqsoGyr64s/Qn X-Received: by 10.202.214.141 with SMTP id n135mr11439215oig.160.1473693374917; Mon, 12 Sep 2016 08:16:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.156.170 with HTTP; Mon, 12 Sep 2016 08:16:14 -0700 (PDT) In-Reply-To: <20160912120938.GR10153@twins.programming.kicks-ass.net> References: <20160912120938.GR10153@twins.programming.kicks-ass.net> From: Bartosz Golaszewski Date: Mon, 12 Sep 2016 17:16:14 +0200 Message-ID: Subject: Re: lockdep: incorrect deadlock warning with two GPIO expanders To: Peter Zijlstra , Linus Walleij , Alexandre Courbot , Ingo Molnar Cc: LKML , linux-gpio Sender: linux-gpio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org + Linus, Alexandre, linux-gpio 2016-09-12 14:09 GMT+02:00 Peter Zijlstra : >2016-09-12 13:51 GMT+02:00 Bartosz Golaszewski : >> I'm trying to figure out a way of getting rid of an incorrect lockdep >> deadlock warning, but the issue is not trivial. >> >> In our hardware an I2C multiplexer is controlled by a GPIO provided by >> an expander. There's a second expander using the same device driver >> (pca953x) on one of the I2C bus segments. The diagram below presents >> the setup: >> >> - - - - - >> ------- --------- Bus segment 1 | | >> | | | |--------------- Devices >> | | SCL/SDA | | | | >> | Linux |-----------| I2C MUX | - - - - - >> | | | | | Bus segment 2 >> | | | | |------------------- >> ------- | --------- | >> | | - - - - - >> ------------ | MUX GPIO | | >> | | | Devices >> | GPIO | | | | >> | Expander 1 |---- - - - - - >> | | | >> ------------ | SCL/SDA >> | >> ------------ >> | | >> | GPIO | >> | Expander 2 | >> | | >> ------------ >> >> Lockdep prints a deadlock message when trying to set the direction or >> get/set the value of any GPIO provided by the second expander. >> >> The reason for the warning is that we take the chip->i2c_lock in >> pca953x_gpio_set/get_value() or pca953x_gpio_direction_input/output() >> and then come right back to pca953x_gpio_set_value() when the GPIO mux >> kicks in. The call path is as follows (starting from a fixed regulator >> controlled by a GPIO provided by the second expander): >> >> regulator_register() >> |_ _regulator_do_enable() >> |_ pca953x_gpio_set_value() <- mutex_lock >> |_ pca953x_write_single() >> |_ i2c_smbus_write_byte_data() >> |_ i2c_smbus_xfer() >> |_ i2c_transfer() >> |_ __i2c_transfer() >> |_ i2c_mux_master_xfer() >> |_ i2c_mux_gpio_select() >> |_ i2c_mux_gpio_set() >> |_ pca953x_gpio_set_value() <- 2nd mutex_lock >> >> The locks actually protect different expanders, but lockdep doesn't >> see this and says: >> >> Possible unsafe locking scenario: >> >> CPU0 >> ---- >> lock(&chip->i2c_lock); >> lock(&chip->i2c_lock); >> >> *** DEADLOCK *** >> >> May be due to missing lock nesting notation >> >> Using mutex_lock_nested(&chip->i2c_lock, SINGLE_DEPTH_NESTING) in >> pca953x_gpio_get_value() and pca953x_gpio_direction_input/output() >> helps for reading the values or setting the direction, but doesn't do >> anything if used in pca953x_gpio_set_value() since we still end up >> taking the lock of the same subclass again. >> >> It would require some nasty hacks to figure out that a GPIO is being >> used by an I2C mux if we wanted to explicitly provide a different >> sublass in this case, but that would not fix the culprit, since the >> same problem would occur in other gpio drivers under similar >> circumstances. >> >> It seems the problem is with the way lockdep works, but I lack the >> knowledge to propose any solution. >> >> Any help & ideas are appreciated. > > So I'm entirely clueless on how the device model works let alone i2c > and/or gpio. So I'm going to need some help as well. What's an SCL/SDA > for instance? > SCL/SDA stands for I2C clock and data lines. It's not important for the problem, I just used it to mark the physical layer between the SoC and I2C multiplexer. > So the 'problem' is that pca953x_probe()'s mutex_init() will collapse > all mutexes it initializes into a single class. It assumes that the > locking rules for all instances will be the same. > > This happens to not be true in this case. > Correct. > The tricky part, and here I have absolutely no clue what so ever, is > being able to tell at pca953x_probe() time that this is so. > AFAIK there is no clean way to tell that a GPIO is used by an I2C multiplexer at probe time. Linus, Alexandre could you confirm? > Once we can tell, at probe time, there are two different annotations we > could use, depending on need. > My current workaround is the following patch: It uses the fact that the two expanders we have are of different type (pca9534 and pca9535). The id pointer points to per-chip device info residing in .data which makes it suitable for mutex key. I don't think such hack is suitable for mainline though. > I suppose that theoretically you can keep nesting like that ad > infinitum, but I also expect that its uncommon enough, and maybe not > practical, to really nest like this -- seeing this is the first instance > of such issues. > > In any case, can you tell at probe time? And how deep a nesting should > we worry about? > In this case we would only need two classes - one for 'regular' GPIOs and another for the GPIOs used to control an I2C multiplexer. > Seeing how this lock is specific to the driver, and there is no generic > infrastructure, I don't see how we could solve it other than on a > per-driver basis. I'm planning to convert this driver to using regmap and I'm afraid we'll run into similar issue (given that regmap doesn't nest the default mutex nor uses different classes). Best regards, Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 5e3be32..40b96bf 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -739,6 +739,7 @@ static const struct of_device_id pca953x_dt_ids[]; static int pca953x_probe(struct i2c_client *client, const struct i2c_device_id *id) { + struct lock_class_key *lock_key = (struct lock_class_key *)id; struct pca953x_platform_data *pdata; struct pca953x_chip *chip; int irq_base = 0; @@ -783,7 +784,7 @@ static int pca953x_probe(struct i2c_client *client, chip->chip_type = PCA_CHIP_TYPE(chip->driver_data); - mutex_init(&chip->i2c_lock); + __mutex_init(&chip->i2c_lock, "chip->i2c_lock", lock_key); /* initialize cached registers from their original values. * we can't share this chip with another i2c master.