Vishnu Pratap Singh <vishnu...@samsung.com> writes:
> 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.
Hi, Vishnu,
The patch looks fine, but I have to ask what prompted you to write it?
If there's a specific failure you saw, it would be interesting to know
what that was.
If we're going to go down this path, then I think we should add error
handling to the callers of add_disk and blk_register_region. Is that
something you'd be interested in working on?
Cheers,
Jeff
> Verfied on X86 based ubuntu machine.
>
> Signed-off-by: Vishnu Pratap Singh <vishnu...@samsung.com>
> ---
> block/genhd.c | 92
> ++++++++++++++++++++++++++++++++++---------------
> include/linux/genhd.h | 4 +--
> 2 files changed, 67 insertions(+), 29 deletions(-)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index e552e1b..8589e01 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;
> }
> }
>
> @@ -558,6 +559,7 @@ static void register_disk(struct gendisk *disk)
> if (err < 0)
> goto exit;
> blkdev_put(bdev, FMODE_READ);
> + return 0;
>
> exit:
> /* announce disk after possible partitions are created */
> @@ -569,6 +571,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 +580,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 +599,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,16 +609,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()
> @@ -627,9 +639,29 @@ 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;
> +
> + 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;
>
> - disk_add_events(disk);
> }
> EXPORT_SYMBOL(add_disk);
>
> @@ -1779,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);
> @@ -1801,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;
> +
> if (!disk->ev)
> - return;
> + return 0;
>
> - /* 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);
> @@ -1822,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 ec274e0..9bcd710 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -416,7 +416,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);
> @@ -619,7 +619,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 *),
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
