| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 18 Jan 2015 11:54:56 +0100
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Tomasz Figa <tomasz.figa@...il.com>,
Paul Osmialowski <p.osmialowsk@...sung.com>
CC: Wolfram Sang <wsa@...-dreams.de>, Jonathan Corbet <corbet@....net>,
Mark Brown <broonie@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kukjin Kim <kgene@...nel.org>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
Michael Turquette <mturquette@...aro.org>,
Peter De Schrijver <pdeschrijver@...dia.com>,
Russell King <rmk+kernel@....linux.org.uk>,
Sylwester Nawrocki <s.nawrocki@...sung.com>
Subject: Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock
dependency problem
W dniu 18.01.2015 o 07:30, Tomasz Figa pisze:
> Hi,
>
> [CCing more people]
>
> 2015-01-16 23:39 GMT+09:00 Paul Osmialowski <p.osmialowsk@...sung.com>:
>> This enhancement of i2c API is designed to address following problem
>> caused by circular lock dependency:
>>
>> -> #1 (prepare_lock){+.+.+.}:
>> [ 2.730502] [<c0061e50>] __lock_acquire+0x3c0/0x8a4
>> [ 2.735970] [<c0062868>] lock_acquire+0x6c/0x8c
>> [ 2.741090] [<c04a2724>] mutex_lock_nested+0x68/0x464
>> [ 2.746733] [<c0395e84>] clk_prepare_lock+0x40/0xe8
>> [ 2.752201] [<c0397698>] clk_unprepare+0x18/0x28
>> [ 2.757409] [<c034cbb0>] s3c24xx_i2c_xfer+0xc8/0x124
>> [ 2.762964] [<c03457e0>] __i2c_transfer+0x74/0x8c
>> [ 2.768259] [<c0347234>] i2c_transfer+0x78/0xec
>> [ 2.773380] [<c02a177c>] regmap_i2c_read+0x48/0x64
>> [ 2.778761] [<c029d5c0>] _regmap_raw_read+0xa8/0xfc
>> [ 2.784230] [<c029d920>] _regmap_bus_read+0x28/0x48
>> [ 2.789699] [<c029ce00>] _regmap_read+0x74/0xdc
>> [ 2.794819] [<c029d0ec>] _regmap_update_bits+0x24/0x70
>> [ 2.800549] [<c029e348>] regmap_update_bits+0x40/0x5c
>> [ 2.806190] [<c024c3c4>] _regulator_do_disable+0x68/0x7c
>> [ 2.812093] [<c024f4d8>] _regulator_disable+0x90/0x12c
>> [ 2.817822] [<c024f5a8>] regulator_disable+0x34/0x60
>> [ 2.823377] [<c0363070>] mmc_regulator_set_ocr+0x74/0xdc
>> [ 2.829279] [<c03783e8>] sdhci_set_power+0x38/0x20c
>> [ 2.834748] [<c0379c10>] sdhci_do_set_ios+0x180/0x450
>> [ 2.840389] [<c0379f00>] sdhci_set_ios+0x20/0x2c
>> [ 2.845597] [<c0364978>] mmc_set_initial_state+0x5c/0xbc
>> [ 2.851500] [<c0364f48>] mmc_power_off+0x2c/0x60
>> [ 2.856708] [<c0365c00>] mmc_rescan+0x268/0x27c
>> [ 2.861829] [<c003a128>] process_one_work+0x18c/0x3f8
>> [ 2.867471] [<c003a400>] worker_thread+0x38/0x2f8
>> [ 2.872766] [<c003f66c>] kthread+0xe4/0x104
>> [ 2.877540] [<c000f0a8>] ret_from_fork+0x14/0x2c
>> [ 2.882749]
>> -> #0 (&map->mutex){+.+...}:
>> [ 2.886828] [<c0061224>] validate_chain.isra.25+0x3bc/0x548
>> [ 2.892990] [<c0061e50>] __lock_acquire+0x3c0/0x8a4
>> [ 2.898459] [<c0062868>] lock_acquire+0x6c/0x8c
>> [ 2.903580] [<c04a2724>] mutex_lock_nested+0x68/0x464
>> [ 2.909222] [<c029ce9c>] regmap_read+0x34/0x5c
>> [ 2.914257] [<c039a994>] max_gen_clk_is_prepared+0x1c/0x38
>> [ 2.920333] [<c0396ec4>] clk_unprepare_unused_subtree+0x64/0x98
>> [ 2.926842] [<c0396f78>] clk_disable_unused+0x80/0xd8
>> [ 2.932484] [<c00089d0>] do_one_initcall+0xac/0x1f0
>> [ 2.937953] [<c068bd44>] do_basic_setup+0x90/0xc8
>> [ 2.943248] [<c068be00>] kernel_init_freeable+0x84/0x120
>> [ 2.949150] [<c0491248>] kernel_init+0x8/0xec
>> [ 2.954097] [<c000f0a8>] ret_from_fork+0x14/0x2c
>> [ 2.959307]
>> [ 2.959307] other info that might help us debug this:
>> [ 2.959307]
>> [ 2.967293] Possible unsafe locking scenario:
>> [ 2.967293]
>> [ 2.973194] CPU0 CPU1
>> [ 2.977708] ---- ----
>> [ 2.982221] lock(prepare_lock);
>> [ 2.985520] lock(&map->mutex);
>> [ 2.991248] lock(prepare_lock);
>> [ 2.997063] lock(&map->mutex);
>> [ 3.000276]
>> [ 3.000276] *** DEADLOCK ***
>>
>> Apparently regulator and clock try to acquire lock which is protecting the
>> same regmap. Communication over i2c requires clock to be started. Both things
>> require access to the same regmap in order to complete.
>
> I stumbled upon this issue (and reported it) quite long time ago
> already, but apparently nobody cared too much (including myself,
> unfortunately). Please see [1] for details.
>
> [1] https://lkml.org/lkml/2014/7/2/171
>
> We have here a typical ABBA deadlock scenario, between I2C clock
> providers and other (logical) devices on the same (physical) I2C
> device, for which a regmap is used to share the registers between
> drivers. Remaining factor here is I2C controller driver, which must
> perform clock gating in I2C ops to trigger this deadlock.
>
> The problem is that any operation on such I2C clock will first try to
> acquire clk_prepare_mutex and then, through driver's callback, access
> the regmap, acquiring its mutex (then an I2C transfer will happen, but
> it irrelevant in this context). On opposite side we have drivers for
> other functionality exposed by this I2C device, which will access the
> regmap, acquiring its mutex and causing I2C transfers to happen.
>
> The key here is that I2C transfers might require some clocks to be
> prepared, so clk_prepare() might get called from this context and
> cause a deadlock, because clk_prepare_mutex might have been already
> acquired by another context, waiting for regmap mutex, which has been
> already acquired by this context.
>
> Now, for the solution, the approach proposed by Paul, as far as I
> could understand it by reading the code (it's definitely lacking a
> cover letter with detailed explanation), should solve the issue by
> enforcing correct locking order at regmap level. However I wonder if
> we really need a heavy solution like this or we could just make I2C
> drivers not require clock preparation in I2C transfers, as suggested
> by Peter De Schrijver in [1], which should fix the issue as well.
>
> So, the question is, do we actually have hardware that _really_
> requires _actual_ preparation or all the clk_prepare_enable()s in I2C
> drivers (at least in i2c-s3c2410) are just to simplify the code?
Hi Tomasz,
I completely forgot that you already thought about this deadlock in 2014. I
think we can try the no-prepare way for i2c-s3c2410. However this would be
only workaround for specific chip. Other buses (like SPI) would require
similar changes.
I wondered why this was not observed (at least not observed by me with
lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14
PMIC provides regulators and 32kHz clocks. I think it is exactly the same
pattern as for max77686... but somehow lockdep never reported that deadlock
there.
Anyway thanks for pointing out old discussion.
Best regards,
Krzysztof
>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists