Re: [git-pull -tip V2] x86: cpu architecture debug code

Ingo Molnar
2009-03-10T12:28:27Z

* Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:

> Added more features, now it supports:
> 1. TSS (GPR, Segment, Eflags)
> 2. Control Regs
> 3. DT (IDT, GDT, LDT, TR)
> 4. Debug regs
> 5. LAPIC
> 6. MSRs

looks pretty good!

A few small details:

> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/smp.h>
> +#include <linux/regset.h>
> +#include <linux/sched.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <asm/desc.h>
> +#include <asm/cpu_debug.h>

Please use the include files style as can be seen in
arch/x86/mm/fault.c. The reason why we do it is to reduce
conflicts when files are modified by multiple topic branches at
once.

> + vendor = per_cpu(cpu_model, cpu) >> 16;
> + modelflag = per_cpu(cpu_modelflag, cpu);
> + index = get_cpu_range_count(cpu);
> + for (i = 0; i < index; i++) {

please put a newline before loops in such cases, to make it
stand out some more.

> +/* This function can also be called with seq = NULL for printk */
> +static void print_msr(struct seq_file *seq, unsigned cpu, unsigned flag)
> +{
> + int i, range;
> + u32 low, high;
> + unsigned msr, msr_min, msr_max;
> + struct cpu_private *priv;

please try to order local variables like this:

> + unsigned msr, msr_min, msr_max;
> + struct cpu_private *priv;
> + u32 low, high;
> + int i, range;

(this is done for similar reasons as the include files section
ordering)

this affects other functions in the file too.

> +static const struct seq_operations cpu_seq_ops = {
> + .start = cpu_seq_start,
> + .next = cpu_seq_next,
> + .stop = cpu_seq_stop,
> + .show = cpu_seq_show,
> +};

Please use consistent vertical alignment wherever possible
thoughout the file, i.e.:

> + .start = cpu_seq_start,
> + .next = cpu_seq_next,
> + .stop = cpu_seq_stop,
> + .show = cpu_seq_show,

(note this applies to other places too in this same file.)

> +static int cpu_seq_open(struct inode *inode, struct file *file)
> +{
> + int err;
> + struct seq_file *seq;
> + struct cpu_private *priv = inode->i_private;
> +
> + err = seq_open(file, &cpu_seq_ops);
> + mutex_lock(&cpu_debug_lock);
> + if (!err) {
> + seq = file->private_data;
> + seq->private = priv;
> + }
> + mutex_unlock(&cpu_debug_lock);

what is the purpose of the locking here? What other codepath
can race with this?

> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (priv == NULL)
> + return -ENOMEM;
> +
> + mutex_lock(&cpu_debug_lock);
> + priv->cpu = cpu;
> + priv->type = type;
> + priv->reg = reg;
> + priv->file = file;
> + per_cpu(priv_arr[type], cpu) = priv;
> + per_cpu(cpu_priv_count, cpu)++;
> + mutex_unlock(&cpu_debug_lock);

what's the purpose of the locking here and why does it cover
more than just the per_cpu() related critical section?

Ingo