Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
the following lock sequences:
1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
2. lock(&ec_dev->lock); lock(prepare_lock);
The actual dependency chains are much longer. The shortened version
looks somewhat like:
1. cros-ec-rpmsg on mtk-scp
ec_dev->lock -> prepare_lock
2. In rt5682_i2c_probe() on native I2C bus:
prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
3. In rt5682_i2c_probe() on native I2C bus:
regmap->lock -> i2c_adapter->bus_lock
4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
i2c_adapter->bus_lock -> ec_dev->lock
While lockdep is correct that the shared lockdep classes have a circular
dependency, it is bogus because
a) 2+3 happen on a native I2C bus
b) 4 happens on the actual EC on ChromeOS devices
c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
happen to expose a cros-ec interface, but do not have a passthrough
I2C bus
In short, the "dependencies" are actually on different devices.
Setup a per-device lockdep key for cros_ec devices so lockdep can tell
the two instances apart. This helps with getting rid of the bogus
lockdep warning. For ChromeOS devices that only have one cros-ec
instance this doesn't change anything.
Also add a missing mutex_destroy, just to make the teardown complete.
[1] This is likely the per I2C bus lock with shared lockdep class
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/platform/chrome/cros_ec.c | 14 +++++++++++---
include/linux/platform_data/cros_ec_proto.h | 2 ++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index ec733f683f34..4ae57820afd5 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -198,12 +198,14 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (!ec_dev->dout)
return -ENOMEM;
+ lockdep_register_key(&ec_dev->lockdep_key);
mutex_init(&ec_dev->lock);
+ lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
err = cros_ec_query_all(ec_dev);
if (err) {
dev_err(dev, "Cannot identify the EC: error %d\n", err);
- return err;
+ goto destroy_mutex;
}
if (ec_dev->irq > 0) {
@@ -215,7 +217,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (err) {
dev_err(dev, "Failed to request IRQ %d: %d\n",
ec_dev->irq, err);
- return err;
+ goto destroy_mutex;
}
}
@@ -226,7 +228,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (IS_ERR(ec_dev->ec)) {
dev_err(ec_dev->dev,
"Failed to create CrOS EC platform device\n");
- return PTR_ERR(ec_dev->ec);
+ err = PTR_ERR(ec_dev->ec);
+ goto destroy_mutex;
}
if (ec_dev->max_passthru) {
@@ -292,6 +295,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
exit:
platform_device_unregister(ec_dev->ec);
platform_device_unregister(ec_dev->pd);
+destroy_mutex:
+ mutex_destroy(&ec_dev->lock);
+ lockdep_unregister_key(&ec_dev->lockdep_key);
return err;
}
EXPORT_SYMBOL(cros_ec_register);
@@ -309,6 +315,8 @@ void cros_ec_unregister(struct cros_ec_device *ec_dev)
if (ec_dev->pd)
platform_device_unregister(ec_dev->pd);
platform_device_unregister(ec_dev->ec);
+ mutex_destroy(&ec_dev->lock);
+ lockdep_unregister_key(&ec_dev->lockdep_key);
}
EXPORT_SYMBOL(cros_ec_unregister);
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index e43107e0bee1..677d2eae1692 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -9,6 +9,7 @@
#define __LINUX_CROS_EC_PROTO_H
#include <linux/device.h>
+#include <linux/lockdep_types.h>
#include <linux/mutex.h>
#include <linux/notifier.h>
@@ -160,6 +161,7 @@ struct cros_ec_device {
struct cros_ec_command *msg);
int (*pkt_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
+ struct lock_class_key lockdep_key;
struct mutex lock;
u8 mkbp_event_supported;
bool host_sleep_v1;
--
2.39.0.314.g84b9a713c41-goog
On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> the following lock sequences:
>
> 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> 2. lock(&ec_dev->lock); lock(prepare_lock);
>
> The actual dependency chains are much longer. The shortened version
> looks somewhat like:
>
> 1. cros-ec-rpmsg on mtk-scp
> ec_dev->lock -> prepare_lock
> 2. In rt5682_i2c_probe() on native I2C bus:
> prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> 3. In rt5682_i2c_probe() on native I2C bus:
> regmap->lock -> i2c_adapter->bus_lock
> 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> i2c_adapter->bus_lock -> ec_dev->lock
>
> While lockdep is correct that the shared lockdep classes have a circular
> dependency, it is bogus because
>
> a) 2+3 happen on a native I2C bus
> b) 4 happens on the actual EC on ChromeOS devices
> c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> happen to expose a cros-ec interface, but do not have a passthrough
> I2C bus
>
> In short, the "dependencies" are actually on different devices.
Path of 4 looks weird to me.
Could you point out where sbs_probe() gets to acquire ec_dev->lock?
I may misunderstand: I thought there is no such I2C bus for passthrough
from kernel's point of view (as the bus and devices behind the EC).
See also [2].
[2]: https://elixir.bootlin.com/linux/v6.2-rc2/source/drivers/platform/chrome/cros_ec.c#L241
On a related note, for the commit title: s/chromeos/chrome/ if it gets
chance to have next version.
On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > the following lock sequences:
> >
> > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > 2. lock(&ec_dev->lock); lock(prepare_lock);
> >
> > The actual dependency chains are much longer. The shortened version
> > looks somewhat like:
> >
> > 1. cros-ec-rpmsg on mtk-scp
> > ec_dev->lock -> prepare_lock
> > 2. In rt5682_i2c_probe() on native I2C bus:
> > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > 3. In rt5682_i2c_probe() on native I2C bus:
> > regmap->lock -> i2c_adapter->bus_lock
> > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > i2c_adapter->bus_lock -> ec_dev->lock
> >
> > While lockdep is correct that the shared lockdep classes have a circular
> > dependency, it is bogus because
> >
> > a) 2+3 happen on a native I2C bus
> > b) 4 happens on the actual EC on ChromeOS devices
> > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > happen to expose a cros-ec interface, but do not have a passthrough
> > I2C bus
> >
> > In short, the "dependencies" are actually on different devices.
>
> Path of 4 looks weird to me.
>
> Could you point out where sbs_probe() gets to acquire ec_dev->lock?
sbs_probe() calls sbs_get_battery_presence_and_health(), which
-> does an I2C transfer. This SBS instance is connected on the I2C bus
on the EC, so the I2C transfer
-> acquires i2c_adapter->bus_lock, and
-> calls ec_i2c_xfer(), which
-> calls cros_ec_cmd_xfer_status(), which
-> calls cros_ec_cmd_xfer(), which
-> acquires ec_dev->lock
> I may misunderstand: I thought there is no such I2C bus for passthrough
> from kernel's point of view (as the bus and devices behind the EC).
> See also [2].
It is an I2C adapter on the EC, also known as i2c-cros-ec-tunnel.
Passthrough probably isn't the right word.
> [2]: https://elixir.bootlin.com/linux/v6.2-rc2/source/drivers/platform/chrome/cros_ec.c#L241
>
>
> On a related note, for the commit title: s/chromeos/chrome/ if it gets
> chance to have next version.
OK.
Thanks
ChenYu
On Sat, Jan 07, 2023 at 01:43:57PM +0800, Chen-Yu Tsai wrote:
> On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > > the following lock sequences:
> > >
> > > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > > 2. lock(&ec_dev->lock); lock(prepare_lock);
> > >
> > > The actual dependency chains are much longer. The shortened version
> > > looks somewhat like:
> > >
> > > 1. cros-ec-rpmsg on mtk-scp
> > > ec_dev->lock -> prepare_lock
> > > 2. In rt5682_i2c_probe() on native I2C bus:
> > > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > > 3. In rt5682_i2c_probe() on native I2C bus:
> > > regmap->lock -> i2c_adapter->bus_lock
> > > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > > i2c_adapter->bus_lock -> ec_dev->lock
> > >
> > > While lockdep is correct that the shared lockdep classes have a circular
> > > dependency, it is bogus because
> > >
> > > a) 2+3 happen on a native I2C bus
> > > b) 4 happens on the actual EC on ChromeOS devices
> > > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > > happen to expose a cros-ec interface, but do not have a passthrough
> > > I2C bus
> > >
> > > In short, the "dependencies" are actually on different devices.
> >
> > Path of 4 looks weird to me.
> >
> > Could you point out where sbs_probe() gets to acquire ec_dev->lock?
>
> sbs_probe() calls sbs_get_battery_presence_and_health(), which
>
> -> does an I2C transfer. This SBS instance is connected on the I2C bus
> on the EC, so the I2C transfer
>
> -> acquires i2c_adapter->bus_lock, and
I see.
Another question: the i2c_adapter here should be different from the native
I2C bus in 2 and 3. Did they really form the circular dependencies?
On Mon, Jan 9, 2023 at 1:46 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Sat, Jan 07, 2023 at 01:43:57PM +0800, Chen-Yu Tsai wrote:
> > On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
> > >
> > > On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > > > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > > > the following lock sequences:
> > > >
> > > > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > > > 2. lock(&ec_dev->lock); lock(prepare_lock);
> > > >
> > > > The actual dependency chains are much longer. The shortened version
> > > > looks somewhat like:
> > > >
> > > > 1. cros-ec-rpmsg on mtk-scp
> > > > ec_dev->lock -> prepare_lock
> > > > 2. In rt5682_i2c_probe() on native I2C bus:
> > > > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > > > 3. In rt5682_i2c_probe() on native I2C bus:
> > > > regmap->lock -> i2c_adapter->bus_lock
> > > > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > > > i2c_adapter->bus_lock -> ec_dev->lock
> > > >
> > > > While lockdep is correct that the shared lockdep classes have a circular
> > > > dependency, it is bogus because
> > > >
> > > > a) 2+3 happen on a native I2C bus
> > > > b) 4 happens on the actual EC on ChromeOS devices
> > > > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > > > happen to expose a cros-ec interface, but do not have a passthrough
> > > > I2C bus
> > > >
> > > > In short, the "dependencies" are actually on different devices.
> > >
> > > Path of 4 looks weird to me.
> > >
> > > Could you point out where sbs_probe() gets to acquire ec_dev->lock?
> >
> > sbs_probe() calls sbs_get_battery_presence_and_health(), which
> >
> > -> does an I2C transfer. This SBS instance is connected on the I2C bus
> > on the EC, so the I2C transfer
> >
> > -> acquires i2c_adapter->bus_lock, and
>
> I see.
>
> Another question: the i2c_adapter here should be different from the native
> I2C bus in 2 and 3. Did they really form the circular dependencies?
That's why it's a false positive. lockdep normally doesn't track individual
instances, only classes of locks. The class is declared as part of the
mutex_init() macro.
ChenYu
On Mon, Jan 09, 2023 at 02:19:38PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jan 9, 2023 at 1:46 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > On Sat, Jan 07, 2023 at 01:43:57PM +0800, Chen-Yu Tsai wrote:
> > > On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
> > > >
> > > > On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > > > > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > > > > the following lock sequences:
> > > > >
> > > > > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > > > > 2. lock(&ec_dev->lock); lock(prepare_lock);
> > > > >
> > > > > The actual dependency chains are much longer. The shortened version
> > > > > looks somewhat like:
> > > > >
> > > > > 1. cros-ec-rpmsg on mtk-scp
> > > > > ec_dev->lock -> prepare_lock
> > > > > 2. In rt5682_i2c_probe() on native I2C bus:
> > > > > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > > > > 3. In rt5682_i2c_probe() on native I2C bus:
> > > > > regmap->lock -> i2c_adapter->bus_lock
> > > > > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > > > > i2c_adapter->bus_lock -> ec_dev->lock
> > > > >
> > > > > While lockdep is correct that the shared lockdep classes have a circular
> > > > > dependency, it is bogus because
> > > > >
> > > > > a) 2+3 happen on a native I2C bus
> > > > > b) 4 happens on the actual EC on ChromeOS devices
> > > > > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > > > > happen to expose a cros-ec interface, but do not have a passthrough
> > > > > I2C bus
> > > > >
> > > > > In short, the "dependencies" are actually on different devices.
> > > >
> > > > Path of 4 looks weird to me.
> > > >
> > > > Could you point out where sbs_probe() gets to acquire ec_dev->lock?
> > >
> > > sbs_probe() calls sbs_get_battery_presence_and_health(), which
> > >
> > > -> does an I2C transfer. This SBS instance is connected on the I2C bus
> > > on the EC, so the I2C transfer
> > >
> > > -> acquires i2c_adapter->bus_lock, and
> >
> > I see.
> >
> > Another question: the i2c_adapter here should be different from the native
> > I2C bus in 2 and 3. Did they really form the circular dependencies?
>
> That's why it's a false positive. lockdep normally doesn't track individual
> instances, only classes of locks. The class is declared as part of the
> mutex_init() macro.
Is the following understanding correct:
It has 2 ways to break the "fake" circular dependencies: separate lockdep key
for i2c_adapter vs. ec_dev. The patch adopts the latter one because it has
limited impact for other I2C-related drivers.
On Mon, Jan 9, 2023 at 3:30 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Mon, Jan 09, 2023 at 02:19:38PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Jan 9, 2023 at 1:46 PM Tzung-Bi Shih <[email protected]> wrote:
> > >
> > > On Sat, Jan 07, 2023 at 01:43:57PM +0800, Chen-Yu Tsai wrote:
> > > > On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
> > > > >
> > > > > On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > > > > > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > > > > > the following lock sequences:
> > > > > >
> > > > > > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > > > > > 2. lock(&ec_dev->lock); lock(prepare_lock);
> > > > > >
> > > > > > The actual dependency chains are much longer. The shortened version
> > > > > > looks somewhat like:
> > > > > >
> > > > > > 1. cros-ec-rpmsg on mtk-scp
> > > > > > ec_dev->lock -> prepare_lock
> > > > > > 2. In rt5682_i2c_probe() on native I2C bus:
> > > > > > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > > > > > 3. In rt5682_i2c_probe() on native I2C bus:
> > > > > > regmap->lock -> i2c_adapter->bus_lock
> > > > > > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > > > > > i2c_adapter->bus_lock -> ec_dev->lock
> > > > > >
> > > > > > While lockdep is correct that the shared lockdep classes have a circular
> > > > > > dependency, it is bogus because
> > > > > >
> > > > > > a) 2+3 happen on a native I2C bus
> > > > > > b) 4 happens on the actual EC on ChromeOS devices
> > > > > > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > > > > > happen to expose a cros-ec interface, but do not have a passthrough
> > > > > > I2C bus
> > > > > >
> > > > > > In short, the "dependencies" are actually on different devices.
> > > > >
> > > > > Path of 4 looks weird to me.
> > > > >
> > > > > Could you point out where sbs_probe() gets to acquire ec_dev->lock?
> > > >
> > > > sbs_probe() calls sbs_get_battery_presence_and_health(), which
> > > >
> > > > -> does an I2C transfer. This SBS instance is connected on the I2C bus
> > > > on the EC, so the I2C transfer
> > > >
> > > > -> acquires i2c_adapter->bus_lock, and
> > >
> > > I see.
> > >
> > > Another question: the i2c_adapter here should be different from the native
> > > I2C bus in 2 and 3. Did they really form the circular dependencies?
> >
> > That's why it's a false positive. lockdep normally doesn't track individual
> > instances, only classes of locks. The class is declared as part of the
> > mutex_init() macro.
>
> Is the following understanding correct:
> It has 2 ways to break the "fake" circular dependencies: separate lockdep key
> for i2c_adapter vs. ec_dev. The patch adopts the latter one because it has
> limited impact for other I2C-related drivers.
That's correct.
On Mon, Jan 09, 2023 at 03:35:08PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jan 9, 2023 at 3:30 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > On Mon, Jan 09, 2023 at 02:19:38PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Jan 9, 2023 at 1:46 PM Tzung-Bi Shih <[email protected]> wrote:
> > > >
> > > > On Sat, Jan 07, 2023 at 01:43:57PM +0800, Chen-Yu Tsai wrote:
> > > > > On Fri, Jan 6, 2023 at 5:08 PM Tzung-Bi Shih <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Jan 06, 2023 at 12:55:37PM +0800, Chen-Yu Tsai wrote:
> > > > > > > Lockdep reports a bogus possible deadlock on MT8192 Chromebooks due to
> > > > > > > the following lock sequences:
> > > > > > >
> > > > > > > 1. lock(i2c_register_adapter) [1]; lock(&ec_dev->lock)
> > > > > > > 2. lock(&ec_dev->lock); lock(prepare_lock);
> > > > > > >
> > > > > > > The actual dependency chains are much longer. The shortened version
> > > > > > > looks somewhat like:
> > > > > > >
> > > > > > > 1. cros-ec-rpmsg on mtk-scp
> > > > > > > ec_dev->lock -> prepare_lock
> > > > > > > 2. In rt5682_i2c_probe() on native I2C bus:
> > > > > > > prepare_lock -> regmap->lock -> (possibly) i2c_adapter->bus_lock
> > > > > > > 3. In rt5682_i2c_probe() on native I2C bus:
> > > > > > > regmap->lock -> i2c_adapter->bus_lock
> > > > > > > 4. In sbs_probe() on cros-ec-i2c (passthrough) I2C bus on cros-ec
> > > > > > > i2c_adapter->bus_lock -> ec_dev->lock
> > > > > > >
> > > > > > > While lockdep is correct that the shared lockdep classes have a circular
> > > > > > > dependency, it is bogus because
> > > > > > >
> > > > > > > a) 2+3 happen on a native I2C bus
> > > > > > > b) 4 happens on the actual EC on ChromeOS devices
> > > > > > > c) 1 happens on the SCP coprocessor on MediaTek Chromebooks that just
> > > > > > > happen to expose a cros-ec interface, but do not have a passthrough
> > > > > > > I2C bus
> > > > > > >
> > > > > > > In short, the "dependencies" are actually on different devices.
> > > > > >
> > > > > > Path of 4 looks weird to me.
> > > > > >
> > > > > > Could you point out where sbs_probe() gets to acquire ec_dev->lock?
> > > > >
> > > > > sbs_probe() calls sbs_get_battery_presence_and_health(), which
> > > > >
> > > > > -> does an I2C transfer. This SBS instance is connected on the I2C bus
> > > > > on the EC, so the I2C transfer
> > > > >
> > > > > -> acquires i2c_adapter->bus_lock, and
> > > >
> > > > I see.
> > > >
> > > > Another question: the i2c_adapter here should be different from the native
> > > > I2C bus in 2 and 3. Did they really form the circular dependencies?
> > >
> > > That's why it's a false positive. lockdep normally doesn't track individual
> > > instances, only classes of locks. The class is declared as part of the
> > > mutex_init() macro.
> >
> > Is the following understanding correct:
> > It has 2 ways to break the "fake" circular dependencies: separate lockdep key
> > for i2c_adapter vs. ec_dev. The patch adopts the latter one because it has
> > limited impact for other I2C-related drivers.
>
> That's correct.
Thanks for the explanation. The patch looks good to me.
Just realized a kernel-doc warning after applying the patch:
$ ./scripts/kernel-doc -none include/linux/platform_data/cros_ec_proto.h
include/linux/platform_data/cros_ec_proto.h:199: warning: Function parameter
or member 'lockdep_key' not described in 'cros_ec_device'
Please fix the warning and commit title.