Skip to content
This repository has been archived by the owner on Sep 24, 2020. It is now read-only.

Commit

Permalink
i2c-cht-wc: Fix lockdep warning
Browse files Browse the repository at this point in the history
[ Upstream commit 232219b ]

When the kernel is build with lockdep support and the i2c-cht-wc driver is
used, the following warning is shown:

[   66.674334] ======================================================
[   66.674337] WARNING: possible circular locking dependency detected
[   66.674340] 5.3.0-rc4+ #83 Not tainted
[   66.674342] ------------------------------------------------------
[   66.674345] systemd-udevd/1232 is trying to acquire lock:
[   66.674349] 00000000a74dab07 (intel_soc_pmic_chtwc:167:(&cht_wc_regmap_cfg)->lock){+.+.}, at: regmap_write+0x31/0x70
[   66.674360]
               but task is already holding lock:
[   66.674362] 00000000d44a85b7 (i2c_register_adapter){+.+.}, at: i2c_smbus_xfer+0x49/0xf0
[   66.674370]
               which lock already depends on the new lock.

[   66.674371]
               the existing dependency chain (in reverse order) is:
[   66.674374]
               -> #1 (i2c_register_adapter){+.+.}:
[   66.674381]        rt_mutex_lock_nested+0x46/0x60
[   66.674384]        i2c_smbus_xfer+0x49/0xf0
[   66.674387]        i2c_smbus_read_byte_data+0x45/0x70
[   66.674391]        cht_wc_byte_reg_read+0x35/0x50
[   66.674394]        _regmap_read+0x63/0x1a0
[   66.674396]        _regmap_update_bits+0xa8/0xe0
[   66.674399]        regmap_update_bits_base+0x63/0xa0
[   66.674403]        regmap_irq_update_bits.isra.0+0x3b/0x50
[   66.674406]        regmap_add_irq_chip+0x592/0x7a0
[   66.674409]        devm_regmap_add_irq_chip+0x89/0xed
[   66.674412]        cht_wc_probe+0x102/0x158
[   66.674415]        i2c_device_probe+0x95/0x250
[   66.674419]        really_probe+0xf3/0x380
[   66.674422]        driver_probe_device+0x59/0xd0
[   66.674425]        device_driver_attach+0x53/0x60
[   66.674428]        __driver_attach+0x92/0x150
[   66.674431]        bus_for_each_dev+0x7d/0xc0
[   66.674434]        bus_add_driver+0x14d/0x1f0
[   66.674437]        driver_register+0x6d/0xb0
[   66.674440]        i2c_register_driver+0x45/0x80
[   66.674445]        do_one_initcall+0x60/0x2f4
[   66.674450]        kernel_init_freeable+0x20d/0x2b4
[   66.674453]        kernel_init+0xa/0x10c
[   66.674457]        ret_from_fork+0x3a/0x50
[   66.674459]
               -> #0 (intel_soc_pmic_chtwc:167:(&cht_wc_regmap_cfg)->lock){+.+.}:
[   66.674465]        __lock_acquire+0xe07/0x1930
[   66.674468]        lock_acquire+0x9d/0x1a0
[   66.674472]        __mutex_lock+0xa8/0x9a0
[   66.674474]        regmap_write+0x31/0x70
[   66.674480]        cht_wc_i2c_adap_smbus_xfer+0x72/0x240 [i2c_cht_wc]
[   66.674483]        __i2c_smbus_xfer+0x1a3/0x640
[   66.674486]        i2c_smbus_xfer+0x67/0xf0
[   66.674489]        i2c_smbus_read_byte_data+0x45/0x70
[   66.674494]        bq24190_probe+0x26b/0x410 [bq24190_charger]
[   66.674497]        i2c_device_probe+0x189/0x250
[   66.674500]        really_probe+0xf3/0x380
[   66.674503]        driver_probe_device+0x59/0xd0
[   66.674506]        device_driver_attach+0x53/0x60
[   66.674509]        __driver_attach+0x92/0x150
[   66.674512]        bus_for_each_dev+0x7d/0xc0
[   66.674515]        bus_add_driver+0x14d/0x1f0
[   66.674518]        driver_register+0x6d/0xb0
[   66.674521]        i2c_register_driver+0x45/0x80
[   66.674524]        do_one_initcall+0x60/0x2f4
[   66.674528]        do_init_module+0x5c/0x230
[   66.674531]        load_module+0x2707/0x2a20
[   66.674534]        __do_sys_init_module+0x188/0x1b0
[   66.674537]        do_syscall_64+0x5c/0xb0
[   66.674541]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   66.674543]
               other info that might help us debug this:

[   66.674545]  Possible unsafe locking scenario:

[   66.674547]        CPU0                    CPU1
[   66.674548]        ----                    ----
[   66.674550]   lock(i2c_register_adapter);
[   66.674553]                                lock(intel_soc_pmic_chtwc:167:(&cht_wc_regmap_cfg)->lock);
[   66.674556]                                lock(i2c_register_adapter);
[   66.674559]   lock(intel_soc_pmic_chtwc:167:(&cht_wc_regmap_cfg)->lock);
[   66.674561]
                *** DEADLOCK ***

The problem is that the CHT Whiskey Cove PMIC's builtin i2c-adapter is
itself a part of an i2c-client (the PMIC). This means that transfers done
through it take adapter->bus_lock twice, once for the parent i2c-adapter
and once for its own bus_lock. Lockdep does not like this nested locking.

To make lockdep happy in the case of busses with muxes, the i2c-core's
i2c_adapter_lock_bus function calls:

 rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));

But i2c_adapter_depth only works when the direct parent of the adapter is
another adapter, as it is only meant for muxes. In this case there is an
i2c-client and MFD instantiated platform_device in the parent->child chain
between the 2 devices.

This commit overrides the default i2c_lock_operations, passing a hardcoded
depth of 1 to rt_mutex_lock_nested, making lockdep happy.

Note that if there were to be a mux attached to the i2c-wc-cht adapter,
this would break things again since the i2c-mux code expects the
root-adapter to have a locking depth of 0. But the i2c-wc-cht adapter
always has only 1 client directly attached in the form of the charger IC
paired with the CHT Whiskey Cove PMIC.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
jwrdegoede authored and gregkh committed Oct 7, 2019
1 parent 371077e commit 70bb4bf
Showing 1 changed file with 46 additions and 0 deletions.
46 changes: 46 additions & 0 deletions drivers/i2c/busses/i2c-cht-wc.c
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,51 @@ static const struct i2c_algorithm cht_wc_i2c_adap_algo = {
.smbus_xfer = cht_wc_i2c_adap_smbus_xfer,
};

/*
* We are an i2c-adapter which itself is part of an i2c-client. This means that
* transfers done through us take adapter->bus_lock twice, once for our parent
* i2c-adapter and once to take our own bus_lock. Lockdep does not like this
* nested locking, to make lockdep happy in the case of busses with muxes, the
* i2c-core's i2c_adapter_lock_bus function calls:
* rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
*
* But i2c_adapter_depth only works when the direct parent of the adapter is
* another adapter, as it is only meant for muxes. In our case there is an
* i2c-client and MFD instantiated platform_device in the parent->child chain
* between the 2 devices.
*
* So we override the default i2c_lock_operations and pass a hardcoded
* depth of 1 to rt_mutex_lock_nested, to make lockdep happy.
*
* Note that if there were to be a mux attached to our adapter, this would
* break things again since the i2c-mux code expects the root-adapter to have
* a locking depth of 0. But we always have only 1 client directly attached
* in the form of the Charger IC paired with the CHT Whiskey Cove PMIC.
*/
static void cht_wc_i2c_adap_lock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
rt_mutex_lock_nested(&adapter->bus_lock, 1);
}

static int cht_wc_i2c_adap_trylock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
return rt_mutex_trylock(&adapter->bus_lock);
}

static void cht_wc_i2c_adap_unlock_bus(struct i2c_adapter *adapter,
unsigned int flags)
{
rt_mutex_unlock(&adapter->bus_lock);
}

static const struct i2c_lock_operations cht_wc_i2c_adap_lock_ops = {
.lock_bus = cht_wc_i2c_adap_lock_bus,
.trylock_bus = cht_wc_i2c_adap_trylock_bus,
.unlock_bus = cht_wc_i2c_adap_unlock_bus,
};

/**** irqchip for the client connected to the extchgr i2c adapter ****/
static void cht_wc_i2c_irq_lock(struct irq_data *data)
{
Expand Down Expand Up @@ -295,6 +340,7 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
adap->adapter.owner = THIS_MODULE;
adap->adapter.class = I2C_CLASS_HWMON;
adap->adapter.algo = &cht_wc_i2c_adap_algo;
adap->adapter.lock_ops = &cht_wc_i2c_adap_lock_ops;
strlcpy(adap->adapter.name, "PMIC I2C Adapter",
sizeof(adap->adapter.name));
adap->adapter.dev.parent = &pdev->dev;
Expand Down

0 comments on commit 70bb4bf

Please sign in to comment.