This is the 5th of 7 set of driver conversion over to use the new add_disk() error handling. Please let me know if you spot any issues. This set deals with miscellaneous block drivers. This patch set is based on axboe/master, you can find the full set of changes on my 20210901-for-axboe-add-disk-error-handling branch [0]. It would seem there are going to be a total of 7 sets of patches. The next one will be the wonderful and exciting world of floppy drivers. The last is the required changes to add a __must_check for the return value for the caller. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210901-for-axboe-add-disk-error-handling Luis Chamberlain (9): cdrom/gdrom: add error handling support for add_disk() ms_block: add error handling support for add_disk() mspro_block: add error handling support for add_disk() rbd: add add_disk() error handling mtd: add add_disk() error handling s390/block/dasd_genhd: add error handling support for add_disk() s390/block/dcssblk: add error handling support for add_disk() s390/block/scm_blk: add error handling support for add_disk() s390/block/xpram: add error handling support for add_disk() drivers/block/rbd.c | 6 +++++- drivers/cdrom/gdrom.c | 7 ++++++- drivers/memstick/core/ms_block.c | 6 +++++- drivers/memstick/core/mspro_block.c | 6 +++++- drivers/mtd/mtd_blkdevs.c | 6 +++++- drivers/s390/block/dasd_genhd.c | 8 ++++++-- drivers/s390/block/dcssblk.c | 4 +++- drivers/s390/block/scm_blk.c | 7 ++++++- drivers/s390/block/xpram.c | 4 +++- 9 files changed, 44 insertions(+), 10 deletions(-) -- 2.30.2
This is the 5th of 7 set of driver conversion over to use the new add_disk() error handling. Please let me know if you spot any issues. This set deals with miscellaneous block drivers. This patch set is based on axboe/master, you can find the full set of changes on my 20210901-for-axboe-add-disk-error-handling branch [0]. It would seem there are going to be a total of 7 sets of patches. The next one will be the wonderful and exciting world of floppy drivers. The last is the required changes to add a __must_check for the return value for the caller. [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210901-for-axboe-add-disk-error-handling Luis Chamberlain (9): cdrom/gdrom: add error handling support for add_disk() ms_block: add error handling support for add_disk() mspro_block: add error handling support for add_disk() rbd: add add_disk() error handling mtd: add add_disk() error handling s390/block/dasd_genhd: add error handling support for add_disk() s390/block/dcssblk: add error handling support for add_disk() s390/block/scm_blk: add error handling support for add_disk() s390/block/xpram: add error handling support for add_disk() drivers/block/rbd.c | 6 +++++- drivers/cdrom/gdrom.c | 7 ++++++- drivers/memstick/core/ms_block.c | 6 +++++- drivers/memstick/core/mspro_block.c | 6 +++++- drivers/mtd/mtd_blkdevs.c | 6 +++++- drivers/s390/block/dasd_genhd.c | 8 ++++++-- drivers/s390/block/dcssblk.c | 4 +++- drivers/s390/block/scm_blk.c | 7 ++++++- drivers/s390/block/xpram.c | 4 +++- 9 files changed, 44 insertions(+), 10 deletions(-) -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- 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 8e1fe75af93f..d50cc1fd34d5 100644 --- a/drivers/cdrom/gdrom.c +++ b/drivers/cdrom/gdrom.c @@ -805,9 +805,14 @@ static int probe_gdrom(struct platform_device *devptr) err = -ENOMEM; goto probe_fail_free_irqs; } - 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_free_irqs: free_irq(HW_EVENT_GDROM_DMA, &gd); free_irq(HW_EVENT_GDROM_CMD, &gd); -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- 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 8e1fe75af93f..d50cc1fd34d5 100644 --- a/drivers/cdrom/gdrom.c +++ b/drivers/cdrom/gdrom.c @@ -805,9 +805,14 @@ static int probe_gdrom(struct platform_device *devptr) err = -ENOMEM; goto probe_fail_free_irqs; } - 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_free_irqs: free_irq(HW_EVENT_GDROM_DMA, &gd); free_irq(HW_EVENT_GDROM_CMD, &gd); -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Contrary to the typical removal which delays the put_disk() until later, since we are failing on a probe we immediately put the disk on failure from add_disk by using blk_cleanup_disk(). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/memstick/core/ms_block.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c index 4a4573fa7b0f..86c626933c1a 100644 --- a/drivers/memstick/core/ms_block.c +++ b/drivers/memstick/core/ms_block.c @@ -2156,10 +2156,14 @@ static int msb_init_disk(struct memstick_dev *card) set_disk_ro(msb->disk, 1); msb_start(card); - device_add_disk(&card->dev, msb->disk, NULL); + rc = device_add_disk(&card->dev, msb->disk, NULL); + if (rc) + goto out_cleanup_disk; dbg("Disk added"); return 0; +out_cleanup_disk: + blk_cleanup_disk(msb->disk); out_free_tag_set: blk_mq_free_tag_set(&msb->tag_set); out_release_id: -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Contrary to the typical removal which delays the put_disk() until later, since we are failing on a probe we immediately put the disk on failure from add_disk by using blk_cleanup_disk(). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/memstick/core/ms_block.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c index 4a4573fa7b0f..86c626933c1a 100644 --- a/drivers/memstick/core/ms_block.c +++ b/drivers/memstick/core/ms_block.c @@ -2156,10 +2156,14 @@ static int msb_init_disk(struct memstick_dev *card) set_disk_ro(msb->disk, 1); msb_start(card); - device_add_disk(&card->dev, msb->disk, NULL); + rc = device_add_disk(&card->dev, msb->disk, NULL); + if (rc) + goto out_cleanup_disk; dbg("Disk added"); return 0; +out_cleanup_disk: + blk_cleanup_disk(msb->disk); out_free_tag_set: blk_mq_free_tag_set(&msb->tag_set); out_release_id: -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Contrary to the typical removal which delays the put_disk() until later, since we are failing on a probe we immediately put the disk on failure from add_disk by using blk_cleanup_disk(). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/memstick/core/mspro_block.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c index 22778d0e24f5..c0450397b673 100644 --- a/drivers/memstick/core/mspro_block.c +++ b/drivers/memstick/core/mspro_block.c @@ -1239,10 +1239,14 @@ static int mspro_block_init_disk(struct memstick_dev *card) set_capacity(msb->disk, capacity); dev_dbg(&card->dev, "capacity set %ld\n", capacity); - device_add_disk(&card->dev, msb->disk, NULL); + rc = device_add_disk(&card->dev, msb->disk, NULL); + if (rc) + goto out_cleanup_disk; msb->active = 1; return 0; +out_cleanup_disk: + blk_cleanup_disk(msb->disk); out_free_tag_set: blk_mq_free_tag_set(&msb->tag_set); out_release_id: -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Contrary to the typical removal which delays the put_disk() until later, since we are failing on a probe we immediately put the disk on failure from add_disk by using blk_cleanup_disk(). Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/memstick/core/mspro_block.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c index 22778d0e24f5..c0450397b673 100644 --- a/drivers/memstick/core/mspro_block.c +++ b/drivers/memstick/core/mspro_block.c @@ -1239,10 +1239,14 @@ static int mspro_block_init_disk(struct memstick_dev *card) set_capacity(msb->disk, capacity); dev_dbg(&card->dev, "capacity set %ld\n", capacity); - device_add_disk(&card->dev, msb->disk, NULL); + rc = device_add_disk(&card->dev, msb->disk, NULL); + if (rc) + goto out_cleanup_disk; msb->active = 1; return 0; +out_cleanup_disk: + blk_cleanup_disk(msb->disk); out_free_tag_set: blk_mq_free_tag_set(&msb->tag_set); out_release_id: -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/block/rbd.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index e65c9d706f6f..341e5da6d029 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -7054,7 +7054,9 @@ static ssize_t do_rbd_add(struct bus_type *bus, if (rc) goto err_out_image_lock; - device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL); + rc = device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL); + if (rc) + goto err_out_cleanup_disk; spin_lock(&rbd_dev_list_lock); list_add_tail(&rbd_dev->node, &rbd_dev_list); @@ -7068,6 +7070,8 @@ static ssize_t do_rbd_add(struct bus_type *bus, module_put(THIS_MODULE); return rc; +err_out_cleanup_disk: + rbd_free_disk(rbd_dev); err_out_image_lock: rbd_dev_image_unlock(rbd_dev); rbd_dev_device_release(rbd_dev); -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/block/rbd.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index e65c9d706f6f..341e5da6d029 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -7054,7 +7054,9 @@ static ssize_t do_rbd_add(struct bus_type *bus, if (rc) goto err_out_image_lock; - device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL); + rc = device_add_disk(&rbd_dev->dev, rbd_dev->disk, NULL); + if (rc) + goto err_out_cleanup_disk; spin_lock(&rbd_dev_list_lock); list_add_tail(&rbd_dev->node, &rbd_dev_list); @@ -7068,6 +7070,8 @@ static ssize_t do_rbd_add(struct bus_type *bus, module_put(THIS_MODULE); return rc; +err_out_cleanup_disk: + rbd_free_disk(rbd_dev); err_out_image_lock: rbd_dev_image_unlock(rbd_dev); rbd_dev_device_release(rbd_dev); -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/mtd/mtd_blkdevs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 44bea3f65060..343ff96589cc 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -427,7 +427,9 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) if (new->readonly) set_disk_ro(gd, 1); - device_add_disk(&new->mtd->dev, gd, NULL); + ret = device_add_disk(&new->mtd->dev, gd, NULL); + if (ret) + goto out_cleanup_disk; if (new->disk_attributes) { ret = sysfs_create_group(&disk_to_dev(gd)->kobj, @@ -436,6 +438,8 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) } return 0; +out_cleanup_disk: + blk_cleanup_disk(new->disk); out_free_tag_set: blk_mq_free_tag_set(new->tag_set); out_kfree_tag_set: -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/mtd/mtd_blkdevs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index 44bea3f65060..343ff96589cc 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -427,7 +427,9 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) if (new->readonly) set_disk_ro(gd, 1); - device_add_disk(&new->mtd->dev, gd, NULL); + ret = device_add_disk(&new->mtd->dev, gd, NULL); + if (ret) + goto out_cleanup_disk; if (new->disk_attributes) { ret = sysfs_create_group(&disk_to_dev(gd)->kobj, @@ -436,6 +438,8 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) } return 0; +out_cleanup_disk: + blk_cleanup_disk(new->disk); out_free_tag_set: blk_mq_free_tag_set(new->tag_set); out_kfree_tag_set: -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/dasd_genhd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..ba07022283bc 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) + return rc; + return 0; } -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/dasd_genhd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..ba07022283bc 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) + return rc; + return 0; } -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/dcssblk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 5be3d1c39a78..b0fd5009a12e 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char } get_device(&dev_info->dev); - device_add_disk(&dev_info->dev, dev_info->gd, NULL); + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); + if (rc) + goto put_dev; switch (dev_info->segment_type) { case SEG_TYPE_SR: -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/dcssblk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 5be3d1c39a78..b0fd5009a12e 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char } get_device(&dev_info->dev); - device_add_disk(&dev_info->dev, dev_info->gd, NULL); + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); + if (rc) + goto put_dev; switch (dev_info->segment_type) { case SEG_TYPE_SR: -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/scm_blk.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/scm_blk.c b/drivers/s390/block/scm_blk.c index 88cba6212ee2..61ecdcb2cc6a 100644 --- a/drivers/s390/block/scm_blk.c +++ b/drivers/s390/block/scm_blk.c @@ -495,9 +495,14 @@ int scm_blk_dev_setup(struct scm_blk_dev *bdev, struct scm_device *scmdev) /* 512 byte sectors */ set_capacity(bdev->gendisk, scmdev->size >> 9); - device_add_disk(&scmdev->dev, bdev->gendisk, NULL); + ret = device_add_disk(&scmdev->dev, bdev->gendisk, NULL); + if (ret) + goto out_cleanup_disk; + return 0; +out_cleanup_disk: + blk_cleanup_disk(bdev->gendisk); out_tag: blk_mq_free_tag_set(&bdev->tag_set); out: -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/scm_blk.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/scm_blk.c b/drivers/s390/block/scm_blk.c index 88cba6212ee2..61ecdcb2cc6a 100644 --- a/drivers/s390/block/scm_blk.c +++ b/drivers/s390/block/scm_blk.c @@ -495,9 +495,14 @@ int scm_blk_dev_setup(struct scm_blk_dev *bdev, struct scm_device *scmdev) /* 512 byte sectors */ set_capacity(bdev->gendisk, scmdev->size >> 9); - device_add_disk(&scmdev->dev, bdev->gendisk, NULL); + ret = device_add_disk(&scmdev->dev, bdev->gendisk, NULL); + if (ret) + goto out_cleanup_disk; + return 0; +out_cleanup_disk: + blk_cleanup_disk(bdev->gendisk); out_tag: blk_mq_free_tag_set(&bdev->tag_set); out: -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/xpram.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c index ce98fab4d43c..ed3904b6a9c8 100644 --- a/drivers/s390/block/xpram.c +++ b/drivers/s390/block/xpram.c @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void) disk->private_data = &xpram_devices[i]; sprintf(disk->disk_name, "slram%d", i); set_capacity(disk, xpram_sizes[i] << 1); - add_disk(disk); + rc = add_disk(disk); + if (rc) + goto out; } return 0; -- 2.30.2
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/s390/block/xpram.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c index ce98fab4d43c..ed3904b6a9c8 100644 --- a/drivers/s390/block/xpram.c +++ b/drivers/s390/block/xpram.c @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void) disk->private_data = &xpram_devices[i]; sprintf(disk->disk_name, "slram%d", i); set_capacity(disk, xpram_sizes[i] << 1); - add_disk(disk); + rc = add_disk(disk); + if (rc) + goto out; } return 0; -- 2.30.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Luis,
Luis Chamberlain <mcgrof@kernel.org> wrote on Thu, 2 Sep 2021 10:41:01
-0700:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Thanks,
Miquèl
Hi Luis, Luis Chamberlain <mcgrof@kernel.org> wrote on Thu, 2 Sep 2021 10:41:01 -0700: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, Sep 02, 2021 at 10:41:05AM -0700, Luis Chamberlain wrote:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> drivers/s390/block/xpram.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c
> index ce98fab4d43c..ed3904b6a9c8 100644
> --- a/drivers/s390/block/xpram.c
> +++ b/drivers/s390/block/xpram.c
> @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void)
> disk->private_data = &xpram_devices[i];
> sprintf(disk->disk_name, "slram%d", i);
> set_capacity(disk, xpram_sizes[i] << 1);
> - add_disk(disk);
> + rc = add_disk(disk);
> + if (rc)
> + goto out;
Hmm, this is a more or less dead device driver, and I'm wondering if
we shouldn't remove it instead. But anyway, your patch is not correct:
- del_gendisk for all registered disks has to be called
- unregister_blkdev(XPRAM_MAJOR, XPRAM_NAME) is missing as well
That would be more or or less xpram_exit with parameter.
You can send a new patch or I can provide a proper one, whatever you
prefer.
On Thu, Sep 02, 2021 at 10:41:05AM -0700, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/xpram.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c > index ce98fab4d43c..ed3904b6a9c8 100644 > --- a/drivers/s390/block/xpram.c > +++ b/drivers/s390/block/xpram.c > @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void) > disk->private_data = &xpram_devices[i]; > sprintf(disk->disk_name, "slram%d", i); > set_capacity(disk, xpram_sizes[i] << 1); > - add_disk(disk); > + rc = add_disk(disk); > + if (rc) > + goto out; Hmm, this is a more or less dead device driver, and I'm wondering if we shouldn't remove it instead. But anyway, your patch is not correct: - del_gendisk for all registered disks has to be called - unregister_blkdev(XPRAM_MAJOR, XPRAM_NAME) is missing as well That would be more or or less xpram_exit with parameter. You can send a new patch or I can provide a proper one, whatever you prefer. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> drivers/s390/block/dcssblk.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> index 5be3d1c39a78..b0fd5009a12e 100644
> --- a/drivers/s390/block/dcssblk.c
> +++ b/drivers/s390/block/dcssblk.c
> @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
> }
>
> get_device(&dev_info->dev);
> - device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> + if (rc)
> + goto put_dev;
This looks not correct to me. We seem to have now in case of an error:
- reference count imbalance (= memory leak)
- dax cleanup is missing
On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/dcssblk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > index 5be3d1c39a78..b0fd5009a12e 100644 > --- a/drivers/s390/block/dcssblk.c > +++ b/drivers/s390/block/dcssblk.c > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > } > > get_device(&dev_info->dev); > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > + if (rc) > + goto put_dev; This looks not correct to me. We seem to have now in case of an error: - reference count imbalance (= memory leak) - dax cleanup is missing ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, Sep 02, 2021 at 10:41:04AM -0700, Luis Chamberlain wrote:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
> drivers/s390/block/scm_blk.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Acked-by: Heiko Carstens <hca@linux.ibm.com>
On Thu, Sep 02, 2021 at 10:41:04AM -0700, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/scm_blk.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Acked-by: Heiko Carstens <hca@linux.ibm.com> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote:
> On Thu, Sep 02, 2021 at 10:41:05AM -0700, Luis Chamberlain wrote:
> > We never checked for errors on add_disk() as this function
> > returned void. Now that this is fixed, use the shiny new
> > error handling.
> >
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> > drivers/s390/block/xpram.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c
> > index ce98fab4d43c..ed3904b6a9c8 100644
> > --- a/drivers/s390/block/xpram.c
> > +++ b/drivers/s390/block/xpram.c
> > @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void)
> > disk->private_data = &xpram_devices[i];
> > sprintf(disk->disk_name, "slram%d", i);
> > set_capacity(disk, xpram_sizes[i] << 1);
> > - add_disk(disk);
> > + rc = add_disk(disk);
> > + if (rc)
> > + goto out;
>
> Hmm, this is a more or less dead device driver, and I'm wondering if
> we shouldn't remove it instead. But anyway, your patch is not correct:
>
> - del_gendisk for all registered disks has to be called
> - unregister_blkdev(XPRAM_MAJOR, XPRAM_NAME) is missing as well
>
> That would be more or or less xpram_exit with parameter.
>
> You can send a new patch or I can provide a proper one, whatever you
> prefer.
You are more familiar with the code so it would be great if you can send
a patch replacement and I will drop mine. I have quite a bit of other
conversions to deal with.
Luis
On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote: > On Thu, Sep 02, 2021 at 10:41:05AM -0700, Luis Chamberlain wrote: > > We never checked for errors on add_disk() as this function > > returned void. Now that this is fixed, use the shiny new > > error handling. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > drivers/s390/block/xpram.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/s390/block/xpram.c b/drivers/s390/block/xpram.c > > index ce98fab4d43c..ed3904b6a9c8 100644 > > --- a/drivers/s390/block/xpram.c > > +++ b/drivers/s390/block/xpram.c > > @@ -371,7 +371,9 @@ static int __init xpram_setup_blkdev(void) > > disk->private_data = &xpram_devices[i]; > > sprintf(disk->disk_name, "slram%d", i); > > set_capacity(disk, xpram_sizes[i] << 1); > > - add_disk(disk); > > + rc = add_disk(disk); > > + if (rc) > > + goto out; > > Hmm, this is a more or less dead device driver, and I'm wondering if > we shouldn't remove it instead. But anyway, your patch is not correct: > > - del_gendisk for all registered disks has to be called > - unregister_blkdev(XPRAM_MAJOR, XPRAM_NAME) is missing as well > > That would be more or or less xpram_exit with parameter. > > You can send a new patch or I can provide a proper one, whatever you > prefer. You are more familiar with the code so it would be great if you can send a patch replacement and I will drop mine. I have quite a bit of other conversions to deal with. Luis ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote:
> On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote:
> > We never checked for errors on add_disk() as this function
> > returned void. Now that this is fixed, use the shiny new
> > error handling.
> >
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> > drivers/s390/block/dcssblk.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> > index 5be3d1c39a78..b0fd5009a12e 100644
> > --- a/drivers/s390/block/dcssblk.c
> > +++ b/drivers/s390/block/dcssblk.c
> > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
> > }
> >
> > get_device(&dev_info->dev);
> > - device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > + if (rc)
> > + goto put_dev;
>
> This looks not correct to me. We seem to have now in case of an error:
>
> - reference count imbalance (= memory leak)
> - dax cleanup is missing
Care to provide an alternative?
Luis
On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote: > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > > We never checked for errors on add_disk() as this function > > returned void. Now that this is fixed, use the shiny new > > error handling. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > drivers/s390/block/dcssblk.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > index 5be3d1c39a78..b0fd5009a12e 100644 > > --- a/drivers/s390/block/dcssblk.c > > +++ b/drivers/s390/block/dcssblk.c > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > } > > > > get_device(&dev_info->dev); > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > + if (rc) > > + goto put_dev; > > This looks not correct to me. We seem to have now in case of an error: > > - reference count imbalance (= memory leak) > - dax cleanup is missing Care to provide an alternative? Luis ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote:
> Hmm, this is a more or less dead device driver, and I'm wondering if
> we shouldn't remove it instead. But anyway, your patch is not correct:
I'm all for removing it. I think we need to do a little more spring
cleaning on unmaintained and likely to be unused block drivers.
On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote: > Hmm, this is a more or less dead device driver, and I'm wondering if > we shouldn't remove it instead. But anyway, your patch is not correct: I'm all for removing it. I think we need to do a little more spring cleaning on unmaintained and likely to be unused block drivers. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, 3 Sep 2021 18:46:26 -0700 Luis Chamberlain <mcgrof@kernel.org> wrote: > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote: > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > > > We never checked for errors on add_disk() as this function > > > returned void. Now that this is fixed, use the shiny new > > > error handling. > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > > --- > > > drivers/s390/block/dcssblk.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > > index 5be3d1c39a78..b0fd5009a12e 100644 > > > --- a/drivers/s390/block/dcssblk.c > > > +++ b/drivers/s390/block/dcssblk.c > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > > } > > > > > > get_device(&dev_info->dev); > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > + if (rc) > > > + goto put_dev; > > > > This looks not correct to me. We seem to have now in case of an error: > > > > - reference count imbalance (= memory leak) > > - dax cleanup is missing > > Care to provide an alternative? See patch below: From 7053b5f8c0a126c3ef450de3668d9963bd68ceaa Mon Sep 17 00:00:00 2001 From: Gerald Schaefer <gerald.schaefer@linux.ibm.com> Date: Mon, 6 Sep 2021 13:18:53 +0200 Subject: [PATCH] s390/block/dcssblk: add error handling support for add_disk() Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> --- drivers/s390/block/dcssblk.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 5be3d1c39a78..0741a9321712 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char } get_device(&dev_info->dev); - device_add_disk(&dev_info->dev, dev_info->gd, NULL); + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); + if (rc) + goto out_dax; switch (dev_info->segment_type) { case SEG_TYPE_SR: @@ -712,6 +714,10 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char rc = count; goto out; +out_dax: + put_device(&dev_info->dev); + kill_dax(dev_info->dax_dev); + put_dax(dev_info->dax_dev); put_dev: list_del(&dev_info->lh); blk_cleanup_disk(dev_info->gd); -- 2.25.1
On Fri, 3 Sep 2021 18:46:26 -0700 Luis Chamberlain <mcgrof@kernel.org> wrote: > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote: > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > > > We never checked for errors on add_disk() as this function > > > returned void. Now that this is fixed, use the shiny new > > > error handling. > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > > --- > > > drivers/s390/block/dcssblk.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > > index 5be3d1c39a78..b0fd5009a12e 100644 > > > --- a/drivers/s390/block/dcssblk.c > > > +++ b/drivers/s390/block/dcssblk.c > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > > } > > > > > > get_device(&dev_info->dev); > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > + if (rc) > > > + goto put_dev; > > > > This looks not correct to me. We seem to have now in case of an error: > > > > - reference count imbalance (= memory leak) > > - dax cleanup is missing > > Care to provide an alternative? See patch below: From 7053b5f8c0a126c3ef450de3668d9963bd68ceaa Mon Sep 17 00:00:00 2001 From: Gerald Schaefer <gerald.schaefer@linux.ibm.com> Date: Mon, 6 Sep 2021 13:18:53 +0200 Subject: [PATCH] s390/block/dcssblk: add error handling support for add_disk() Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> --- drivers/s390/block/dcssblk.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 5be3d1c39a78..0741a9321712 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char } get_device(&dev_info->dev); - device_add_disk(&dev_info->dev, dev_info->gd, NULL); + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); + if (rc) + goto out_dax; switch (dev_info->segment_type) { case SEG_TYPE_SR: @@ -712,6 +714,10 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char rc = count; goto out; +out_dax: + put_device(&dev_info->dev); + kill_dax(dev_info->dax_dev); + put_dax(dev_info->dax_dev); put_dev: list_del(&dev_info->lh); blk_cleanup_disk(dev_info->gd); -- 2.25.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote:
> On Fri, 3 Sep 2021 18:46:26 -0700
> Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > > > + if (rc)
> > > > + goto put_dev;
> > >
> > > This looks not correct to me. We seem to have now in case of an error:
> > >
> > > - reference count imbalance (= memory leak)
> > > - dax cleanup is missing
> >
> > Care to provide an alternative?
>
> See patch below:
>
> From 7053b5f8c0a126c3ef450de3668d9963bd68ceaa Mon Sep 17 00:00:00 2001
> From: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> Date: Mon, 6 Sep 2021 13:18:53 +0200
> Subject: [PATCH] s390/block/dcssblk: add error handling support for add_disk()
>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> ---
> drivers/s390/block/dcssblk.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
Thanks Gerald! FWIW:
Acked-by: Heiko Carstens <hca@linux.ibm.com>
On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote: > On Fri, 3 Sep 2021 18:46:26 -0700 > Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > > + if (rc) > > > > + goto put_dev; > > > > > > This looks not correct to me. We seem to have now in case of an error: > > > > > > - reference count imbalance (= memory leak) > > > - dax cleanup is missing > > > > Care to provide an alternative? > > See patch below: > > From 7053b5f8c0a126c3ef450de3668d9963bd68ceaa Mon Sep 17 00:00:00 2001 > From: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > Date: Mon, 6 Sep 2021 13:18:53 +0200 > Subject: [PATCH] s390/block/dcssblk: add error handling support for add_disk() > > Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > --- > drivers/s390/block/dcssblk.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Thanks Gerald! FWIW: Acked-by: Heiko Carstens <hca@linux.ibm.com> ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 06, 2021 at 10:15:40AM +0100, Christoph Hellwig wrote:
> On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote:
> > Hmm, this is a more or less dead device driver, and I'm wondering if
> > we shouldn't remove it instead. But anyway, your patch is not correct:
>
> I'm all for removing it. I think we need to do a little more spring
> cleaning on unmaintained and likely to be unused block drivers.
Yes, we'll remove it. I'll schedule it even for this merge
window. Should be away with -rc1.
On Mon, Sep 06, 2021 at 10:15:40AM +0100, Christoph Hellwig wrote: > On Fri, Sep 03, 2021 at 04:06:15PM +0200, Heiko Carstens wrote: > > Hmm, this is a more or less dead device driver, and I'm wondering if > > we shouldn't remove it instead. But anyway, your patch is not correct: > > I'm all for removing it. I think we need to do a little more spring > cleaning on unmaintained and likely to be unused block drivers. Yes, we'll remove it. I'll schedule it even for this merge window. Should be away with -rc1. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, 2 Sept 2021 at 19:41, Luis Chamberlain <mcgrof@kernel.org> wrote: > > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Contrary to the typical removal which delays the put_disk() > until later, since we are failing on a probe we immediately > put the disk on failure from add_disk by using > blk_cleanup_disk(). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Queued for v5.16 on the temporary devel branch, thanks! Kind regards Uffe > --- > drivers/memstick/core/ms_block.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c > index 4a4573fa7b0f..86c626933c1a 100644 > --- a/drivers/memstick/core/ms_block.c > +++ b/drivers/memstick/core/ms_block.c > @@ -2156,10 +2156,14 @@ static int msb_init_disk(struct memstick_dev *card) > set_disk_ro(msb->disk, 1); > > msb_start(card); > - device_add_disk(&card->dev, msb->disk, NULL); > + rc = device_add_disk(&card->dev, msb->disk, NULL); > + if (rc) > + goto out_cleanup_disk; > dbg("Disk added"); > return 0; > > +out_cleanup_disk: > + blk_cleanup_disk(msb->disk); > out_free_tag_set: > blk_mq_free_tag_set(&msb->tag_set); > out_release_id: > -- > 2.30.2 >
On Thu, 2 Sept 2021 at 19:41, Luis Chamberlain <mcgrof@kernel.org> wrote: > > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Contrary to the typical removal which delays the put_disk() > until later, since we are failing on a probe we immediately > put the disk on failure from add_disk by using > blk_cleanup_disk(). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Queued for v5.16 on the temporary devel branch, thanks! Kind regards Uffe > --- > drivers/memstick/core/ms_block.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c > index 4a4573fa7b0f..86c626933c1a 100644 > --- a/drivers/memstick/core/ms_block.c > +++ b/drivers/memstick/core/ms_block.c > @@ -2156,10 +2156,14 @@ static int msb_init_disk(struct memstick_dev *card) > set_disk_ro(msb->disk, 1); > > msb_start(card); > - device_add_disk(&card->dev, msb->disk, NULL); > + rc = device_add_disk(&card->dev, msb->disk, NULL); > + if (rc) > + goto out_cleanup_disk; > dbg("Disk added"); > return 0; > > +out_cleanup_disk: > + blk_cleanup_disk(msb->disk); > out_free_tag_set: > blk_mq_free_tag_set(&msb->tag_set); > out_release_id: > -- > 2.30.2 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Thu, 2 Sept 2021 at 19:41, Luis Chamberlain <mcgrof@kernel.org> wrote: > > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Contrary to the typical removal which delays the put_disk() > until later, since we are failing on a probe we immediately > put the disk on failure from add_disk by using > blk_cleanup_disk(). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Queued for v5.16 on the temporary devel branch, thanks! Kind regards Uffe > --- > drivers/memstick/core/mspro_block.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c > index 22778d0e24f5..c0450397b673 100644 > --- a/drivers/memstick/core/mspro_block.c > +++ b/drivers/memstick/core/mspro_block.c > @@ -1239,10 +1239,14 @@ static int mspro_block_init_disk(struct memstick_dev *card) > set_capacity(msb->disk, capacity); > dev_dbg(&card->dev, "capacity set %ld\n", capacity); > > - device_add_disk(&card->dev, msb->disk, NULL); > + rc = device_add_disk(&card->dev, msb->disk, NULL); > + if (rc) > + goto out_cleanup_disk; > msb->active = 1; > return 0; > > +out_cleanup_disk: > + blk_cleanup_disk(msb->disk); > out_free_tag_set: > blk_mq_free_tag_set(&msb->tag_set); > out_release_id: > -- > 2.30.2 >
On Thu, 2 Sept 2021 at 19:41, Luis Chamberlain <mcgrof@kernel.org> wrote: > > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Contrary to the typical removal which delays the put_disk() > until later, since we are failing on a probe we immediately > put the disk on failure from add_disk by using > blk_cleanup_disk(). > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Queued for v5.16 on the temporary devel branch, thanks! Kind regards Uffe > --- > drivers/memstick/core/mspro_block.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c > index 22778d0e24f5..c0450397b673 100644 > --- a/drivers/memstick/core/mspro_block.c > +++ b/drivers/memstick/core/mspro_block.c > @@ -1239,10 +1239,14 @@ static int mspro_block_init_disk(struct memstick_dev *card) > set_capacity(msb->disk, capacity); > dev_dbg(&card->dev, "capacity set %ld\n", capacity); > > - device_add_disk(&card->dev, msb->disk, NULL); > + rc = device_add_disk(&card->dev, msb->disk, NULL); > + if (rc) > + goto out_cleanup_disk; > msb->active = 1; > return 0; > > +out_cleanup_disk: > + blk_cleanup_disk(msb->disk); > out_free_tag_set: > blk_mq_free_tag_set(&msb->tag_set); > out_release_id: > -- > 2.30.2 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 02/09/2021 19:41, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/dasd_genhd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > index fa966e0db6ca..ba07022283bc 100644 > --- a/drivers/s390/block/dasd_genhd.c > +++ b/drivers/s390/block/dasd_genhd.c > @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > { > struct gendisk *gdp; > struct dasd_device *base; > - int len; > + int len, rc; > > /* Make sure the minor for this device exists. */ > base = block->base; > @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) > dasd_add_link_to_gendisk(gdp, base); > block->gdp = gdp; > set_capacity(block->gdp, 0); > - device_add_disk(&base->cdev->dev, block->gdp, NULL); > + > + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > + if (rc) > + return rc; > + I think, just like with some of the other changes, there is some cleanup required before returning. I'll prepare a patch and come back to you. > return 0; > } > >
On 02/09/2021 19:41, Luis Chamberlain wrote: > We never checked for errors on add_disk() as this function > returned void. Now that this is fixed, use the shiny new > error handling. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/s390/block/dasd_genhd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > index fa966e0db6ca..ba07022283bc 100644 > --- a/drivers/s390/block/dasd_genhd.c > +++ b/drivers/s390/block/dasd_genhd.c > @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > { > struct gendisk *gdp; > struct dasd_device *base; > - int len; > + int len, rc; > > /* Make sure the minor for this device exists. */ > base = block->base; > @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) > dasd_add_link_to_gendisk(gdp, base); > block->gdp = gdp; > set_capacity(block->gdp, 0); > - device_add_disk(&base->cdev->dev, block->gdp, NULL); > + > + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > + if (rc) > + return rc; > + I think, just like with some of the other changes, there is some cleanup required before returning. I'll prepare a patch and come back to you. > return 0; > } > > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote: > I think, just like with some of the other changes, there is some > cleanup required before returning. I'll prepare a patch and > come back to you. If you are touching the dasd probe path anyway, can you look insto switching to use blk_mq_alloc_disk as well? Right now dasd allocates the request_queue and gendisk separately in different stages of initialization, but unlike SCSI which needs to probe using just the request_queue I can't find a good reason for that. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote:
> I think, just like with some of the other changes, there is some
> cleanup required before returning. I'll prepare a patch and
> come back to you.
If you are touching the dasd probe path anyway, can you look insto
switching to use blk_mq_alloc_disk as well? Right now dasd allocates
the request_queue and gendisk separately in different stages of
initialization, but unlike SCSI which needs to probe using just the
request_queue I can't find a good reason for that.
On 13/09/2021 10:42, Christoph Hellwig wrote:
> On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote:
>> I think, just like with some of the other changes, there is some
>> cleanup required before returning. I'll prepare a patch and
>> come back to you.
>
> If you are touching the dasd probe path anyway, can you look insto
> switching to use blk_mq_alloc_disk as well? Right now dasd allocates
> the request_queue and gendisk separately in different stages of
> initialization, but unlike SCSI which needs to probe using just the
> request_queue I can't find a good reason for that.
>
Thanks for the hint. We'll be working on it separately though, as
it seems to require a bit more effort from a first glance.
We'll send a separate patch (or patchset) soon.
regards,
Jan
On 13/09/2021 10:42, Christoph Hellwig wrote: > On Mon, Sep 13, 2021 at 10:17:48AM +0200, Jan H??ppner wrote: >> I think, just like with some of the other changes, there is some >> cleanup required before returning. I'll prepare a patch and >> come back to you. > > If you are touching the dasd probe path anyway, can you look insto > switching to use blk_mq_alloc_disk as well? Right now dasd allocates > the request_queue and gendisk separately in different stages of > initialization, but unlike SCSI which needs to probe using just the > request_queue I can't find a good reason for that. > Thanks for the hint. We'll be working on it separately though, as it seems to require a bit more effort from a first glance. We'll send a separate patch (or patchset) soon. regards, Jan ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 13/09/2021 10:17, Jan Höppner wrote: > On 02/09/2021 19:41, Luis Chamberlain wrote: >> We never checked for errors on add_disk() as this function >> returned void. Now that this is fixed, use the shiny new >> error handling. >> >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >> --- >> drivers/s390/block/dasd_genhd.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >> index fa966e0db6ca..ba07022283bc 100644 >> --- a/drivers/s390/block/dasd_genhd.c >> +++ b/drivers/s390/block/dasd_genhd.c >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> { >> struct gendisk *gdp; >> struct dasd_device *base; >> - int len; >> + int len, rc; >> >> /* Make sure the minor for this device exists. */ >> base = block->base; >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> dasd_add_link_to_gendisk(gdp, base); >> block->gdp = gdp; >> set_capacity(block->gdp, 0); >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + if (rc) >> + return rc; >> + > > I think, just like with some of the other changes, there is some > cleanup required before returning. I'll prepare a patch and > come back to you. > It's actually just one call that is required. The patch should look like this: diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..80673dbfb1f9 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) { + dasd_gendisk_free(block); + return rc; + } + return 0; } regards, Jan
On 13/09/2021 10:17, Jan Höppner wrote: > On 02/09/2021 19:41, Luis Chamberlain wrote: >> We never checked for errors on add_disk() as this function >> returned void. Now that this is fixed, use the shiny new >> error handling. >> >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >> --- >> drivers/s390/block/dasd_genhd.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >> index fa966e0db6ca..ba07022283bc 100644 >> --- a/drivers/s390/block/dasd_genhd.c >> +++ b/drivers/s390/block/dasd_genhd.c >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> { >> struct gendisk *gdp; >> struct dasd_device *base; >> - int len; >> + int len, rc; >> >> /* Make sure the minor for this device exists. */ >> base = block->base; >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> dasd_add_link_to_gendisk(gdp, base); >> block->gdp = gdp; >> set_capacity(block->gdp, 0); >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + if (rc) >> + return rc; >> + > > I think, just like with some of the other changes, there is some > cleanup required before returning. I'll prepare a patch and > come back to you. > It's actually just one call that is required. The patch should look like this: diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c index fa966e0db6ca..80673dbfb1f9 100644 --- a/drivers/s390/block/dasd_genhd.c +++ b/drivers/s390/block/dasd_genhd.c @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) { struct gendisk *gdp; struct dasd_device *base; - int len; + int len, rc; /* Make sure the minor for this device exists. */ base = block->base; @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) dasd_add_link_to_gendisk(gdp, base); block->gdp = gdp; set_capacity(block->gdp, 0); - device_add_disk(&base->cdev->dev, block->gdp, NULL); + + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); + if (rc) { + dasd_gendisk_free(block); + return rc; + } + return 0; } regards, Jan ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote:
> On 13/09/2021 10:17, Jan Höppner wrote:
> > On 02/09/2021 19:41, Luis Chamberlain wrote:
> >> We never checked for errors on add_disk() as this function
> >> returned void. Now that this is fixed, use the shiny new
> >> error handling.
> >>
> >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> >> ---
> >> drivers/s390/block/dasd_genhd.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
> >> index fa966e0db6ca..ba07022283bc 100644
> >> --- a/drivers/s390/block/dasd_genhd.c
> >> +++ b/drivers/s390/block/dasd_genhd.c
> >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block)
> >> {
> >> struct gendisk *gdp;
> >> struct dasd_device *base;
> >> - int len;
> >> + int len, rc;
> >>
> >> /* Make sure the minor for this device exists. */
> >> base = block->base;
> >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block)
> >> dasd_add_link_to_gendisk(gdp, base);
> >> block->gdp = gdp;
> >> set_capacity(block->gdp, 0);
> >> - device_add_disk(&base->cdev->dev, block->gdp, NULL);
> >> +
> >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL);
> >> + if (rc)
> >> + return rc;
> >> +
> >
> > I think, just like with some of the other changes, there is some
> > cleanup required before returning. I'll prepare a patch and
> > come back to you.
> >
>
> It's actually just one call that is required. The patch should
> look like this:
>
> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
> index fa966e0db6ca..80673dbfb1f9 100644
> --- a/drivers/s390/block/dasd_genhd.c
> +++ b/drivers/s390/block/dasd_genhd.c
> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block)
> {
> struct gendisk *gdp;
> struct dasd_device *base;
> - int len;
> + int len, rc;
>
> /* Make sure the minor for this device exists. */
> base = block->base;
> @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block)
> dasd_add_link_to_gendisk(gdp, base);
> block->gdp = gdp;
> set_capacity(block->gdp, 0);
> - device_add_disk(&base->cdev->dev, block->gdp, NULL);
> +
> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL);
> + if (rc) {
> + dasd_gendisk_free(block);
> + return rc;
> + }
> +
Thanks!
Would you like to to fold this fix into my patch and resend eventually?
Or will you send a replacement?
Luis
On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote: > On 13/09/2021 10:17, Jan Höppner wrote: > > On 02/09/2021 19:41, Luis Chamberlain wrote: > >> We never checked for errors on add_disk() as this function > >> returned void. Now that this is fixed, use the shiny new > >> error handling. > >> > >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > >> --- > >> drivers/s390/block/dasd_genhd.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > >> index fa966e0db6ca..ba07022283bc 100644 > >> --- a/drivers/s390/block/dasd_genhd.c > >> +++ b/drivers/s390/block/dasd_genhd.c > >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > >> { > >> struct gendisk *gdp; > >> struct dasd_device *base; > >> - int len; > >> + int len, rc; > >> > >> /* Make sure the minor for this device exists. */ > >> base = block->base; > >> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) > >> dasd_add_link_to_gendisk(gdp, base); > >> block->gdp = gdp; > >> set_capacity(block->gdp, 0); > >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); > >> + > >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > >> + if (rc) > >> + return rc; > >> + > > > > I think, just like with some of the other changes, there is some > > cleanup required before returning. I'll prepare a patch and > > come back to you. > > > > It's actually just one call that is required. The patch should > look like this: > > diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c > index fa966e0db6ca..80673dbfb1f9 100644 > --- a/drivers/s390/block/dasd_genhd.c > +++ b/drivers/s390/block/dasd_genhd.c > @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) > { > struct gendisk *gdp; > struct dasd_device *base; > - int len; > + int len, rc; > > /* Make sure the minor for this device exists. */ > base = block->base; > @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) > dasd_add_link_to_gendisk(gdp, base); > block->gdp = gdp; > set_capacity(block->gdp, 0); > - device_add_disk(&base->cdev->dev, block->gdp, NULL); > + > + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); > + if (rc) { > + dasd_gendisk_free(block); > + return rc; > + } > + Thanks! Would you like to to fold this fix into my patch and resend eventually? Or will you send a replacement? Luis ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote:
> On Fri, 3 Sep 2021 18:46:26 -0700
> Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote:
> > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote:
> > > > We never checked for errors on add_disk() as this function
> > > > returned void. Now that this is fixed, use the shiny new
> > > > error handling.
> > > >
> > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > ---
> > > > drivers/s390/block/dcssblk.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> > > > index 5be3d1c39a78..b0fd5009a12e 100644
> > > > --- a/drivers/s390/block/dcssblk.c
> > > > +++ b/drivers/s390/block/dcssblk.c
> > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
> > > > }
> > > >
> > > > get_device(&dev_info->dev);
> > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > > > + if (rc)
> > > > + goto put_dev;
> > >
> > > This looks not correct to me. We seem to have now in case of an error:
> > >
> > > - reference count imbalance (= memory leak)
> > > - dax cleanup is missing
> >
> > Care to provide an alternative?
>
> See patch below:
Thanks! Will you queue this up on your end or do would you
prefer for me to roll this into my tree and eventually resend
with the rest?
Luis
On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote: > On Fri, 3 Sep 2021 18:46:26 -0700 > Luis Chamberlain <mcgrof@kernel.org> wrote: > > > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote: > > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > > > > We never checked for errors on add_disk() as this function > > > > returned void. Now that this is fixed, use the shiny new > > > > error handling. > > > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > > > --- > > > > drivers/s390/block/dcssblk.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > > > index 5be3d1c39a78..b0fd5009a12e 100644 > > > > --- a/drivers/s390/block/dcssblk.c > > > > +++ b/drivers/s390/block/dcssblk.c > > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > > > } > > > > > > > > get_device(&dev_info->dev); > > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > > + if (rc) > > > > + goto put_dev; > > > > > > This looks not correct to me. We seem to have now in case of an error: > > > > > > - reference count imbalance (= memory leak) > > > - dax cleanup is missing > > > > Care to provide an alternative? > > See patch below: Thanks! Will you queue this up on your end or do would you prefer for me to roll this into my tree and eventually resend with the rest? Luis ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 13/09/2021 18:51, Luis Chamberlain wrote:
> On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote:
>> On 13/09/2021 10:17, Jan Höppner wrote:
>>> On 02/09/2021 19:41, Luis Chamberlain wrote:
>>>> We never checked for errors on add_disk() as this function
>>>> returned void. Now that this is fixed, use the shiny new
>>>> error handling.
>>>>
>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>>> ---
>>>> drivers/s390/block/dasd_genhd.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
>>>> index fa966e0db6ca..ba07022283bc 100644
>>>> --- a/drivers/s390/block/dasd_genhd.c
>>>> +++ b/drivers/s390/block/dasd_genhd.c
>>>> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block)
>>>> {
>>>> struct gendisk *gdp;
>>>> struct dasd_device *base;
>>>> - int len;
>>>> + int len, rc;
>>>>
>>>> /* Make sure the minor for this device exists. */
>>>> base = block->base;
>>>> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block)
>>>> dasd_add_link_to_gendisk(gdp, base);
>>>> block->gdp = gdp;
>>>> set_capacity(block->gdp, 0);
>>>> - device_add_disk(&base->cdev->dev, block->gdp, NULL);
>>>> +
>>>> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>
>>> I think, just like with some of the other changes, there is some
>>> cleanup required before returning. I'll prepare a patch and
>>> come back to you.
>>>
>>
>> It's actually just one call that is required. The patch should
>> look like this:
>>
>> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
>> index fa966e0db6ca..80673dbfb1f9 100644
>> --- a/drivers/s390/block/dasd_genhd.c
>> +++ b/drivers/s390/block/dasd_genhd.c
>> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block)
>> {
>> struct gendisk *gdp;
>> struct dasd_device *base;
>> - int len;
>> + int len, rc;
>>
>> /* Make sure the minor for this device exists. */
>> base = block->base;
>> @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block)
>> dasd_add_link_to_gendisk(gdp, base);
>> block->gdp = gdp;
>> set_capacity(block->gdp, 0);
>> - device_add_disk(&base->cdev->dev, block->gdp, NULL);
>> +
>> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL);
>> + if (rc) {
>> + dasd_gendisk_free(block);
>> + return rc;
>> + }
>> +
>
> Thanks!
>
> Would you like to to fold this fix into my patch and resend eventually?
> Or will you send a replacement?
>
> Luis
>
I'd be fine with you just taking the changes for your patchset.
Once you've resent the whole patchset I'll review it and send
the usual ack or r-b.
regards,
Jan
On 13/09/2021 18:51, Luis Chamberlain wrote: > On Mon, Sep 13, 2021 at 02:19:38PM +0200, Jan Höppner wrote: >> On 13/09/2021 10:17, Jan Höppner wrote: >>> On 02/09/2021 19:41, Luis Chamberlain wrote: >>>> We never checked for errors on add_disk() as this function >>>> returned void. Now that this is fixed, use the shiny new >>>> error handling. >>>> >>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >>>> --- >>>> drivers/s390/block/dasd_genhd.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >>>> index fa966e0db6ca..ba07022283bc 100644 >>>> --- a/drivers/s390/block/dasd_genhd.c >>>> +++ b/drivers/s390/block/dasd_genhd.c >>>> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >>>> { >>>> struct gendisk *gdp; >>>> struct dasd_device *base; >>>> - int len; >>>> + int len, rc; >>>> >>>> /* Make sure the minor for this device exists. */ >>>> base = block->base; >>>> @@ -79,7 +79,11 @@ int dasd_gendisk_alloc(struct dasd_block *block) >>>> dasd_add_link_to_gendisk(gdp, base); >>>> block->gdp = gdp; >>>> set_capacity(block->gdp, 0); >>>> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >>>> + >>>> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >>>> + if (rc) >>>> + return rc; >>>> + >>> >>> I think, just like with some of the other changes, there is some >>> cleanup required before returning. I'll prepare a patch and >>> come back to you. >>> >> >> It's actually just one call that is required. The patch should >> look like this: >> >> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c >> index fa966e0db6ca..80673dbfb1f9 100644 >> --- a/drivers/s390/block/dasd_genhd.c >> +++ b/drivers/s390/block/dasd_genhd.c >> @@ -33,7 +33,7 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> { >> struct gendisk *gdp; >> struct dasd_device *base; >> - int len; >> + int len, rc; >> >> /* Make sure the minor for this device exists. */ >> base = block->base; >> @@ -79,7 +79,13 @@ int dasd_gendisk_alloc(struct dasd_block *block) >> dasd_add_link_to_gendisk(gdp, base); >> block->gdp = gdp; >> set_capacity(block->gdp, 0); >> - device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + >> + rc = device_add_disk(&base->cdev->dev, block->gdp, NULL); >> + if (rc) { >> + dasd_gendisk_free(block); >> + return rc; >> + } >> + > > Thanks! > > Would you like to to fold this fix into my patch and resend eventually? > Or will you send a replacement? > > Luis > I'd be fine with you just taking the changes for your patchset. Once you've resent the whole patchset I'll review it and send the usual ack or r-b. regards, Jan ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Sep 13, 2021 at 09:53:14AM -0700, Luis Chamberlain wrote:
> On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote:
> > On Fri, 3 Sep 2021 18:46:26 -0700
> > Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote:
> > > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote:
> > > > > We never checked for errors on add_disk() as this function
> > > > > returned void. Now that this is fixed, use the shiny new
> > > > > error handling.
> > > > >
> > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > ---
> > > > > drivers/s390/block/dcssblk.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> > > > > index 5be3d1c39a78..b0fd5009a12e 100644
> > > > > --- a/drivers/s390/block/dcssblk.c
> > > > > +++ b/drivers/s390/block/dcssblk.c
> > > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char
> > > > > }
> > > > >
> > > > > get_device(&dev_info->dev);
> > > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL);
> > > > > + if (rc)
> > > > > + goto put_dev;
> > > >
> > > > This looks not correct to me. We seem to have now in case of an error:
> > > >
> > > > - reference count imbalance (= memory leak)
> > > > - dax cleanup is missing
> > >
> > > Care to provide an alternative?
> >
> > See patch below:
>
> Thanks! Will you queue this up on your end or do would you
> prefer for me to roll this into my tree and eventually resend
> with the rest?
Please add the patch to your tree.
On Mon, Sep 13, 2021 at 09:53:14AM -0700, Luis Chamberlain wrote: > On Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote: > > On Fri, 3 Sep 2021 18:46:26 -0700 > > Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > On Fri, Sep 03, 2021 at 04:08:48PM +0200, Heiko Carstens wrote: > > > > On Thu, Sep 02, 2021 at 10:41:03AM -0700, Luis Chamberlain wrote: > > > > > We never checked for errors on add_disk() as this function > > > > > returned void. Now that this is fixed, use the shiny new > > > > > error handling. > > > > > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > > > > --- > > > > > drivers/s390/block/dcssblk.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > > > > index 5be3d1c39a78..b0fd5009a12e 100644 > > > > > --- a/drivers/s390/block/dcssblk.c > > > > > +++ b/drivers/s390/block/dcssblk.c > > > > > @@ -696,7 +696,9 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > > > > } > > > > > > > > > > get_device(&dev_info->dev); > > > > > - device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > > > + rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > > > > + if (rc) > > > > > + goto put_dev; > > > > > > > > This looks not correct to me. We seem to have now in case of an error: > > > > > > > > - reference count imbalance (= memory leak) > > > > - dax cleanup is missing > > > > > > Care to provide an alternative? > > > > See patch below: > > Thanks! Will you queue this up on your end or do would you > prefer for me to roll this into my tree and eventually resend > with the rest? Please add the patch to your tree. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/