Re: [Patch 2/2] Special Memory (mspec) driver.

Dave Hansen
2005-10-14T05:12:05Z

On Wed, 2005-10-12 at 14:42 -0500, Robin Holt wrote:
> +static void
> +mspec_close(struct vm_area_struct *vma)
> +{
> + struct vma_data *vdata;
> + int i, pages, result;
> +
> + vdata = vma->vm_private_data;
> + if (atomic_dec_and_test(&vdata->refcnt)) {
> + pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
...

Looks like you could un-indent almost the entire function of you just
did this instead:

if (!atomic_dec_and_test(&vdata->refcnt))
return;

> +static __inline__ int
> +mspec_get_one_pte(struct mm_struct *mm, u64 address, pte_t ** page_table)
> +{
> + pgd_t *pgd;
> + pmd_t *pmd;
> + pud_t *pud;
> +
> + pgd = pgd_offset(mm, address);
> + if (pgd_present(*pgd)) {
> + pud = pud_offset(pgd, address);
> + if (pud_present(*pud)) {
> + pmd = pmd_offset(pud, address);
> + if (pmd_present(*pmd)) {
> + *page_table = pte_offset_map(pmd, address);
> + if (pte_present(**page_table)) {
> + return 0;
> + }
> + }
> + }
> + }
> +
> + return -1;
> +}

This looks pretty similar to get_one_pte_map(). Is there enough
commonality to use it?

> +static int
> +mspec_mmap(struct file *file, struct vm_area_struct *vma, int type)
> +{
> + struct vma_data *vdata;
> + int pages;
> +
> + if (vma->vm_pgoff != 0)
> + return -EINVAL;
> +
> + if ((vma->vm_flags & VM_WRITE) = 0)
> + return -EPERM;
> +
> + pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> + if (!
> + (vdata > + vmalloc(sizeof(struct vma_data) + pages * sizeof(long))))
> + return -ENOMEM;

How about:

vdata = vmalloc(sizeof(struct vma_data) + pages * sizeof(long));
if (!vdata)
return -ENOMEM;

> +#ifdef CONFIG_PROC_FS
> +static void *
> +mspec_seq_start(struct seq_file *file, loff_t * offset)
> +{
> + if (*offset < MAX_NUMNODES)
> + return offset;
> + return NULL;
> +}

This whole thing really is a driver for a piece of arch-specific
hardware, right? Does it really belong in /proc? You already have a
misc device, so you already have some area in sysfs. Would that make a
better place for it?

> +static int __init
> +mspec_init(void)
> +{
> + if ((ret = misc_register(&cached_miscdev))) {
> + printk(KERN_ERR "%s: failed to register device %i\n",
> + CACHED_ID, ret);
> + misc_deregister(&fetchop_miscdev);
> + return ret;
> + }

Isn't the general kernel style for these to keep the action out of the
if() condition?

ret = misc_register(&cached_miscdev);
if (ret) {
...
}

-- Dave