Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX

[Kevin Diggs <kevdig@hypersurf.com>]

Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
> 
>>Arnd Bergmann wrote:
>>
>>>I think the module_exit() function should leave the frequency in a
>>>well-defined state, so the easiest way to get there is probably
>>>to delete the timer, and then manually set the frequency.
>>>
>>
>>I really don't follow you here? If I let the timer fire then the cpu
>>(and the cpufreq sub-system) will be left in a well-defined state. I
>>don't understand why you want me to delete the timer and then
>>basically do manually what it was going to do anyway. There are two
>>calls to cpufreq_notify_transition(). One just before the modify_PLL()
>>call, with CPUFREQ_PRECHANGE as an argument, and one in the
>>pll_switch_cb() routine, with CPUFREQ_POSTCHANGE as an argument. I
>>would need to make sure that these are matched up.
>>
>>Even without the HRTimer stuff being used the timer fires in less than
>>4 ms (@ 250 HZ). So I can't see the user actually trying to interrupt
>>a frequency change. With HRTimers it is 100 us.
>>
>>Can we please, please leave this part as is?
> 
> 
> I'm still not convinced that it's actually correct if you call complete()
> from the other places as well. You have three locations in your code where
> you call complete() but only one for INIT_COMPLETION. The part that I don't
> understand (and therefore don't expect other readers to understand) is how
> the driver guarantees that only one complete() will be called on the
> completion variable after it has been initialized.
> 
> What I meant with the well-defined state is that after unloading the module,
> the CPU frequency should be the same as before loading the module, typically
> the maximum frequency, but not the one that was set last.
> 
As you point out, there are three calls to complete().

The first is in the switch callback, in the idle_pll_off disabled branch.
This callback is triggered from the pll switch routine in pll_if. So that
means the call to _modify_PLL() in _target worked. So the complete() call
in _target will NOT be called. The complete() call in the lock callback
is only called in the idle_pll_off enabled branch.

As just mentioned, the second complete() call in the lock callback is
only called when idle_pll_off is enabled.

The final complete() call is in the unlock exit path in _target(). This
is an error path, where for one reason or another, there was no successful
call to _modify_PLL(). Thus there will be triggering of either callback.

There is another initialization of the completion:  the DECLARE_COMPLETION().
I think I will deadlock if I get unloaded before _target() is ever called.
This can happen. I may add a test_bit(...changing_pll_bit) condition on the
wait_for_completion() call.
> 
> 	Arnd <><
> 
Thanks for taking the time to review and for the comments!

kevin