Re: [PATCH] Power5,Power6 BSR driver

[Nathan Lynch <ntl@pobox.com>]

Hi, mainly a couple of coding style things, but one minor bug (I
think).

jschopp@austin.ibm.com wrote:
> From: Sonny Rao <sonnyrao@linux.vnet.ibm.com>
> 
> +static int bsr_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	unsigned long size   = vma->vm_end - vma->vm_start;
> +	struct bsr_dev *dev = filp->private_data;
> +
> +	if (size > dev->bsr_len || (size & (PAGE_SIZE-1)))
> +		return -EINVAL;
> +
> +	vma->vm_flags |= (VM_IO | VM_DONTEXPAND);
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +
> +	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
> +			       size, vma->vm_page_prot))
> +	    return -EAGAIN;

Indentation is wrong.


> +static void bsr_cleanup_devs(void)
> +{
> +	int i;
> +	for (i=0 ; i < num_bsr_devs; i++) {

             i = 0

> +		struct bsr_dev *cur = bsr_devs + i;
> +		if (cur->bsr_device) {
> +			cdev_del(&cur->bsr_cdev);
> +			device_del(cur->bsr_device);
> +		}
> +	}
> +
> +	kfree(bsr_devs);
> +}
> +
> +static int bsr_create_devs(struct device_node *bn)
> +{
> +	int reg_len, bsr_stride_len, bsr_bytes_len;
> +	const u64 *reg;
> +	const u32 *bsr_stride;
> +	const u32 *bsr_bytes;
> +	unsigned i;
> +
> +	reg        = of_get_property(bn, "reg", &reg_len);
> +	bsr_stride = of_get_property(bn, "ibm,lock-stride", &bsr_stride_len);
> +	bsr_bytes  = of_get_property(bn, "ibm,#lock-bytes", &bsr_bytes_len);
> +
> +	if (!reg || !bsr_stride || !bsr_bytes ||
> +	    (bsr_stride_len != bsr_bytes_len) ||
> +	    (bsr_stride_len/4 != reg_len/16)) {

             bsr_stride_len / 4 != reg_len / 16


> +		printk(KERN_ERR "bsr of-node has missing/incorrect property\n");
> +		return -ENODEV;
> +	}

...

> +static int __init bsr_init(void)
> +{
> +	struct device_node *np;
> +	dev_t bsr_dev = MKDEV(bsr_major, 0);
> +	int ret = -ENODEV;
> +	int result;
> +
> +	np = of_find_compatible_node(NULL, "ibm,bsr", "ibm,bsr");
> +	if (!np)
> +		goto out_err;
> +
> +	bsr_class = class_create(THIS_MODULE, "bsr");
> +	if (IS_ERR(bsr_class)) {
> +		printk(KERN_ERR "class_create() failed for bsr_class\n");
> +		goto out_err;

At this point I think you can leak a reference to np.


> +	}
> +	bsr_class->dev_attrs = bsr_dev_attrs;
> +
> +	result = alloc_chrdev_region(&bsr_dev, 0, BSR_MAX_DEVS, "bsr");
> +	bsr_major = MAJOR(bsr_dev);
> +	if (result < 0) {
> +		printk(KERN_ERR "alloc_chrdev_region() failed for bsr\n");
> +		goto out_err_1;
> +	}
> +
> +	if ((ret = bsr_create_devs(np)) < 0)
> +		goto out_err_2;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +
> + out_err_2:
> +	unregister_chrdev_region(bsr_dev, BSR_MAX_DEVS);
> +
> + out_err_1:
> +	class_destroy(bsr_class);
> +	of_node_put(np);
> +
> + out_err:
> +
> +	return ret;
> +}