From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751530AbcGGMm3 (ORCPT ); Thu, 7 Jul 2016 08:42:29 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:65392 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750859AbcGGMmW (ORCPT ); Thu, 7 Jul 2016 08:42:22 -0400 X-AuditID: cbfec7f4-f796c6d000001486-44-577e4e2a1023 Subject: Re: clk: Per controller locks (prepare & enable) To: Charles Keepax , Javier Martinez Canillas References: <57737761.2020708@samsung.com> <577A1D54.9010203@samsung.com> <98eb9718-1267-dafe-2e36-323f4976ece0@osg.samsung.com> <577B54CE.90004@samsung.com> <913d98f6-0d7c-63e3-8748-961eafd776f4@osg.samsung.com> <20160707120626.GE1835@localhost.localdomain> Cc: Michael Turquette , Stephen Boyd , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel , Tomasz Figa , Sylwester Nawrocki , Andrzej Hajda , Marek Szyprowski , Bartlomiej Zolnierkiewicz From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <577E4E29.5080103@samsung.com> Date: Thu, 07 Jul 2016 14:42:17 +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: <20160707120626.GE1835@localhost.localdomain> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprAIsWRmVeSWpSXmKPExsVy+t/xq7pafnXhBjunGVjcWneO1WLjjPWs Fv+m3GC3ePN2DZPF6xeGFpseX2O1+Nhzj9Xi8q45bBZrj9xlt7h4ytXi8Jt2VosfZ7pZLFbt +sPowOvx/kYru8flvl4mj52z7rJ7bF5S7/Fy4m82jy39QF7fllWMHp83yQVwRHHZpKTmZJal FunbJXBldDw5xFLQqVAx//9q1gbG6ZJdjJwcEgImEhf+/WKCsMUkLtxbz9bFyMUhJLCUUaK5 8y4zhPOMUaLt9y7WLkYODmEBS4k7J8JAGkQEciUmtJ1jA7GFBI4ySay66wpSzyywgVniyrJG dpAEm4CxxOblS9ggNshJ9HZPYgGxeQW0JGZM+8YKYrMIqEpMXr0ezBYViJCYtf0HE0SNoMSP yffA6jkFrCSuf25iA7mBWUBP4v5FLZAws4C8xOY1b5knMArOQtIxC6FqFpKqBYzMqxhFU0uT C4qT0nMN9YoTc4tL89L1kvNzNzFC4unLDsbFx6wOMQpwMCrx8C7IqQ0XYk0sK67MPcQowcGs JML7z6MuXIg3JbGyKrUoP76oNCe1+BCjNAeLkjjv3F3vQ4QE0hNLUrNTUwtSi2CyTBycUg2M c9S+JKxR4i49fyw3//X6qNN+PAfyPjku/2lwy9w4TrfgqbPUvK1XT1+LzuDw+j095aPy+fcT 94ceqlnOdDdpm4qenef9U5+b49iZWu9xnm86c1iw5opwlFTjLknXhc4KjDVvE9jWXuYyuW6x 7uWvkIlTJDWtLJiTdiY++Xb89otFFTPmXdweosRSnJFoqMVcVJwIAE+cyCujAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/07/2016 02:06 PM, Charles Keepax wrote: > On Tue, Jul 05, 2016 at 09:48:34AM -0400, Javier Martinez Canillas wrote: >> Hello Krzysztof, >> >> On 07/05/2016 02:33 AM, Krzysztof Kozlowski wrote: >>> On 07/04/2016 05:15 PM, Javier Martinez Canillas wrote: >> >> [snip] > > I have also been have a brief look at this as we have been > encountering issues attempting to move some of the clocking on > our audio CODECs to the clock framework. The problems are even > worse when the device can be controlled over SPI as well, as the > SPI framework may occasionally defer the transfer to a worker > thread rather than doing it in the same thread which causes the > re-enterant behaviour of the clock locking to no longer function. As you mentioned later, in such case per-controller-lock won't help. > >> >>>> >>>> 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. >>> >> > > I think there is still some milage in thinking about them just > to be sure, if we are going to the effort of changing the clock > framework locking it is worth getting it right as it will be > non-trivial. > > I could perhaps imagine a situation where one device is passing > a clock to second device and that device is doing some FLL/PLL > and passing the resulting clock back. For example supplying a > non-audio rate clock to a CODEC which then supplies back a clock > at an audio rate, which is used for audio functions on the first > device. What do you think by "passing" here? Pass the pointer to struct? > > Although that said I do think that by controller locking probably > fixes my primary issues right now. > >> Ok, if the configurations I described doesn't exist in practice and are >> just theoretical then yes, doing a per controller lock is a good 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... >>> >> >> I'm not against you re-working the locks to do it per controller, is just >> that I thought it would be good to have a solution that is going to work >> for all possible scenarios. >> >> You asked for comments/opinions/ideas and I gave mine, that's all :) >> > > I had also been leaning more towards a lock per clock rather > than a lock per controller. But one other issue that needs to be > kept in mind (with both the controller or clock based locking) > through is that the enable and prepare operations propagate down > the clock tree, where as the set rate operations propagate up the > clock tree. This makes things a rather furtile breeding ground > for mutex inversions as well. > Yeah, that is the problem we were thinking about just a sec ago. :) The set rate (and reparent which might cause set rate) is complicating the design. Idea I have is (simplifying only to prepare lock... leave away the enable): 1. Hava a global lock which will protect: a. traversing clock controller hierarchy, b. acquiring per clock controller locks, 2. Add struct for clock controller. 3. Add lock per clock controller. The basic locking in case of prepare for a simplified case one clock per clock controller: A (top controller = top clock) \-B \-C clk_prepare(C) { global_lock(); for (clk_ctrl = C) { lock(clk_ctrl); clk_ctrl = get_parent_controller(C); } global_unlock(); prepare_cnt++; // do the same for hierarchy for (clk_ctrl = C) { unlock(clk_ctrl) clock = get_parent_controller(C); } } The global lock is actually needed for the case of inverted walk during set_rate(). Best regards, Krzysztof