From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754678AbcGEGeA (ORCPT ); Tue, 5 Jul 2016 02:34:00 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:20405 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751219AbcGEGd4 (ORCPT ); Tue, 5 Jul 2016 02:33:56 -0400 X-AuditID: cbfec7f5-f792a6d000001302-42-577b54d04cf6 Subject: Re: clk: Per controller locks (prepare & enable) To: Javier Martinez Canillas , Michael Turquette , Stephen Boyd , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel , Tomasz Figa , Sylwester Nawrocki References: <57737761.2020708@samsung.com> <577A1D54.9010203@samsung.com> <98eb9718-1267-dafe-2e36-323f4976ece0@osg.samsung.com> Cc: Bartlomiej Zolnierkiewicz , Marek Szyprowski , Andrzej Hajda From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <577B54CE.90004@samsung.com> Date: Tue, 05 Jul 2016 08:33:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-version: 1.0 In-reply-to: <98eb9718-1267-dafe-2e36-323f4976ece0@osg.samsung.com> Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpnkeLIzCtJLcpLzFFi42I5/e/4Zd0LIdXhBsc+CljcWneO1WLjjPWs Fm/ermGyeP3C0GLT42usFh977rFaXN41h81i7ZG77BYXT7laHH7Tzmrx40w3i8WqXX8YHXg8 3t9oZfe43NfL5LFz1l12j81L6j229AMZfVtWMXp83iQXwB7FZZOSmpNZllqkb5fAlfHu5mH2 gg65iscbp7M0MN4T7WLk5JAQMJGY8G06I4QtJnHh3nq2LkYuDiGBpYwSl558ZoVwnjFKnF6z lqWLkYNDWMBS4s6JMJC4iMB1JokH36awQxTtZZQ4svsMmMMs0Mco8XbvXXaQuWwCxhKbly9h g9ghJ9HbPYkFxOYV0JCY/uoVK8hUFgFVieUL9UHCogIRErO2/2CCKBGU+DH5Hlg5p4CzxOZ7 G8HKmQXUJaZMyQUJMwvIS2xe85Z5AqPgLCQdsxCqZiGpWsDIvIpRNLU0uaA4KT3XSK84Mbe4 NC9dLzk/dxMjJHq+7mBceszqEKMAB6MSD2/B/KpwIdbEsuLK3EOMEhzMSiK824Kqw4V4UxIr q1KL8uOLSnNSiw8xSnOwKInzztz1PkRIID2xJDU7NbUgtQgmy8TBKdXAeOWU/o54Pd65B2rX 6fNuXqHc1JJ4zfz1ky2T5CRLd3TznrGpdNp++Grfz4BGDQ/fpEUy2S4ME3q2ubxf3udtGlwy 7/+hr4yy09dHyh/7vFNqlx//rw8lrTkPGnjqVsU82bQ1z2G3nNNZ5w7Lvi+sK/yvHD/SHB1u 86T4qCKvw3Vhjzeq7SZXlFiKMxINtZiLihMBmW9Vs5oCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 07/04/2016 04:24 AM, Krzysztof Kozlowski wrote: >> On 06/30/2016 06:22 PM, Javier Martinez Canillas wrote: >>>> Question: >>>> What do you think about it? I know that talk is cheap and code looks >>>> better but before starting the work I would like to hear some >>>> comments/opinions/ideas. >>>> >>> >>> The problem is that the enable and prepare operations are propagated to >>> the parents, so what the locks want to protecting is really a sub-tree >>> of the clock tree. They currently protect the whole clock hierarchy to >>> make sure that the changes in the clock tree are atomically. >> >> Although there is a hierarchy between clocks from different controllers >> but still these are all clocks controllers coming from one hardware >> device (like SoC). At least on Exynos, I think there is no real >> inter-device dependencies. The deadlock you mentioned (and which I want >> to fix) is between: > > Yes, my point was that this may not be the case in all systems. IOW the > framework should be generic enough to allow hierarchies where a parent > clock is a clock provided by a different controller from a different HW. Is there such configuration? > >> 1. clock in PMIC (the one needed by s3c_rtc_probe()), >> 2. clock for I2C in SoC (the one needed by regmap_write()), >> 3. and regmap lock: >> >> What I want to say is that the relationship between clocks even when >> crossing clock controller boundaries is still self-contained. It is >> simple parent-child relationship so acquiring both >> clock-controller-level locks is safe. >> > > Is safe if the clock controllers are always aquired in the same order but > I'm not sure if that will always be the case. I.e: you have controllers A > and B that have clocks A{1,2} and B{1,2} respectively. So you could have > something like this: > > A1 with parent B1 > B2 with parent A2 Again, is there such configuration? We thought here about it and at least it is not known to us. Of course this is not a proof that such configuration does not exist... > > That can cause a deadlock since in the first case, the controller A will be > aquired and then the controller B but in the other case, the opposite order > will be attempted. Yes. > >> Current dead lock looks like, simplifying your code: >> A: B: >> lock(regmap) >> lock(prepare) >> lock(prepare) - wait >> lock(regmap) - wait >> >> >> When split locks per clock controller this would be: >> A: B: >> lock(regmap) >> lock(s2mps11) >> lock(i2c/exynos) >> lock(regmap) - wait >> do the transfer >> unlock(i2c/exynos) >> unlock(regmap) >> lock(regmap) - acquired >> lock(i2c/exynos) >> do the transfer >> unlock(i2c/exynos) >> unlock(regmap) >> unlock(s2mps11) >> > > Yes, splitting the lock per controller will fix the possible deadlock in > this case but I think we need an approach that is safe for all possible > scenarios. Otherwise it will work more by coincidence than due a design. This is not a coincidence. This design is meant to fix this deadlock. Not by coincidence. By design. You are talking about theoretical different configurations... without even real bug reports. I am providing an idea to fix a real deadlock and your argument is that it might not fix other (non-reported) deadlocks. These other deadlocks happen now as well probably... Best regards, Krzysztof