From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752278AbcDRHuN (ORCPT ); Mon, 18 Apr 2016 03:50:13 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:29427 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751011AbcDRHuK (ORCPT ); Mon, 18 Apr 2016 03:50:10 -0400 X-AuditID: cbfec7f5-f792a6d000001302-e9-571491afdeae Subject: Re: [PATCH] i2c: exynos5: Fix possible ABBA deadlock by keeping I2C clock prepared To: Javier Martinez Canillas , linux-kernel@vger.kernel.org References: <1460757887-18128-1-git-send-email-javier@osg.samsung.com> Cc: Anand Moon , Kukjin Kim , linux-samsung-soc@vger.kernel.org, Wolfram Sang , Krzysztof Kozlowski , linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org From: Marek Szyprowski Message-id: <571491AE.30908@samsung.com> Date: Mon, 18 Apr 2016 09:50:06 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-version: 1.0 In-reply-to: <1460757887-18128-1-git-send-email-javier@osg.samsung.com> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrPLMWRmVeSWpSXmKPExsVy+t/xK7rrJ4qEG+y9z2/x5u0aJovXLwwt +h+/ZrbY9Pgaq0XH3y+MFpd3zWGzmHF+H5PFuo232C1WnpjF7MDpsXPWXXaPTas62Tw2L6n3 2NIP5PVtWcXocfLUExaPz5vkAtijuGxSUnMyy1KL9O0SuDJO/d/LXNCiUHFx80bGBsZ1El2M nBwSAiYS3w4/YoawxSQu3FvP1sXIxSEksJRR4tq/X6wQznNGiZcPNrKAVAkLxEo8XHsUrENE IFTi38XbjCC2kICbRN+W12DdzAINTBLfD30Fa2ATMJToetvFBmLzCmhI9ExdBRZnEVCVmLt7 FxOILSoQI9H44BQTRI2gxI/J98BqOAXcJS4/2wC2jFnATOLLy8OsELa8xOY1b5knMArMQtIy C0nZLCRlCxiZVzGKppYmFxQnpeca6RUn5haX5qXrJefnbmKERMPXHYxLj1kdYhTgYFTi4Y1g EAkXYk0sK67MPcQowcGsJMKb0w4U4k1JrKxKLcqPLyrNSS0+xCjNwaIkzjtz1/sQIYH0xJLU 7NTUgtQimCwTB6dUA2O9RQM37/m2O4ma0wNXx7Sdey4eeGlPfH3MdW2DfpnTU/Z2PTjl7zah cXO5aEnWv1L+8tMNijeX/O6e/+SCuWE1772zeY4qawV0pExvzlb6s+/UcQWWBTc3vdGaOPf5 uofzr3kWPHJnKX6mzbbloXvFNOejMwOPnJ6y7sOcHZ9sVFiP1j1dxrBHiaU4I9FQi7moOBEA oURhYoICAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On 2016-04-16 00:04, Javier Martinez Canillas wrote: > The exynos5 I2C controller driver always prepares and enables a clock > before using it and then disables unprepares it when the clock is not > used anymore. > > But this can cause a possible ABBA deadlock in some scenarios since a > driver that uses regmap to access its I2C registers, will first grab > the regmap lock and then the I2C xfer function will grab the prepare > lock when preparing the I2C clock. But since the clock driver also > uses regmap for I2C accesses, preparing a clock will first grab the > prepare lock and then the regmap lock when using the regmap API. > > An example of this happens on the Exynos5422 Odroid XU board where a > s2mps11 PMIC is used and both the s2mps11 regulators and clk drivers > share the same I2C regmap. > > The possible deadlock is reported by the kernel lockdep: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(sec_core:428:(regmap)->lock); > lock(prepare_lock); > lock(sec_core:428:(regmap)->lock); > lock(prepare_lock); > > *** DEADLOCK *** > > Fix this by only preparing the clock on probe and {en,dis}able in the > rest of the driver. > > This patch is similar to commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA > deadlock by keeping clock prepared") that fixes the same bug in other > driver for an I2C controller found in Samsung SoCs. I'm sorry, but this is not the right approach imho. It is just a workaround applied to specific driver, it also duplicates incorrect clock usage pattern (there is really no point keeping clock prepared all the time). IMHO this ABBA deadlock should be really fixed in clocks core (probably by removing global prepare mutex and replacing it with per clock controller mutexes). Without a proper patch this issue will hit us again with other i2c controllers or other drivers as well. > Reported-by: Anand Moon > Signed-off-by: Javier Martinez Canillas > > --- > > drivers/i2c/busses/i2c-exynos5.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > index b29c7500461a..602633747149 100644 > --- a/drivers/i2c/busses/i2c-exynos5.c > +++ b/drivers/i2c/busses/i2c-exynos5.c > @@ -671,7 +671,9 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap, > return -EIO; > } > > - clk_prepare_enable(i2c->clk); > + ret = clk_enable(i2c->clk); > + if (ret) > + return ret; > > for (i = 0; i < num; i++, msgs++) { > stop = (i == num - 1); > @@ -695,7 +697,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap, > } > > out: > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > return ret; > } > > @@ -799,6 +801,10 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, i2c); > > + clk_disable(i2c->clk); > + > + return 0; > + > err_clk: > clk_disable_unprepare(i2c->clk); > return ret; > @@ -810,6 +816,8 @@ static int exynos5_i2c_remove(struct platform_device *pdev) > > i2c_del_adapter(&i2c->adap); > > + clk_unprepare(i2c->clk); > + > return 0; > } > > @@ -830,16 +838,18 @@ static int exynos5_i2c_resume_noirq(struct device *dev) > struct exynos5_i2c *i2c = platform_get_drvdata(pdev); > int ret = 0; > > - clk_prepare_enable(i2c->clk); > + ret = clk_enable(i2c->clk); > + if (ret) > + return ret; > > ret = exynos5_hsi2c_clock_setup(i2c); > if (ret) { > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > return ret; > } > > exynos5_i2c_init(i2c); > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > i2c->suspended = 0; > > return 0; Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland From mboxrd@z Thu Jan 1 00:00:00 1970 From: m.szyprowski@samsung.com (Marek Szyprowski) Date: Mon, 18 Apr 2016 09:50:06 +0200 Subject: [PATCH] i2c: exynos5: Fix possible ABBA deadlock by keeping I2C clock prepared In-Reply-To: <1460757887-18128-1-git-send-email-javier@osg.samsung.com> References: <1460757887-18128-1-git-send-email-javier@osg.samsung.com> Message-ID: <571491AE.30908@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello, On 2016-04-16 00:04, Javier Martinez Canillas wrote: > The exynos5 I2C controller driver always prepares and enables a clock > before using it and then disables unprepares it when the clock is not > used anymore. > > But this can cause a possible ABBA deadlock in some scenarios since a > driver that uses regmap to access its I2C registers, will first grab > the regmap lock and then the I2C xfer function will grab the prepare > lock when preparing the I2C clock. But since the clock driver also > uses regmap for I2C accesses, preparing a clock will first grab the > prepare lock and then the regmap lock when using the regmap API. > > An example of this happens on the Exynos5422 Odroid XU board where a > s2mps11 PMIC is used and both the s2mps11 regulators and clk drivers > share the same I2C regmap. > > The possible deadlock is reported by the kernel lockdep: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(sec_core:428:(regmap)->lock); > lock(prepare_lock); > lock(sec_core:428:(regmap)->lock); > lock(prepare_lock); > > *** DEADLOCK *** > > Fix this by only preparing the clock on probe and {en,dis}able in the > rest of the driver. > > This patch is similar to commit 34e81ad5f0b6 ("i2c: s3c2410: fix ABBA > deadlock by keeping clock prepared") that fixes the same bug in other > driver for an I2C controller found in Samsung SoCs. I'm sorry, but this is not the right approach imho. It is just a workaround applied to specific driver, it also duplicates incorrect clock usage pattern (there is really no point keeping clock prepared all the time). IMHO this ABBA deadlock should be really fixed in clocks core (probably by removing global prepare mutex and replacing it with per clock controller mutexes). Without a proper patch this issue will hit us again with other i2c controllers or other drivers as well. > Reported-by: Anand Moon > Signed-off-by: Javier Martinez Canillas > > --- > > drivers/i2c/busses/i2c-exynos5.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c > index b29c7500461a..602633747149 100644 > --- a/drivers/i2c/busses/i2c-exynos5.c > +++ b/drivers/i2c/busses/i2c-exynos5.c > @@ -671,7 +671,9 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap, > return -EIO; > } > > - clk_prepare_enable(i2c->clk); > + ret = clk_enable(i2c->clk); > + if (ret) > + return ret; > > for (i = 0; i < num; i++, msgs++) { > stop = (i == num - 1); > @@ -695,7 +697,7 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap, > } > > out: > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > return ret; > } > > @@ -799,6 +801,10 @@ static int exynos5_i2c_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, i2c); > > + clk_disable(i2c->clk); > + > + return 0; > + > err_clk: > clk_disable_unprepare(i2c->clk); > return ret; > @@ -810,6 +816,8 @@ static int exynos5_i2c_remove(struct platform_device *pdev) > > i2c_del_adapter(&i2c->adap); > > + clk_unprepare(i2c->clk); > + > return 0; > } > > @@ -830,16 +838,18 @@ static int exynos5_i2c_resume_noirq(struct device *dev) > struct exynos5_i2c *i2c = platform_get_drvdata(pdev); > int ret = 0; > > - clk_prepare_enable(i2c->clk); > + ret = clk_enable(i2c->clk); > + if (ret) > + return ret; > > ret = exynos5_hsi2c_clock_setup(i2c); > if (ret) { > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > return ret; > } > > exynos5_i2c_init(i2c); > - clk_disable_unprepare(i2c->clk); > + clk_disable(i2c->clk); > i2c->suspended = 0; > > return 0; Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland