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

From: Kevin Diggs
Date: Wed Aug 27 2008 - 17:03:21 EST


Arnd Bergmann wrote:
On Wednesday 27 August 2008, Kevin Diggs wrote:

Arnd Bergmann wrote:


Ok, thanks for the explanation. I now saw that you also
use '_v' for variables (I guess). These should probably
go the same way.


Actually the _v means global variable. I would prefer to keep it.


The reasoning on Linux is that you can tell from the declaration
whether something is global or automatic. In fact, functions should
be so short that you can always see all automatic declarations
when you look at some code.

If you use nonstandard variable naming, people will never stop
asking you about that, so it's easier to just to the same as
everyone else.

I will remove the "_v".

Then at least for the first two, the common coding style would
be to leave out the '= 0'. For minmaxmode, the most expressive
way would be to define an enum, like

enum {
MODE_NORMAL,
MODE_MINMAX,
};

int minmaxmode = MODE_NORMAL;


Since this is a boolean parameter I don't know? What if I change the
name to enable_minmax. And actually use the "bool" module parameter
type?


Module parameter names should be short, so just "minmax" would
be a good name, but better put the module_param() line right
after that. If it's a bool type, I would just leave out the
initialization.

Ok. But leaving out the initialization will make me itch. Should I
also replace "override_min_core" with "mincore" (or "min_core")? And
"override_max_core" with "maxcore" (or "max_core")?

Leaving out the initializations makes me ... uneasy. It's ok to leave
them out if they are 0?

..._min_max_mode is a boolean to hold the state of
minmaxmode. Seems to be only used to print the current
value.


this looks like a duplicate then. why would you need both?
It seems really confusing to have both a cpufreq attribute
and a module attribute with the same name, writing to
different variables.


I probably don't need both? I kinda treat the variables tied to module
parameters as read only.


But you have marked as read/write in the module_param line.

I would prefer to just have the module parameter and have
a tool to modify that one.

If a module parameter only makes sense as read-only, you
should mark it as 0444 instead of 0644, but this one looks
like it can be writable.

I meant I treat them as read only from the code. That is why I have a
separate variable to change from the sysfs routines. I'll eliminate it
if you like. I have removed the auto added sysfs attributes.

The completion would certainly be better than the sleep here, but
I think you shouldn't need that in the first place. AFAICT, you
have three different kinds of events that you need to protect
against, when some other code can call into your module:

1. timer function,
2. cpufreq notifier, and
3. sysfs attribute.

In each case, the problem is a race between two threads that you
need to close. In case of the timer, you need to call del_timer_sync
because the timers already have this method builtin. For the other
two, you already unregister the interfaces, which ought to be enough.


The choice I made here was to wait for the timer to fire rather than
to delete it. I think it is easier to just wait for it than to delete
it and try to get things back in order. Though since this is in a
module exit routine it probably does not matter. Also I would have to
check whether there was an analogous HRTimer call and call the right
one.


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?

Arnd <><

--
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/