From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752640AbbARKzK (ORCPT ); Sun, 18 Jan 2015 05:55:10 -0500 Received: from mail-wi0-f178.google.com ([209.85.212.178]:54986 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751750AbbARKzG (ORCPT ); Sun, 18 Jan 2015 05:55:06 -0500 Message-ID: <54BB9100.7000506@samsung.com> Date: Sun, 18 Jan 2015 11:54:56 +0100 From: Krzysztof Kozlowski User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Tomasz Figa , Paul Osmialowski CC: Wolfram Sang , Jonathan Corbet , Mark Brown , Greg Kroah-Hartman , Kukjin Kim , "linux-i2c@vger.kernel.org" , "linux-doc@vger.kernel.org" , linux-kernel , "linux-samsung-soc@vger.kernel.org" , Michael Turquette , Peter De Schrijver , Russell King , Sylwester Nawrocki Subject: Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem References: <1421419194-1849-1-git-send-email-p.osmialowsk@samsung.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : >> This enhancement of i2c API is designed to address following problem >> caused by circular lock dependency: >> >> -> #1 (prepare_lock){+.+.+.}: >> [ 2.730502] [] __lock_acquire+0x3c0/0x8a4 >> [ 2.735970] [] lock_acquire+0x6c/0x8c >> [ 2.741090] [] mutex_lock_nested+0x68/0x464 >> [ 2.746733] [] clk_prepare_lock+0x40/0xe8 >> [ 2.752201] [] clk_unprepare+0x18/0x28 >> [ 2.757409] [] s3c24xx_i2c_xfer+0xc8/0x124 >> [ 2.762964] [] __i2c_transfer+0x74/0x8c >> [ 2.768259] [] i2c_transfer+0x78/0xec >> [ 2.773380] [] regmap_i2c_read+0x48/0x64 >> [ 2.778761] [] _regmap_raw_read+0xa8/0xfc >> [ 2.784230] [] _regmap_bus_read+0x28/0x48 >> [ 2.789699] [] _regmap_read+0x74/0xdc >> [ 2.794819] [] _regmap_update_bits+0x24/0x70 >> [ 2.800549] [] regmap_update_bits+0x40/0x5c >> [ 2.806190] [] _regulator_do_disable+0x68/0x7c >> [ 2.812093] [] _regulator_disable+0x90/0x12c >> [ 2.817822] [] regulator_disable+0x34/0x60 >> [ 2.823377] [] mmc_regulator_set_ocr+0x74/0xdc >> [ 2.829279] [] sdhci_set_power+0x38/0x20c >> [ 2.834748] [] sdhci_do_set_ios+0x180/0x450 >> [ 2.840389] [] sdhci_set_ios+0x20/0x2c >> [ 2.845597] [] mmc_set_initial_state+0x5c/0xbc >> [ 2.851500] [] mmc_power_off+0x2c/0x60 >> [ 2.856708] [] mmc_rescan+0x268/0x27c >> [ 2.861829] [] process_one_work+0x18c/0x3f8 >> [ 2.867471] [] worker_thread+0x38/0x2f8 >> [ 2.872766] [] kthread+0xe4/0x104 >> [ 2.877540] [] ret_from_fork+0x14/0x2c >> [ 2.882749] >> -> #0 (&map->mutex){+.+...}: >> [ 2.886828] [] validate_chain.isra.25+0x3bc/0x548 >> [ 2.892990] [] __lock_acquire+0x3c0/0x8a4 >> [ 2.898459] [] lock_acquire+0x6c/0x8c >> [ 2.903580] [] mutex_lock_nested+0x68/0x464 >> [ 2.909222] [] regmap_read+0x34/0x5c >> [ 2.914257] [] max_gen_clk_is_prepared+0x1c/0x38 >> [ 2.920333] [] clk_unprepare_unused_subtree+0x64/0x98 >> [ 2.926842] [] clk_disable_unused+0x80/0xd8 >> [ 2.932484] [] do_one_initcall+0xac/0x1f0 >> [ 2.937953] [] do_basic_setup+0x90/0xc8 >> [ 2.943248] [] kernel_init_freeable+0x84/0x120 >> [ 2.949150] [] kernel_init+0x8/0xec >> [ 2.954097] [] 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >