[PATCH 0/9] block: 5th batch of add_disk() error handling conversions

[PATCH 0/9] block: 5th batch of add_disk() error handling conversions

[Luis Chamberlain]

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

[PATCH 1/9] cdrom/gdrom: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 2/9] ms_block: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 3/9] mspro_block: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 4/9] rbd: add add_disk() error handling

[Luis Chamberlain]

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

[PATCH 5/9] mtd: add add_disk() error handling

[Luis Chamberlain]

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

[PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 8/9] s390/block/scm_blk: add error handling support for add_disk()

[Luis Chamberlain]

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

[PATCH 9/9] s390/block/xpram: add error handling support for add_disk()

[Luis Chamberlain]

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

Re: [PATCH 5/9] mtd: add add_disk() error handling

[Miquel Raynal]

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

Re: [PATCH 9/9] s390/block/xpram: add error handling support for add_disk()

[Heiko Carstens]

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.

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Heiko Carstens]

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

Re: [PATCH 8/9] s390/block/scm_blk: add error handling support for add_disk()

[Heiko Carstens]

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>

Re: [PATCH 9/9] s390/block/xpram: add error handling support for add_disk()

[Luis Chamberlain]

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

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Luis Chamberlain]

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

Re: [PATCH 9/9] s390/block/xpram: add error handling support for add_disk()

[Christoph Hellwig]

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.

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Gerald Schaefer]

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

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Heiko Carstens]

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>

Re: [PATCH 9/9] s390/block/xpram: add error handling support for add_disk()

[Heiko Carstens]

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.

Re: [PATCH 2/9] ms_block: add error handling support for add_disk()

[Ulf Hansson]

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
>

Re: [PATCH 3/9] mspro_block: add error handling support for add_disk()

[Ulf Hansson]

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
>

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Jan Höppner]

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;
>  }
>  
> 

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Christoph Hellwig]

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.

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Jan Höppner]

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

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Jan Höppner]

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

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Luis Chamberlain]

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

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Luis Chamberlain]

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

Re: [PATCH 6/9] s390/block/dasd_genhd: add error handling support for add_disk()

[Jan Höppner]

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

Re: [PATCH 7/9] s390/block/dcssblk: add error handling support for add_disk()

[Heiko Carstens]

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.