[PATCH 1/8] block/genhd.c: Add error handling

[PATCH 1/8] block/genhd.c: Add error handling

[Vishnu Pratap Singh]

This patch adds error handling for blk_register_region(), register_disk(),
add_disk(), disk_alloc_events() & disk_add_events().
add_disk() & register_disk() functions  error handling is very much needed
as this is used mainly by many modules like mmc, zram, mtd, scsi etc.
But there is no error handling and it may cause stability issues also.

Verfied on X86 based ubuntu machine.

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 block/genhd.c         | 89 +++++++++++++++++++++++++++++++++++----------------
 include/linux/genhd.h |  4 +--
 2 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 3213b66..a63ebd6 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -39,8 +39,8 @@ static struct device_type disk_type;
 
 static void disk_check_events(struct disk_events *ev,
 			      unsigned int *clearing_ptr);
-static void disk_alloc_events(struct gendisk *disk);
-static void disk_add_events(struct gendisk *disk);
+static int disk_alloc_events(struct gendisk *disk);
+static int disk_add_events(struct gendisk *disk);
 static void disk_del_events(struct gendisk *disk);
 static void disk_release_events(struct gendisk *disk);
 
@@ -473,11 +473,11 @@ static char *bdevt_str(dev_t devt, char *buf)
  * range must be nonzero
  * The hash chain is sorted on range, so that subranges can override.
  */
-void blk_register_region(dev_t devt, unsigned long range, struct module *module,
+int blk_register_region(dev_t devt, unsigned long range, struct module *module,
 			 struct kobject *(*probe)(dev_t, int *, void *),
 			 int (*lock)(dev_t, void *), void *data)
 {
-	kobj_map(bdev_map, devt, range, module, probe, lock, data);
+	return kobj_map(bdev_map, devt, range, module, probe, lock, data);
 }
 
 EXPORT_SYMBOL(blk_register_region);
@@ -505,7 +505,7 @@ static int exact_lock(dev_t devt, void *data)
 	return 0;
 }
 
-static void register_disk(struct gendisk *disk)
+static int register_disk(struct gendisk *disk)
 {
 	struct device *ddev = disk_to_dev(disk);
 	struct block_device *bdev;
@@ -520,14 +520,15 @@ static void register_disk(struct gendisk *disk)
 	/* delay uevents, until we scanned partition table */
 	dev_set_uevent_suppress(ddev, 1);
 
-	if (device_add(ddev))
-		return;
+	err = device_add(ddev);
+	if (err)
+		return err;
 	if (!sysfs_deprecated) {
 		err = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));
 		if (err) {
 			device_del(ddev);
-			return;
+			return err;
 		}
 	}
 
@@ -569,6 +570,7 @@ exit:
 	while ((part = disk_part_iter_next(&piter)))
 		kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
 	disk_part_iter_exit(&piter);
+	return err;
 }
 
 /**
@@ -577,10 +579,8 @@ exit:
  *
  * This function registers the partitioning information in @disk
  * with the kernel.
- *
- * FIXME: error handling
  */
-void add_disk(struct gendisk *disk)
+int add_disk(struct gendisk *disk)
 {
 	struct backing_dev_info *bdi;
 	dev_t devt;
@@ -598,7 +598,7 @@ void add_disk(struct gendisk *disk)
 	retval = blk_alloc_devt(&disk->part0, &devt);
 	if (retval) {
 		WARN_ON(1);
-		return;
+		return retval;
 	}
 	disk_to_dev(disk)->devt = devt;
 
@@ -608,17 +608,27 @@ void add_disk(struct gendisk *disk)
 	disk->major = MAJOR(devt);
 	disk->first_minor = MINOR(devt);
 
-	disk_alloc_events(disk);
-
+	retval = disk_alloc_events(disk);
+	if (retval)
+		goto err_blk_devt;
 	/* Register BDI before referencing it from bdev */
 	bdi = &disk->queue->backing_dev_info;
-	bdi_register_dev(bdi, disk_devt(disk));
+	retval = bdi_register_dev(bdi, disk_devt(disk));
+	if (retval)
+		goto err_alloc_event;
 
-	blk_register_region(disk_devt(disk), disk->minors, NULL,
+	retval = blk_register_region(disk_devt(disk), disk->minors, NULL,
 			    exact_match, exact_lock, disk);
-	register_disk(disk);
-	blk_register_queue(disk);
+	if (retval)
+		goto err_alloc_event;
+
+	retval = register_disk(disk);
+	if (retval)
+		goto err_reg_region;
 
+	retval = blk_register_queue(disk);
+	if (retval)
+		goto err_reg_disk;
 	/*
 	 * Take an extra ref on queue which will be put on disk_release()
 	 * so that it sticks around as long as @disk is there.
@@ -627,9 +637,28 @@ void add_disk(struct gendisk *disk)
 
 	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
 				   "bdi");
-	WARN_ON(retval);
+	if (retval)
+		goto err_blk_reg_queue;
+
+	retval = disk_add_events(disk);
+	if (retval)
+		goto err_sysfs;
 
-	disk_add_events(disk);
+	return 0;
+
+err_blk_devt:
+	blk_free_devt(devt);
+err_alloc_event:
+	disk_del_events(disk);
+err_reg_region:
+	blk_unregister_region(disk_devt(disk), disk->minors);
+err_reg_disk:
+	del_gendisk(disk);
+err_blk_reg_queue:
+	blk_unregister_queue(disk);
+err_sysfs:
+	sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+	return retval;
 }
 EXPORT_SYMBOL(add_disk);
 
@@ -1782,17 +1811,17 @@ module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
 /*
  * disk_{alloc|add|del|release}_events - initialize and destroy disk_events.
  */
-static void disk_alloc_events(struct gendisk *disk)
+static int disk_alloc_events(struct gendisk *disk)
 {
 	struct disk_events *ev;
 
 	if (!disk->fops->check_events)
-		return;
+		return 0;
 
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
 	if (!ev) {
 		pr_warn("%s: failed to initialize events\n", disk->disk_name);
-		return;
+		return -ENOMEM;
 	}
 
 	INIT_LIST_HEAD(&ev->node);
@@ -1804,17 +1833,22 @@ static void disk_alloc_events(struct gendisk *disk)
 	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
 
 	disk->ev = ev;
+	return 0;
 }
 
-static void disk_add_events(struct gendisk *disk)
+static int disk_add_events(struct gendisk *disk)
 {
+	int ret = 0;
+
 	if (!disk->ev)
-		return;
+		return ret;
 
-	/* FIXME: error handling */
-	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
+	ret = sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
+	if (ret) {
 		pr_warn("%s: failed to create sysfs files for events\n",
 			disk->disk_name);
+		return ret;
+	}
 
 	mutex_lock(&disk_events_mutex);
 	list_add_tail(&disk->ev->node, &disk_events);
@@ -1825,6 +1859,7 @@ static void disk_add_events(struct gendisk *disk)
 	 * unblock kicks it into action.
 	 */
 	__disk_unblock_events(disk, true);
+	return ret;
 }
 
 static void disk_del_events(struct gendisk *disk)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 2adbfa6..dae3160 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -417,7 +417,7 @@ static inline void free_part_info(struct hd_struct *part)
 extern void part_round_stats(int cpu, struct hd_struct *part);
 
 /* block/genhd.c */
-extern void add_disk(struct gendisk *disk);
+extern int add_disk(struct gendisk *disk);
 extern void del_gendisk(struct gendisk *gp);
 extern struct gendisk *get_gendisk(dev_t dev, int *partno);
 extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
@@ -620,7 +620,7 @@ extern struct gendisk *alloc_disk_node(int minors, int node_id);
 extern struct gendisk *alloc_disk(int minors);
 extern struct kobject *get_disk(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
-extern void blk_register_region(dev_t devt, unsigned long range,
+extern int blk_register_region(dev_t devt, unsigned long range,
 			struct module *module,
 			struct kobject *(*probe)(dev_t, int *, void *),
 			int (*lock)(dev_t, void *),
-- 
1.9.1

[PATCH 2/8] mmc: handle add_disk() return value

[Vishnu Pratap Singh]

This patch handles  add_disk() return value.
Earlier add_disk() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/mmc/card/block.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 23b6c8e..543c670 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2432,7 +2432,10 @@ static int mmc_add_disk(struct mmc_blk_data *md)
 	int ret;
 	struct mmc_card *card = md->queue.card;
 
-	add_disk(md->disk);
+	ret = add_disk(md->disk);
+	if (ret)
+		goto add_disk_fail;
+
 	md->force_ro.show = force_ro_show;
 	md->force_ro.store = force_ro_store;
 	sysfs_attr_init(&md->force_ro.attr);
@@ -2468,7 +2471,7 @@ power_ro_lock_fail:
 	device_remove_file(disk_to_dev(md->disk), &md->force_ro);
 force_ro_fail:
 	del_gendisk(md->disk);
-
+add_disk_fail:
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 2/8] mmc: handle add_disk() return value

[Ulf Hansson]

On 6 November 2015 at 13:22, Vishnu Pratap Singh <vishnu.ps@samsung.com> wrote:
> This patch handles  add_disk() return value.
> Earlier add_disk() function doesn't handle error
> cases, now it is added, so the callers of this function
> should also handle it.
>
> Verfied on X86 based ubuntu machine.
> This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Please remove these two lines from the change log.

Still it's great that you provide this information while posting the
patches, but you can do that by adding text between the sign-off-by
and "---". Just add a new line consisting of "---" and then add you
text below it.

>
> Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
> ---
>  drivers/mmc/card/block.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 23b6c8e..543c670 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2432,7 +2432,10 @@ static int mmc_add_disk(struct mmc_blk_data *md)
>         int ret;
>         struct mmc_card *card = md->queue.card;
>
> -       add_disk(md->disk);
> +       ret = add_disk(md->disk);
> +       if (ret)
> +               goto add_disk_fail;

You don't need a goto here. Please just do "return ret;" as I think it
makes code easier.

> +
>         md->force_ro.show = force_ro_show;
>         md->force_ro.store = force_ro_store;
>         sysfs_attr_init(&md->force_ro.attr);
> @@ -2468,7 +2471,7 @@ power_ro_lock_fail:
>         device_remove_file(disk_to_dev(md->disk), &md->force_ro);
>  force_ro_fail:
>         del_gendisk(md->disk);
> -
> +add_disk_fail:
>         return ret;
>  }
>
> --
> 1.9.1
>

Kind regards
Uffe

[PATCH 2/8] mmc: handle add_disk() return value

[Vishnu Pratap Singh]

This patch handles  add_disk() return value.
Earlier add_disk() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
--- Verfied on X86 based ubuntu machine.
--- This patch depends on [PATCH 1/8] block/genhd.c: Add error handling
---
 drivers/mmc/card/block.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 23b6c8e..543c670 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2432,7 +2432,10 @@ static int mmc_add_disk(struct mmc_blk_data *md)
 	int ret;
 	struct mmc_card *card = md->queue.card;
 
-	add_disk(md->disk);
+	ret = add_disk(md->disk);
+	if (ret)
+		goto add_disk_fail;
+
 	md->force_ro.show = force_ro_show;
 	md->force_ro.store = force_ro_store;
 	sysfs_attr_init(&md->force_ro.attr);
@@ -2468,7 +2471,7 @@ power_ro_lock_fail:
 	device_remove_file(disk_to_dev(md->disk), &md->force_ro);
 force_ro_fail:
 	del_gendisk(md->disk);
-
+add_disk_fail:
 	return ret;
 }
 
-- 
1.9.1

[PATCH 3/8] block/floppy.c: handle blk_register_region() return value

[Vishnu Pratap Singh]

This patch handles blk_register_region() return value.
Earlier blk_register_region() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/block/floppy.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 331363e..50802a6 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4219,8 +4219,10 @@ static int __init do_floppy_init(void)
 	if (err)
 		goto out_unreg_blkdev;
 
-	blk_register_region(MKDEV(FLOPPY_MAJOR, 0), 256, THIS_MODULE,
+	err = blk_register_region(MKDEV(FLOPPY_MAJOR, 0), 256, THIS_MODULE,
 			    floppy_find, NULL, NULL);
+	if (err)
+		goto out_unreg_region;
 
 	for (i = 0; i < 256; i++)
 		if (ITYPE(i))
@@ -4250,7 +4252,7 @@ static int __init do_floppy_init(void)
 	if (fdc_state[0].address == -1) {
 		cancel_delayed_work(&fd_timeout);
 		err = -ENODEV;
-		goto out_unreg_region;
+		goto out_fdc_err;
 	}
 #if N_FDC > 1
 	fdc_state[1].address = FDC2;
@@ -4261,7 +4263,7 @@ static int __init do_floppy_init(void)
 	if (err) {
 		cancel_delayed_work(&fd_timeout);
 		err = -EBUSY;
-		goto out_unreg_region;
+		goto out_fdc_region;
 	}
 
 	/* initialise drive state */
@@ -4357,8 +4359,9 @@ out_remove_drives:
 out_release_dma:
 	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
-out_unreg_region:
+out_fdc_err:
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
+out_unreg_region:
 	platform_driver_unregister(&floppy_driver);
 out_unreg_blkdev:
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
-- 
1.9.1

Re: [PATCH 3/8] block/floppy.c: handle blk_register_region() return value

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

Hi Vishnu,

[auto build test ERROR on ide/master]
[also build test ERROR on v4.3]
[cannot apply to block/for-next next-20151106]

url:    https://github.com/0day-ci/linux/commits/Vishnu-Pratap-Singh/block-genhd-c-Add-error-handling/20151106-204249
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/ide.git master
config: x86_64-randconfig-x002-11051802 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/block/floppy.c: In function 'do_floppy_init':
>> drivers/block/floppy.c:4266:3: error: label 'out_fdc_region' used but not defined
      goto out_fdc_region;
      ^

vim +/out_fdc_region +4266 drivers/block/floppy.c

  4260	
  4261		fdc = 0;		/* reset fdc in case of unexpected interrupt */
  4262		err = floppy_grab_irq_and_dma();
  4263		if (err) {
  4264			cancel_delayed_work(&fd_timeout);
  4265			err = -EBUSY;
> 4266			goto out_fdc_region;
  4267		}
  4268	
  4269		/* initialise drive state */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 28253 bytes --]

Re: [PATCH 3/8] block/floppy.c: handle blk_register_region() return value

[Grant Grundler]

On Fri, Nov 6, 2015 at 4:22 AM, Vishnu Pratap Singh
<vishnu.ps@samsung.com> wrote:
> This patch handles blk_register_region() return value.
> Earlier blk_register_region() function doesn't handle error
> cases, now it is added, so the callers of this function
> should also handle it.
>
> Verfied on X86 based ubuntu machine.
> This patch depends on [PATCH 1/8] block/genhd.c: Add error handling
>
> Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
> ---
>  drivers/block/floppy.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 331363e..50802a6 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4219,8 +4219,10 @@ static int __init do_floppy_init(void)
>         if (err)
>                 goto out_unreg_blkdev;
>
> -       blk_register_region(MKDEV(FLOPPY_MAJOR, 0), 256, THIS_MODULE,
> +       err = blk_register_region(MKDEV(FLOPPY_MAJOR, 0), 256, THIS_MODULE,
>                             floppy_find, NULL, NULL);
> +       if (err)
> +               goto out_unreg_region;
>
>         for (i = 0; i < 256; i++)
>                 if (ITYPE(i))
> @@ -4250,7 +4252,7 @@ static int __init do_floppy_init(void)
>         if (fdc_state[0].address == -1) {
>                 cancel_delayed_work(&fd_timeout);
>                 err = -ENODEV;
> -               goto out_unreg_region;
> +               goto out_fdc_err;
>         }
>  #if N_FDC > 1
>         fdc_state[1].address = FDC2;
> @@ -4261,7 +4263,7 @@ static int __init do_floppy_init(void)
>         if (err) {
>                 cancel_delayed_work(&fd_timeout);
>                 err = -EBUSY;
> -               goto out_unreg_region;
> +               goto out_fdc_region;

Please drop this hunk.

>         }
>
>         /* initialise drive state */
> @@ -4357,8 +4359,9 @@ out_remove_drives:
>  out_release_dma:
>         if (atomic_read(&usage_count))
>                 floppy_release_irq_and_dma();
> -out_unreg_region:
> +out_fdc_err:
>         blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
> +out_unreg_region:

Nit: out_unreg_region should continue to be a label for the
unregister_region call.

My preference would be to add a new label here so something like
"goto out_driver_unregister" works.

I normally would ignore this sort of thing but this patch needs to be
reposted as V2 anyway.

cheers,
grant

>         platform_driver_unregister(&floppy_driver);
>  out_unreg_blkdev:
>         unregister_blkdev(FLOPPY_MAJOR, "fd");
> --
> 1.9.1
>

[PATCH 4/8] block/loop.c: handle add_disk() & blk_register_region() return value

[Vishnu Pratap Singh]

This patch handles blk_register_region() & add_disk() return value.
Earlier these function doesn't handle error cases, now it is added,
so the callers of this function should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/block/loop.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 423f4ca..22e6fd0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1794,10 +1794,14 @@ static int loop_add(struct loop_device **l, int i)
 	disk->private_data	= lo;
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
-	add_disk(disk);
+	err = add_disk(disk);
+	if (err)
+		goto out_free_disk;
 	*l = lo;
 	return lo->lo_number;
 
+out_free_disk:
+	put_disk(disk);
 out_free_queue:
 	blk_cleanup_queue(lo->lo_queue);
 out_cleanup_tags:
@@ -1998,8 +2002,10 @@ static int __init loop_init(void)
 		goto misc_out;
 	}
 
-	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
+	err = blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
 				  THIS_MODULE, loop_probe, NULL, NULL);
+	if (err)
+		goto out_blkdev;
 
 	/* pre-create number of devices given by config or max_loop */
 	mutex_lock(&loop_index_mutex);
@@ -2010,6 +2016,8 @@ static int __init loop_init(void)
 	printk(KERN_INFO "loop: module loaded\n");
 	return 0;
 
+out_blkdev:
+	unregister_blkdev(LOOP_MAJOR, "loop");
 misc_out:
 	misc_deregister(&loop_misc);
 	return err;
-- 
1.9.1

[PATCH 5/8] zram: handle add_disk() & blk_register_region() return value

[Vishnu Pratap Singh]

This patch handles blk_register_region() & add_disk() return value.
Earlier these function doesn't handle error cases, now it is added,
so the callers of this function should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/block/z2ram.c         | 12 ++++++++++--
 drivers/block/zram/zram_drv.c |  9 ++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/block/z2ram.c b/drivers/block/z2ram.c
index 968f9e5..85e841f 100644
--- a/drivers/block/z2ram.c
+++ b/drivers/block/z2ram.c
@@ -364,12 +364,20 @@ z2_init(void)
     sprintf(z2ram_gendisk->disk_name, "z2ram");
 
     z2ram_gendisk->queue = z2_queue;
-    add_disk(z2ram_gendisk);
-    blk_register_region(MKDEV(Z2RAM_MAJOR, 0), Z2MINOR_COUNT, THIS_MODULE,
+    ret = add_disk(z2ram_gendisk);
+    if (ret)
+	goto out_add_disk;
+
+    ret = blk_register_region(MKDEV(Z2RAM_MAJOR, 0), Z2MINOR_COUNT, THIS_MODULE,
 				z2_find, NULL, NULL);
+    if (ret)
+	goto out_blk_reg;
 
     return 0;
 
+out_blk_reg:
+	del_gendisk(z2ram_gendisk);
+out_add_disk:
 out_queue:
     put_disk(z2ram_gendisk);
 out_disk:
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 81a557c..f3d7a49 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1261,14 +1261,16 @@ static int zram_add(void)
 		zram->disk->queue->limits.discard_zeroes_data = 0;
 	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, zram->disk->queue);
 
-	add_disk(zram->disk);
+	ret = add_disk(zram->disk);
+	if (ret)
+		goto out_free_disk;
 
 	ret = sysfs_create_group(&disk_to_dev(zram->disk)->kobj,
 				&zram_disk_attr_group);
 	if (ret < 0) {
 		pr_err("Error creating sysfs group for device %d\n",
 				device_id);
-		goto out_free_disk;
+		goto out_del_disk;
 	}
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 	zram->meta = NULL;
@@ -1277,8 +1279,9 @@ static int zram_add(void)
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;
 
-out_free_disk:
+out_del_disk:
 	del_gendisk(zram->disk);
+out_free_disk:
 	put_disk(zram->disk);
 out_free_queue:
 	blk_cleanup_queue(queue);
-- 
1.9.1

Re: [PATCH 5/8] zram: handle add_disk() & blk_register_region() return value

[Sergey Senozhatsky]

On (11/06/15 17:52), Vishnu Pratap Singh wrote:
> This patch adds error handling for blk_register_region(),
> register_disk(), add_disk(), disk_alloc_events() & disk_add_events().
> add_disk() & register_disk() functions  error handling is very much
> needed as this is used mainly by many modules like mmc, zram, mtd, scsi etc.
> But there is no error handling and it may cause stability issues also.

hm... I came across "FIXME: error handling" comment in add_disk() some
time ago and after a quick google search I found this:
https://lkml.org/lkml/2007/7/24/207

>> The attached patch fixes the problem by changing the prototype of
>> add_disk() and register_disk() to return errors.

Al Viro wrote:

> This is bogus.  Just what would callers do with these error values?
> Ignore them silently?  Bail out?  Can't do - at that point disk just
> might have been opened already.  add_disk() is the point of no return;
> we are already past the last point where we could bail out.




>  drivers/block/z2ram.c         | 12 ++++++++++--
>  drivers/block/zram/zram_drv.c |  9 ++++++---

those are different drivers, split the patches (well, if add_disk()
change actually makes sense).


>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/z2ram.c b/drivers/block/z2ram.c
> index 968f9e5..85e841f 100644
> --- a/drivers/block/z2ram.c
> +++ b/drivers/block/z2ram.c
> @@ -364,12 +364,20 @@ z2_init(void)
>      sprintf(z2ram_gendisk->disk_name, "z2ram");
>  
>      z2ram_gendisk->queue = z2_queue;
> -    add_disk(z2ram_gendisk);
> -    blk_register_region(MKDEV(Z2RAM_MAJOR, 0), Z2MINOR_COUNT, THIS_MODULE,
> +    ret = add_disk(z2ram_gendisk);
> +    if (ret)
> +	goto out_add_disk;
> +
> +    ret = blk_register_region(MKDEV(Z2RAM_MAJOR, 0), Z2MINOR_COUNT, THIS_MODULE,
>  				z2_find, NULL, NULL);
> +    if (ret)
> +	goto out_blk_reg;

A separate nitpick. z2_init() coding styles need to be 'fixed'. So the
patch will not violate the kernel coding styles.

 ./scripts/checkpatch.pl
 WARNING: please, no spaces at the start of a line
 #113: FILE: drivers/block/z2ram.c:367:
 +    ret = add_disk(z2ram_gendisk);$

 WARNING: please, no spaces at the start of a line
 #114: FILE: drivers/block/z2ram.c:368:
 +    if (ret)$

 WARNING: please, no spaces at the start of a line
 #117: FILE: drivers/block/z2ram.c:371:
 +    ret = blk_register_region(MKDEV(Z2RAM_MAJOR, 0), Z2MINOR_COUNT,
 THIS_MODULE,$

 WARNING: please, no spaces at the start of a line
 #119: FILE: drivers/block/z2ram.c:373:
 +    if (ret)$

 total: 0 errors, 4 warnings, 50 lines checked


>  
>      return 0;
>  
> +out_blk_reg:
> +	del_gendisk(z2ram_gendisk);
> +out_add_disk:
>  out_queue:

Hm... a fall-through empty `out_add_disk' label?


	-ss

>      put_disk(z2ram_gendisk);
>  out_disk:
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 81a557c..f3d7a49 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -1261,14 +1261,16 @@ static int zram_add(void)
>  		zram->disk->queue->limits.discard_zeroes_data = 0;
>  	queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, zram->disk->queue);
>  
> -	add_disk(zram->disk);
> +	ret = add_disk(zram->disk);
> +	if (ret)
> +		goto out_free_disk;
>  
>  	ret = sysfs_create_group(&disk_to_dev(zram->disk)->kobj,
>  				&zram_disk_attr_group);
>  	if (ret < 0) {
>  		pr_err("Error creating sysfs group for device %d\n",
>  				device_id);
> -		goto out_free_disk;
> +		goto out_del_disk;
>  	}
>  	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
>  	zram->meta = NULL;
> @@ -1277,8 +1279,9 @@ static int zram_add(void)
>  	pr_info("Added device: %s\n", zram->disk->disk_name);
>  	return device_id;
>  
> -out_free_disk:
> +out_del_disk:
>  	del_gendisk(zram->disk);
> +out_free_disk:
>  	put_disk(zram->disk);

>  out_free_queue:
>  	blk_cleanup_queue(queue);
> -- 
> 1.9.1
> 

[PATCH 6/8] md/md.c: handle blk_register_region() return value

[Vishnu Pratap Singh]

This patch handles blk_register_region() return value.
Earlier blk_register_region() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/md/md.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index a71b36f..244bb26 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8940,10 +8940,12 @@ static int __init md_init(void)
 		goto err_mdp;
 	mdp_major = ret;
 
-	blk_register_region(MKDEV(MD_MAJOR, 0), 512, THIS_MODULE,
-			    md_probe, NULL, NULL);
-	blk_register_region(MKDEV(mdp_major, 0), 1UL<<MINORBITS, THIS_MODULE,
-			    md_probe, NULL, NULL);
+	if (blk_register_region(MKDEV(MD_MAJOR, 0), 512, THIS_MODULE,
+			    md_probe, NULL, NULL))
+		goto err_md_blk;
+	if (blk_register_region(MKDEV(mdp_major, 0), 1UL<<MINORBITS,
+			    THIS_MODULE, md_probe, NULL, NULL))
+		goto err_mdp_blk;
 
 	register_reboot_notifier(&md_notifier);
 	raid_table_header = register_sysctl_table(raid_root_table);
@@ -8951,6 +8953,10 @@ static int __init md_init(void)
 	md_geninit();
 	return 0;
 
+err_mdp_blk:
+	blk_unregister_region(MKDEV(MD_MAJOR, 0), 512);
+err_md_blk:
+	unregister_blkdev(MD_MAJOR, "mdp");
 err_mdp:
 	unregister_blkdev(MD_MAJOR, "md");
 err_md:
-- 
1.9.1

[PATCH 7/8] cdrom/gdrom.c: handle add_disk() return value

[Vishnu Pratap Singh]

This patch handles  add_disk() return value.
Earlier add_disk() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/cdrom/gdrom.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 584bc31..38ca43a 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -817,9 +817,14 @@ static int probe_gdrom(struct platform_device *devptr)
 	gd.toc = kzalloc(sizeof(struct gdromtoc), GFP_KERNEL);
 	if (!gd.toc)
 		goto probe_fail_toc;
-	add_disk(gd.disk);
+	err = add_disk(gd.disk);
+	if (err)
+		goto probe_fail_add_disk;
+
 	return 0;
 
+probe_fail_add_disk:
+	kfree(gd.toc);
 probe_fail_toc:
 	blk_cleanup_queue(gd.gdrom_rq);
 probe_fail_requestq:
-- 
1.9.1

[PATCH 8/8] ide/ide-probe.c: handle blk_register_region() return value

[Vishnu Pratap Singh]

This patch handles blk_register_region() return value.
Earlier blk_register_region() function doesn't handle error
cases, now it is added, so the callers of this function
should also handle it.

Verfied on X86 based ubuntu machine.
This patch depends on [PATCH 1/8] block/genhd.c: Add error handling

Signed-off-by: Vishnu Pratap Singh <vishnu.ps@samsung.com>
---
 drivers/ide/ide-probe.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 0b63fac..651eb05 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -917,9 +917,9 @@ static int exact_lock(dev_t dev, void *data)
 	return 0;
 }
 
-void ide_register_region(struct gendisk *disk)
+int ide_register_region(struct gendisk *disk)
 {
-	blk_register_region(MKDEV(disk->major, disk->first_minor),
+	return blk_register_region(MKDEV(disk->major, disk->first_minor),
 			    disk->minors, NULL, exact_match, exact_lock, disk);
 }
 
@@ -988,8 +988,9 @@ static int hwif_init(ide_hwif_t *hwif)
 		goto out;
 	}
 
-	blk_register_region(MKDEV(hwif->major, 0), MAX_DRIVES << PARTN_BITS,
-			    THIS_MODULE, ata_probe, ata_lock, hwif);
+	if (blk_register_region(MKDEV(hwif->major, 0), MAX_DRIVES << PARTN_BITS,
+			    THIS_MODULE, ata_probe, ata_lock, hwif))
+		goto out;
 	return 1;
 
 out:
-- 
1.9.1

Re: [PATCH 1/8] block/genhd.c: Add error handling

[Al Viro]

On Fri, Nov 06, 2015 at 05:52:08PM +0530, Vishnu Pratap Singh wrote:

Have you even tried to trigger the failure exits you've added?  The
more you've successfully set up, the _less_ your cleanup code ends
up undoing; that simply can't be right.

That aside, as soon as we'd done register_disk(), the damn thing is
available for open(), so bailing out is _really_ not something for
faint-hearted - it's essentially equivalent to removal of device under
somebody who'd opened it and might've started IO, etc.  Going there
simply because some sysfs shite didn't get created doesn't look sane
as far as I'm concerned...

Especially since these failure exits are not going to be tested on
a regular basis, so the amount of bitrot will be pretty high ;-/

Re: [PATCH 1/8] block/genhd.c: Add error handling

[Jens Axboe]

On 11/09/2015 08:33 PM, Al Viro wrote:
> On Fri, Nov 06, 2015 at 05:52:08PM +0530, Vishnu Pratap Singh wrote:
>
> Have you even tried to trigger the failure exits you've added?  The
> more you've successfully set up, the _less_ your cleanup code ends
> up undoing; that simply can't be right.
>
> That aside, as soon as we'd done register_disk(), the damn thing is
> available for open(), so bailing out is _really_ not something for
> faint-hearted - it's essentially equivalent to removal of device under
> somebody who'd opened it and might've started IO, etc.  Going there
> simply because some sysfs shite didn't get created doesn't look sane
> as far as I'm concerned...
>
> Especially since these failure exits are not going to be tested on
> a regular basis, so the amount of bitrot will be pretty high ;-/

I'd greatly prefer it we just leave it alone. Much better to have a disk 
that does actually work and with some sysfs spew in the logs, than bail 
out for fairly vague reasons.

The road to hell is paved with good intentions. It's a noble thought to 
want to clean this up, but I think it does more harm than good.

-- 
Jens Axboe

Re: [PATCH 1/8] block/genhd.c: Add error handling

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 11/09/2015 08:33 PM, Al Viro wrote:
>> On Fri, Nov 06, 2015 at 05:52:08PM +0530, Vishnu Pratap Singh wrote:
>>
>> Have you even tried to trigger the failure exits you've added?  The
>> more you've successfully set up, the _less_ your cleanup code ends
>> up undoing; that simply can't be right.
>>
>> That aside, as soon as we'd done register_disk(), the damn thing is
>> available for open(), so bailing out is _really_ not something for
>> faint-hearted - it's essentially equivalent to removal of device under
>> somebody who'd opened it and might've started IO, etc.  Going there
>> simply because some sysfs shite didn't get created doesn't look sane
>> as far as I'm concerned...
>>
>> Especially since these failure exits are not going to be tested on
>> a regular basis, so the amount of bitrot will be pretty high ;-/
>
> I'd greatly prefer it we just leave it alone. Much better to have a
> disk that does actually work and with some sysfs spew in the logs,
> than bail out for fairly vague reasons.
>
> The road to hell is paved with good intentions. It's a noble thought
> to want to clean this up, but I think it does more harm than good.

That's my fault, I asked Vishnu to repost.  I had also asked whether he
had seen this in the real world, and this was the response:

  Actually while working with zram I found that during zram
  initialization it uses the add_disk function and during that time i
  have seen sometimes under low mem condition when we enable swap (zram)
  there was kzalloc fail to allocate he memory, since there is no error
  handling it took good amount of time to debug.

Cheers,
Jeff