details, details ...

details, details ...

[Kevin Diggs]

Hi,

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?

Also, from checkpatch:

ERROR: do not initialise externals to 0 or NULL
#2483: FILE: arch/powerpc/kernel/cpu/pll_if.c:486:
+int rval = 0;

ERROR: do not initialise statics to 0 or NULL
#2058: FILE: arch/powerpc/kernel/cpu/pll_if.c:61:
+static unsigned int override_bus_clock = 0;

Huh? What? Anyone care to teach an old dog a new trick???

kevin

Re: details, details ...

[Arnd Bergmann]

On Wednesday 13 August 2008, Kevin Diggs wrote:
> In cpu exit function of a cpufreq driver:
>=20
> =A0 =A0 =A0 =A0 =A0while (test_bit(cf750gxmChangingPllBit, &cf750gxvState=
Bits))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msleep(1);
>=20
> This bit will get cleared by a notifier callback.
>=20
> In module_exit function of a related module:
>=20
> =A0 =A0 =A0 =A0 =A0while (test_bit(PLL_LOCK_BIT, (unsigned long *)&boot_r=
atio)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msleep(1);
> =A0 =A0 =A0 =A0 =A0}
>=20
> This bit will get cleared by a timer. It will also fire the notifiers=20
> needed above.
>=20
> 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().

> Also, from checkpatch:
>=20
> ERROR: do not initialise externals to 0 or NULL
> #2483: FILE: arch/powerpc/kernel/cpu/pll_if.c:486:
> +int rval =3D 0;
>=20
> ERROR: do not initialise statics to 0 or NULL
> #2058: FILE: arch/powerpc/kernel/cpu/pll_if.c:61:
> +static unsigned int override_bus_clock =3D 0;
>=20
> Huh? What? Anyone care to teach an old dog a new trick???

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.

	Arnd <><

Re: details, details ...

[Kevin Diggs]

Arnd Bergmann wrote:
> 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.
> 
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?
> 
>>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.
> 
Can I post the 2 routines for RFC style comments? Or is that to much 
trouble?
> 
>>>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.
> 
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?

kevin
> 	Arnd <><
> 
>