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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <48B6D382.6010204@hypersurf.com>
Date:	Thu, 28 Aug 2008 09:34:11 -0700
From:	Kevin Diggs <kevdig@...ersurf.com>
To:	Arnd Bergmann <arnd@...db.de>
CC:	linuxppc-dev@...abs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX

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