Re: [PATCH 5/5] ARM: OMAP: remove unnecessary locking in clk_get_rate()

[Kevin Hilman <khilman@mvista.com>]

Tony Lindgren wrote:
> * Kevin Hilman <khilman@mvista.com> [080604 09:37]:
>> Paul Walmsley wrote:
>>> Hi Kevin,
>>>
>>> On Tue, 20 May 2008, Kevin Hilman wrote:
>>>
>>>  > The locking in the get_rate() hook is unnecessary, and causes problems
>>>  > when used with the -rt patch, since it may be called recursively.
>>>  >
>>>  > Signed-off-by: Kevin Hilman <khilman@mvista.com>
>>>
>>> Acked-by: Paul Walmsley <paul@pwsan.com>
>>>
>>> BTW, looks like we could probably get rid of some of the other spinlocking
>>> in plat-omap/clock.c also -- clk_get_usecount() ?
>> I agree, most of the locking in there seems unnecessary, but this was  
>> the only one that could be removed without looking closer at at all the  
>> platform specific clock code.
>>
>> Basically, I think there minimal (maybe no) locking in the generic  
>> layer, and the locking should be done if necessary in the platform  
>> specific parts.
> 
> To me it seems locking is needed. What if something is changing a clock's
> rate and then something else calls get_rate() while the other thread is
> changing the rate?

Then the caller of get_rate() has an out of date value.

The same can happen even with the locking.  If someone asks for the 
rate, and then another thread changes the rate right after, the thread 
who asked for the rate has an out-of-date value.

That being said, there may be some locks that need to stay, I haven't 
done a thorough analysis of this.  I just think that there is currently 
to much unnecessary locking.

> If get_rate() is not locking, get_rate() may get invalid rate between
> something else programming the hardware and changing clk->rate. I guess
> just locking set_rate() would still work with local irq disabled,
> but would not be SMP safe.
> 
> Anyways, ideally the spin_lock would be specific to the clocks accessing
> the same clock registers... But that would mean adding yet another field
> to struct clk or clock domains. Anybody got better ideas?
> 
> And of course anything going through the clocks list needs to be protected
> by the mutex.

Agreed, the mutex needs to stay.

Kevin