Messages in this thread | | | Subject | Re: [PATCH 3/6] 2.6.16-rc1 perfmon2 patch for review | From | Bryan O'Sullivan <> | Date | Fri, 20 Jan 2006 08:47:24 -0800 |
| |
On Fri, 2006-01-20 at 07:20 -0800, Stephane Eranian wrote:
> +static struct pfm_smpl_fmt dfl_fmt={ > + .fmt_name = "default_format2", > + .fmt_uuid = PFM_DFL_SMPL_UUID,
What's the point of using a UUID here?
> +static struct file_system_type pfm_fs_type = { > + .name = "pfmfs", > + .get_sb = pfmfs_get_sb, > + .kill_sb = kill_anon_super, > +};
A comment that describes what pfmfs is for would be useful here, and perhaps a warning to hold one's nose, if the code that follows is anything to go by :-)
> +#if 0 > +irqreturn_t pfm_interrupt_handler(int irq, void *arg, struct pt_regs *regs)
Why a dead interrupt handler here?
> +static ctl_table pfm_ctl_table[]={
Why are you using sysctls, and not sysfs? Why is this in a file that claims to be procfs-related?
Also, it looks like much of the procfs goo is actually not related to individual processes, really doesn't belong in /proc at all, and should move to some place in sysfs somewhere.
> +/* > + * invoked by writing to /proc/sys/kernel/perfmon/reset_stats > + */
Yep, this shouldn't be in /proc, unless I'm massively misunderstanding the current state of the world.
> +int pfm_get_smpl_arg(pfm_uuid_t uuid, void *uaddr, size_t usize, void **arg, > + struct pfm_smpl_fmt **fmt)
That should be void __user *uaddr. Please run the code through sparse.
> + if (addr) > + kfree(addr);
kfree ignores a NULL argument.
<b
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |