[Cbe-oss-dev] [PATCH] axonram: 1st version

[Christoph Hellwig]

On Fri, Feb 16, 2007 at 09:33:48AM +0100, Maxim Shchetynin wrote:
> Hello,
> 
> Here is a couple of lines shorter version of axonram driver. I have removed
> all the chrdev stuff and changed the rest of code according your
> recommendations.

We, that's a nice small driver :)

> +     bank->device = device;
> +     bank->size = resource.end - resource.start + 1;
> +     bank->ph_addr = resource.start;
> +     bank->io_addr = (unsigned long)
> +                 __iomem ioremap_flags(bank->ph_addr, bank->size, 0);

That __iomem handling still doesn't seem to be right.  io_addr
shuld be a variable of type void __iomem *. And we shouldn't need
that pseudo-casting.

> +     if (of_address_to_resource(device->node, 0, &resource) != 0) {
> +           dev_err(&device->dev, "Cannot access device tree\n");
> +           kfree(bank);
> +           return -EFAULT;
> +     }

> +
> +     bank->disk = alloc_disk(AXON_RAM_MINORS_PER_DISK);
> +     if (bank->disk == NULL) {
> +           dev_err(&device->dev, "Cannot register disk\n");
> +           kfree(bank);
> +           return -EFAULT;
> +     }

> +     bank->disk->major = register_blkdev(0, bank->disk->disk_name);
> +     if (bank->disk->major < 0) {
> +           dev_err(&device->dev, "Cannot register block device\n");
> +           del_gendisk(bank->disk);
> +           kfree(bank);
> +           return -EFAULT;

I think error handling could be improved a lot by using the
"traditional" kerne lgoto-style  error handling

> +     bank->disk->first_minor = 0;
> +     bank->disk->fops = &axon_ram_devops;
> +     bank->disk->queue = blk_alloc_queue(GFP_KERNEL);

We probably need to handle a memory allocation failure here.

> +/**
> + * axon_ram_remove - remove() method for platform driver
> + * @device: see of_platform_driver method
> + */
> +static int
> +axon_ram_remove(struct of_device *device)
> +{
> +     struct axon_ram_bank *bank = device->dev.platform_data;
> +     int rc;
> +
> +     BUG_ON(!bank || !bank->disk);
> +
> +     rc = unregister_blkdev(bank->disk->major, bank->disk->disk_name);
> +     del_gendisk(bank->disk);
> +
> +     if (rc == 0)
> +           kfree(bank);

We need to kfree here even in case unregister_blkdev returns an error,
else we leak memory.

> +
> +     return rc;
> +}

speaking of leaks you probably need a put_disk at the end of the
function to remove your last reference on the gendisk.