Re: [PATCH] Power5,Power6 BSR driver

[Sonny Rao <sonnyrao@us.ibm.com>]

On Tue, Jun 17, 2008 at 05:39:52PM -0500, Nathan Lynch wrote:
> 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.

Yeah I noticed that too.

> > +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.

Yeah, you're right.

> 
> > +	}
> > +	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;
> > +}

Ok Will fix and send out again

-- 
Sonny Rao, LTC Ozlabs