Message-ID: <48B6D382.6010204@hypersurf.com>
Date: Thu, 28 Aug 2008 09:34:11 -0700
From: Kevin Diggs <kevdig@hypersurf.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i486; en-US; rv:1.8b2) Gecko/20050724
MIME-Version: 1.0
To: Arnd Bergmann <arnd@arndb.de>
CC: linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
References: <48B28F23.1000906@hypersurf.com> <200808271334.16712.arnd@arndb.de> <48B5C0AD.9040006@hypersurf.com> <200808280946.10077.arnd@arndb.de>
In-Reply-To: <200808280946.10077.arnd@arndb.de>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3071
Lines: 68

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/