| 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
| ||
|
Date: Mon, 25 Aug 2008 17:57:32 -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 Monday 25 August 2008, Kevin Diggs wrote:
>
>>+ * cf750gx.c - cpufreq driver for the dual PLLs in the 750gx
>
>
> Thanks for posting this driver and for your attention for detail
> and for documentation in particular. Few people bother to write
> documentation at this level.
>
> I don't understand enough of cpufreq or your hardware to comment
> on that, but please let me give you a few hints on coding style.
>
>
>>+ * Copyright (C) 2008 kevin Diggs
>
>
> Most people list their email address here as well
>
For reasons I'd rather not go into, my email address is not likely
to remain valid for much longer.
>
>>+#define cf750gxmChangingPll (0x80000000)
>>+#define cf750gxmChangingPllBit (31)
>>+#define cf750gxmTurningIdlePllOff (0x40000000)
>>+#define cf750gxmTurningIdlePllOffBit (30)
>
>
> constants should be ALL_CAPS, not sIllYCaPS.
>
Are cf750gxm_CHANGING_PLL and cf750gxm_CHANGING_PLL_BIT_POS ok?
>
>>+struct pll_750fgx_t {
>>+ unsigned short min_ratio; /* min bus ratio */
>>+ unsigned short max_ratio; /* max bus ratio */
>>+ unsigned int min_core; /* min core frequency per spec (KHz) */
>>+ unsigned int max_core; /* max core frequency per spec (KHz) */
>>+};
>
>
> please drop the _t at the end of the identifier.
>
done
>
>>+MODULE_AUTHOR("Kevin Diggs");
>>+MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
>>+MODULE_LICENSE("GPL");
>
>
> Move this to the end.
>
done
>
>>+struct cf750gx_t_call_data {
>>+ struct cpufreq_freqs freqs;
>>+ unsigned long current_pll;
>>+ int idle_pll_off;
>>+};
>
>
> drop the _t here, or make explicit what is meant by it.
>
Now that I look at it the _t is supposed to be at the end. It is
meant to indicate that this is a structure tag or type. I'll
remove it.
>
>>+static const struct pll_750fgx_t __initdata pll_750fx = {
>>+ .min_ratio = 2,
>>+ .max_ratio = 20,
>>+ .min_core = 400000,
>>+ .max_core = 800000,
>>+};
>>+
>>+static const struct pll_750fgx_t __initdata pll_750gx = {
>>+ .min_ratio = 2,
>>+ .max_ratio = 20,
>>+ .min_core = 500000,
>>+ .max_core = 1000000,
>>+};
>
>
> Are these correct on any board? If they can be different
> depending on the board design, it would be better to get
> this data from the device tree.
>
They are a spceification of the processor itself. Should be
the same for any board using the 750GX (or FX).
>
>>+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
>>+
>>+static unsigned int override_min_core = 0;
>>+static unsigned int override_max_core = 0;
>>+static unsigned int minmaxmode = 0;
>>+
>>+static unsigned int cf750gx_v_min_core = 0;
>>+static unsigned int cf750gx_v_max_core = 0;
>>+static int cf750gx_v_idle_pll_off = 0;
>>+static int cf750gx_v_min_max_mode = 0;
>>+static unsigned long cf750gx_v_state_bits = 0;
>
>
> Is 0 a meaningful value for these? If it just means 'uninitialized',
> then better don't initialize them in the first place, for clarity.
>
The first 3 are module parameters. For the first 2, 0 means
that they were not set. minmaxmode is a boolean. 0 is the
default of disabled.
When I was initially writing the code I figured I would
need the min and max core frequencies in several places.
As it turns out they are only used in the code
initialization routine (cf750gx_init()). I have made
them locals.
..._idle_pll_off is a boolean for a sysfs attribute. 0 is
the default of disabled.
..._min_max_mode is a boolean to hold the state of
minmaxmode. Seems to be only used to print the current
value.
..._state_bits is a global to maintain state.
Does the PowerPC suffer any performance penalties when
accessing shorts compared to ints? Can I save a few bytes
by using shorts?
>
>>+static struct cpufreq_frequency_table *cf750gx_v_f_table;
>>+static struct cpufreq_frequency_table *cf750gx_v_freq_table;
>>+static struct cpufreq_frequency_table *cf750gx_v_min_max_freq_table;
>>+
>>+static struct cf750gx_t_call_data cf750gx_v_switch_call_data;
>>+static struct cf750gx_t_call_data cf750gx_v_lock_call_data;
>>+static struct notifier_block cf750gx_v_pll_switch_nb;
>>+static struct notifier_block cf750gx_v_pll_lock_nb;
>
>
> Also, in general, try to avoid global variables here, even
> in file scope (static), but rather put all device specific
> data into a per-device data structure.
>
How big of a problem is this? I regret the decision to rip
the SMP stuff out. But it is kinda done. If absolutely
necessary I can put these into a structure?
>
>>+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
>>+ action, void *data)
>>+{
>>+struct cf750gx_t_call_data *cd;
>>+unsigned int idle_pll;
>>+unsigned int pll_off_cmd;
>>+unsigned int new_pll;
>
>
> The whitespace appears damaged here.
>
Just a coding style thing. I put declarations (or definitions -
I get the two confused?) on the same indent as the block they are
in. Is this a 15 yard illegal procedure penalty?
>
>>+ cd = (struct cf750gx_t_call_data *)data;
>
done
>
> data is a void pointer, so you don't need the cast, and shouldn't
> have it therefore.
>
>>+static int cf750gx_pll_lock_cb(struct notifier_block *nb, unsigned long action,
>>+ void *data)
>>+{
>>+struct cf750gx_t_call_data *cd;
>>+
>>+ cd = (struct cf750gx_t_call_data *)data;
>
>
> same here.
>
and done
>
>>+static int cf750gx_target(struct cpufreq_policy *policy,
>>+ unsigned int target_freq, unsigned int relation)
>>+{
>>+unsigned int next_index = 0; /* Index into freq_table */
>>+unsigned int next_freq = 0; /* next frequency from perf table */
>>+unsigned int next_perf_state = 0; /* Index from perf table */
>>+int result = 0;
>
>
> Don't initialize local variables in the declaration, as that will prevent
> the compiler from warning about uninitialized use.
>
done
>
>>+unsigned int pll;
>>+unsigned int new_pll;
>>+unsigned int active_pll;
>>+struct cpufreq_freqs freqs;
>>+struct cpufreq_frequency_table *ft = cf750gx_v_f_table;
>
>
> more whitespace damage. Maybe there is something wrong with your
> text editor.
>
Nope, just a faulty programmer ...
>
>>+ dprintk(__FILE__">%s(, %u KHz, relation %u)-%d: on cpu %d\n",
>>+ __func__, target_freq, relation, __LINE__, policy->cpu);
>>+
>>+ if (test_and_set_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits))
>>+ return -EAGAIN;
>>+
>>+ INIT_COMPLETION(cf750gx_v_exit_completion);
>>+
>>+ result = cpufreq_frequency_table_target(policy,
>>+ ft,
>>+ target_freq,
>>+ relation, &next_index);
>>+
>>+ if (unlikely(result))
>>+ goto cf750gxTargetNoFreq;
>
>
> The unlikely() here looks like overoptimization, drop it in favor of
> readability unless you can measure a real-world difference.
>
This was stolen from the ACPI Processor P-States Driver. Given the
frequency of calls I would guess it does not make a difference.
>
>>+ if (active_pll) {
>>+ unsigned int current_state;
>
>
> whitespace damage.
>
same here ...
>
>>+ dprintk(__FILE__">%s()-%d: Modifying PLL: 0x%x\n", __func__, __LINE__,
>>+ new_pll);
>
>
> Please go through all your dprintk and see if you really really need all of them.
> Usually they are useful while you are doing the initial code, but only get in the
> way as soon as it starts working.
>
This from a code readability standpoint? Or an efficiency one?
I think the cpufreq stuff has a debug configure option that
disables compilation of these unless enabled.
>
>>+cf750gxTargetOut:
>>+ return result;
>>+
>>+cf750gxTargetNoFreq:
>>+ result = -ENODEV;
>>+
>>+ goto cf750gxTargetUnlock;
>>+cf750gxTargetFreqSet:
>>+ result = 0;
>>+
>>+ goto cf750gxTargetUnlock;
>>+cf750gxTargetUnlock:
>>+ clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
>>+ complete(&cf750gx_v_exit_completion);
>>+
>>+ goto cf750gxTargetOut;
>
>
> The conventional way to write this would be:
>
> result = -ENODEV;
> if (foo)
> goto out_unlock;
>
> result = 0;
> if (bar)
> goto out_unlock;
>
> return 0;
>
> out_unlock:
> clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
> complete(&cf750gx_v_exit_completion);
> out:
> return result;
>
I'll fix this.
>
>>+/* policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; */
>>+ policy->cpuinfo.transition_latency = pllif_get_latency();
>
>
> The comment does not really explain anything. If you just want to disable
> code, use #if 0, but better drop it right away and add a comment about
> what might need changing.
>
deleted.
>
>>+ result = cpufreq_frequency_table_cpuinfo(policy, cf750gx_v_f_table);
>>+ if (result)
>>+ goto err_freqfree;
>>+
>>+ cpufreq_frequency_table_get_attr(cf750gx_v_f_table, policy->cpu);
>>+
>>+ cf750gx_v_pll_switch_nb.notifier_call = cf750gx_pll_switch_cb;
>>+ cf750gx_v_pll_switch_nb.next = (struct notifier_block *)
>>+ &cf750gx_v_switch_call_data;
>>+ cf750gx_v_pll_switch_nb.priority = 0;
>>+
>>+ result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
>>+
>>+ cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
>>+ cf750gx_v_pll_lock_nb.next =
>>+ (struct notifier_block *)&cf750gx_v_lock_call_data;
>
>
> These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
> What are you trying to do here?
>
Just a little sneaky. I should document in the kernel doc though.
>
>>+ cf750gx_v_pll_lock_nb.priority = 0;
>>+
>>+ result = pllif_register_pll_lock_cb(&cf750gx_v_pll_lock_nb);
>>+
>>+ return result;
>>+
>>+err_freqfree:
>>+ return result;
>>+}
>
>
> The first 'return result' is redundant, drop it.
>
done.
>
>>+
>>+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
>>+{
>>+ dprintk("%s()\n", __func__);
>>+
>>+ /*
>>+ * Wait for any active requests to ripple through before exiting
>>+ */
>>+ wait_for_completion(&cf750gx_v_exit_completion);
>
>
> This "wait for anything" use of wait_for_completion looks wrong,
> because once any other function has done the 'complete', you won't
> wait here any more.
>
> What exactly are you trying to accomplish with this?
>
Originall I had something like:
while(some_bit_is_still_set)
sleep
I think you suggested I use a completion because it is in
fact simpler than a sleep. Now that I think about it also seems
intuitive to give the system a hint as to when something will
be done. 'complete' just means there is no timer pending (unless,
of course, I screwed up the code).
>
>>+static int __init cf750gx_init(void)
>>+{
>>+int ret;
>>+unsigned int freq, i, j, rng, bus_clock;
>>+unsigned short min_ratio, max_ratio;
>>+struct cpufreq_frequency_table *tbp;
>>+const struct pll_750fgx_t *pll_defaults;
>
>
> whitespace.
>
>
>>+ dprintk("%s()\n", __func__);
>>+
>>+ if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
>>+ return 0;
>
>
> Is this purely a feature of the CPU or does it need logic
> in the board design? If you need external hardware for it,
> you need to check the device tree for the presence of that
> hardware.
>
Purely a feature of the CPU. From what I know, voltage scaling
is an important part of reducing power comsumption. That would
be platform specific code. Anyone know if this is possible for
any 750GX based system? As far as I know it is not possible on
mine?
>
>>+ if (cf750gx_v_freq_table == NULL) {
>>+ ret = -ENOMEM;
>>+ goto ErrSimple;
>>+ }
>
>
> ret = -ENOMEM;
> if (!cf750gx_freq_table)
> goto err_simple;
>
done
>
> Arnd <><
>
>
--
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/
Powered by blists - more mailing lists