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
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 <[email protected]>
---
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 <[email protected]>
---
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.
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 <[email protected]>
---
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.
Signed-off-by: Luis Chamberlain <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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.
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 <[email protected]>
---
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
Hi Luis,
Luis Chamberlain <[email protected]> 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 <[email protected]>
Acked-by: Miquel Raynal <[email protected]>
Thanks,
Miquèl
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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: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 <[email protected]>
> ---
> drivers/s390/block/scm_blk.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Acked-by: Heiko Carstens <[email protected]>
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 <[email protected]>
> ---
> 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 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 <[email protected]>
> > ---
> > 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: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 <[email protected]>
> > ---
> > 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: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, 3 Sep 2021 18:46:26 -0700
Luis Chamberlain <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
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 <[email protected]>
---
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 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 01:43:46PM +0200, Gerald Schaefer wrote:
> On Fri, 3 Sep 2021 18:46:26 -0700
> Luis Chamberlain <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> drivers/s390/block/dcssblk.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
Thanks Gerald! FWIW:
Acked-by: Heiko Carstens <[email protected]>
On Thu, 2 Sept 2021 at 19:41, Luis Chamberlain <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
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 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 <[email protected]>
> ---
> 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 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 Mon, Sep 06, 2021 at 01:43:46PM +0200, Gerald Schaefer wrote:
> On Fri, 3 Sep 2021 18:46:26 -0700
> Luis Chamberlain <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 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 <[email protected]>
>> ---
>> 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: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 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 <[email protected]>
> >> ---
> >> 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 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 <[email protected]>
>>>> ---
>>>> 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 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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.