Arnd Bergmann wrote:
Please forgive me. I'm not trying to be argumentative. I'm just trying to learn. I found a section in the O'Reilly Linux device driver guide on this completion stuff. If I understand it correctly, I initialize a completion thing. In the code that starts the task I do a complete(). In the exit code I'll do a wait_for_completion(). In my usage paradigm I will VERY rarely ever call wait_for_completion() and have it actually wait. This still match completion's intended use?On Wednesday 13 August 2008, Kevin Diggs wrote:Arnd Bergmann wrote:On Wednesday 13 August 2008, Kevin Diggs wrote:In cpu exit function of a cpufreq driver: while (test_bit(cf750gxmChangingPllBit, &cf750gxvStateBits)) msleep(1); This bit will get cleared by a notifier callback. In module_exit function of a related module: while (test_bit(PLL_LOCK_BIT, (unsigned long *)&boot_ratio)) { msleep(1); }This bit will get cleared by a timer. It will also fire the notifiers needed above.I don't think these are in interrupt context. The sleeps ok here?Technically ok, but not nice. Besides the coding style issues, it's still a busy loop that should be replaced by wait_for_completion().I took a brief look at wait_for_completion(). Looks kinda heavy weight. Just to be clear both of these code fragments are in code shutdown (i.e. exit) paths. Is the use of wait_for_completion() still preferred? I thought a "busy loop" used the delay routines?You should always write code that is easy to understand and tells the reader what you mean. If you want to wait for something to complete, use wait_for_completion. If you look at what msleep does internally, you will find that it isn't simpler than wait_for_completion either. The loop doing msleep(1) is not as bad as a loop doing delays, but it can still cause unnecessary wakeups and on average will sleep one milisecond too long. Neither of these is a real problem, but if you can do it correctly, just do.
Can I post the 2 routines for RFC style comments? Or is that to much trouble?Can you elaborate on the coding style issues? Variable names? Use of the bit stuff? Those brace thingies?Variables should be lower-case names, constants should be upper-case. Both should have names that tell you what they are used for. The case in the second code sample is either wrong, or redundant. You should leave out curly braces when you only have a single line in the basic block. Read Documentation/CodingStyle to learn about more things to consider.
When you say initializing is frowned upon, do you mean only when you are initializing to zero? Is the redundancy (for the case of 0?) a part of the C spec? Or is it gcc specific? error-prone for automatic variables?Don't bother. Old gcc variants would put these variables into .data instead of .bss, but with a new (less than 5 years or so) gcc, both will result in .bss storage that is initialized to zero at boot time.??? So ... I can ignore the error? Or I should not be initializing variables to 0 (or NULL)?Either fix checkpatch.pl not to warn about these, or just don't initialize the variables. Initializing a variable at declaration time is frowned upon by some people, because it is redundant in case of static or global variables, and error-prone for automatic variables.
kevin
Arnd <><
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
