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

[Kevin Diggs]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/