[PATCH v2 0/2] block: 7th -- last batch of add_disk() error handling conversions

[PATCH v2 0/2] block: 7th -- last batch of add_disk() error handling conversions

[Luis Chamberlain]

This is the 7th and last set of driver conversions for add_disk() error
handling. The entire set of pending changes can be found on my
20210927-for-axboe-add-disk-error-handling branch [0].

Changes on this v2:

  - rebased onto linux-next tag 20210927
  - I modified the drivers to be sure to treat an existing block device on
    probe as a non-issue, and expanded the documentation to explain why we
    want to driver's probe routine to behave this way.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210927-for-axboe-add-disk-error-handling

Luis Chamberlain (2):
  block: make __register_blkdev() return an error
  block: add __must_check for *add_disk*() callers

 block/bdev.c            |  5 ++++-
 block/genhd.c           | 27 ++++++++++++++++++---------
 drivers/block/ataflop.c | 20 +++++++++++++++-----
 drivers/block/brd.c     |  7 +++++--
 drivers/block/floppy.c  | 14 ++++++++++----
 drivers/block/loop.c    | 11 ++++++++---
 drivers/md/md.c         | 12 +++++++++---
 drivers/scsi/sd.c       |  3 ++-
 include/linux/genhd.h   | 10 +++++-----
 9 files changed, 76 insertions(+), 33 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

This makes __register_blkdev() return an error, and also changes the
probe() call to return an error as well.

We expand documentation for the probe call to ensure that if the block
device already exists we don't return on error on that condition. We do
this as otherwise we loose ability to handle concurrent requests if the
block device already existed.

Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/bdev.c            |  5 ++++-
 block/genhd.c           | 21 +++++++++++++++------
 drivers/block/ataflop.c | 20 +++++++++++++++-----
 drivers/block/brd.c     |  7 +++++--
 drivers/block/floppy.c  | 14 ++++++++++----
 drivers/block/loop.c    | 11 ++++++++---
 drivers/md/md.c         | 12 +++++++++---
 drivers/scsi/sd.c       |  3 ++-
 include/linux/genhd.h   |  4 ++--
 9 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index cf2780cb44a7..a9b39c3d13d7 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -733,10 +733,13 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 {
 	struct block_device *bdev;
 	struct inode *inode;
+	int ret;
 
 	inode = ilookup(blockdev_superblock, dev);
 	if (!inode) {
-		blk_request_module(dev);
+		ret = blk_request_module(dev);
+		if (ret)
+			return NULL;
 		inode = ilookup(blockdev_superblock, dev);
 		if (!inode)
 			return NULL;
diff --git a/block/genhd.c b/block/genhd.c
index 7b6e5e1cf956..a5a41628aa59 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -180,7 +180,7 @@ static struct blk_major_name {
 	struct blk_major_name *next;
 	int major;
 	char name[16];
-	void (*probe)(dev_t devt);
+	int (*probe)(dev_t devt);
 } *major_names[BLKDEV_MAJOR_HASH_SIZE];
 static DEFINE_MUTEX(major_names_lock);
 static DEFINE_SPINLOCK(major_names_spinlock);
@@ -210,7 +210,13 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
  * @major: the requested major device number [1..BLKDEV_MAJOR_MAX-1]. If
  *         @major = 0, try to allocate any unused major number.
  * @name: the name of the new block device as a zero terminated string
- * @probe: allback that is called on access to any minor number of @major
+ * @probe: callback that is called on access to any minor number of @major.
+ * 	This will return 0 if the block device is already present or was
+ * 	not present and it succcessfully added a new one. If the block device
+ * 	was not already present but it was a valid request an error reflecting
+ * 	the reason why adding the block device is returned. An error should not
+ * 	be returned if the block device already exists as otherwise concurrent
+ * 	requests to open an existing block device would fail.
  *
  * The @name must be unique within the system.
  *
@@ -228,7 +234,7 @@ void blkdev_show(struct seq_file *seqf, off_t offset)
  * Use register_blkdev instead for any new code.
  */
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt))
+		int (*probe)(dev_t devt))
 {
 	struct blk_major_name **n, *p;
 	int index, ret = 0;
@@ -626,17 +632,18 @@ static ssize_t disk_badblocks_store(struct device *dev,
 	return badblocks_store(disk->bb, page, len, 0);
 }
 
-void blk_request_module(dev_t devt)
+int blk_request_module(dev_t devt)
 {
 	unsigned int major = MAJOR(devt);
 	struct blk_major_name **n;
+	int err;
 
 	mutex_lock(&major_names_lock);
 	for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) {
 		if ((*n)->major == major && (*n)->probe) {
-			(*n)->probe(devt);
+			err = (*n)->probe(devt);
 			mutex_unlock(&major_names_lock);
-			return;
+			return err;
 		}
 	}
 	mutex_unlock(&major_names_lock);
@@ -644,6 +651,8 @@ void blk_request_module(dev_t devt)
 	if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0)
 		/* Make old-style 2.4 aliases work */
 		request_module("block-major-%d", MAJOR(devt));
+
+	return 0;
 }
 
 /*
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 5dc9b3d32415..be0627345b21 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
 
 static DEFINE_MUTEX(ataflop_probe_lock);
 
-static void ataflop_probe(dev_t dev)
+static int ataflop_probe(dev_t dev)
 {
 	int drive = MINOR(dev) & 3;
 	int type  = MINOR(dev) >> 2;
+	int err = 0;
 
 	if (type)
 		type--;
 
-	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
-		return;
+	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	mutex_lock(&ataflop_probe_lock);
 	if (!unit[drive].disk[type]) {
-		if (ataflop_alloc_disk(drive, type) == 0) {
-			add_disk(unit[drive].disk[type]);
+		err = ataflop_alloc_disk(drive, type);
+		if (err == 0) {
+			err = add_disk(unit[drive].disk[type]);
+			if (err)
+				blk_cleanup_disk(unit[drive].disk[type]);
 			unit[drive].registered[type] = true;
 		}
 	}
 	mutex_unlock(&ataflop_probe_lock);
+
+out:
+	return err;
 }
 
 static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index c2bf4946f4e3..82a93044de95 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -426,10 +426,11 @@ static int brd_alloc(int i)
 	return err;
 }
 
-static void brd_probe(dev_t dev)
+static int brd_probe(dev_t dev)
 {
 	int i = MINOR(dev) / max_part;
 	struct brd_device *brd;
+	int err = 0;
 
 	mutex_lock(&brd_devices_mutex);
 	list_for_each_entry(brd, &brd_devices, brd_list) {
@@ -437,9 +438,11 @@ static void brd_probe(dev_t dev)
 			goto out_unlock;
 	}
 
-	brd_alloc(i);
+	err = brd_alloc(i);
 out_unlock:
 	mutex_unlock(&brd_devices_mutex);
+
+	return err;
 }
 
 static void brd_del_one(struct brd_device *brd)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 0434f28742e7..95a1c8ef62f7 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
 
 static DEFINE_MUTEX(floppy_probe_lock);
 
-static void floppy_probe(dev_t dev)
+static int floppy_probe(dev_t dev)
 {
 	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
 	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
+	int err = 0;
 
 	if (drive >= N_DRIVE || !floppy_available(drive) ||
 	    type >= ARRAY_SIZE(floppy_type))
-		return;
+		return -EINVAL;
 
 	mutex_lock(&floppy_probe_lock);
 	if (!disks[drive][type]) {
-		if (floppy_alloc_disk(drive, type) == 0)
-			add_disk(disks[drive][type]);
+		if (floppy_alloc_disk(drive, type) == 0) {
+			err = add_disk(disks[drive][type]);
+			if (err)
+				blk_cleanup_disk(disks[drive][type]);
+		}
 	}
 	mutex_unlock(&floppy_probe_lock);
+
+	return err;
 }
 
 static int __init do_floppy_init(void)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 00ee365ed5e0..5ffb1900baa9 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2433,13 +2433,18 @@ static void loop_remove(struct loop_device *lo)
 	kfree(lo);
 }
 
-static void loop_probe(dev_t dev)
+static int loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
+	int ret;
 
 	if (max_loop && idx >= max_loop)
-		return;
-	loop_add(idx);
+		return -EINVAL;
+	ret = loop_add(idx);
+	if (ret == -EEXIST)
+		return 0;
+
+	return ret;
 }
 
 static int loop_control_remove(int idx)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 6bd5ad3c30b4..a68b47462ad7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5729,12 +5729,18 @@ static int md_alloc(dev_t dev, char *name)
 	return error;
 }
 
-static void md_probe(dev_t dev)
+static int md_probe(dev_t dev)
 {
+	int error = 0;
+
 	if (MAJOR(dev) == MD_MAJOR && MINOR(dev) >= 512)
-		return;
+		return -EINVAL;
 	if (create_on_open)
-		md_alloc(dev, NULL);
+		error = md_alloc(dev, NULL);
+	if (error == -EEXIST)
+		return 0;
+
+	return error;
 }
 
 static int add_named_array(const char *val, const struct kernel_param *kp)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3a101ad4d16e..691dae32f02b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -631,8 +631,9 @@ static struct scsi_driver sd_template = {
  * Don't request a new module, as that could deadlock in multipath
  * environment.
  */
-static void sd_default_probe(dev_t devt)
+static int sd_default_probe(dev_t devt)
 {
+	return 0;
 }
 
 /*
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index c68d83c87f83..5828ecda5c49 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -281,7 +281,7 @@ struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
 void blk_cleanup_disk(struct gendisk *disk);
 
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt));
+		int (*probe)(dev_t devt));
 #define register_blkdev(major, name) \
 	__register_blkdev(major, name, NULL)
 void unregister_blkdev(unsigned int major, const char *name);
@@ -317,7 +317,7 @@ static inline int bd_register_pending_holders(struct gendisk *disk)
 dev_t part_devt(struct gendisk *disk, u8 partno);
 void inc_diskseq(struct gendisk *disk);
 dev_t blk_lookup_devt(const char *name, int partno);
-void blk_request_module(dev_t devt);
+int blk_request_module(dev_t devt);
 #ifdef CONFIG_BLOCK
 void printk_all_partitions(void);
 #else /* CONFIG_BLOCK */
-- 
2.30.2

[PATCH v2 2/2] block: add __must_check for *add_disk*() callers

[Luis Chamberlain]

Now that we have done a spring cleaning on all drivers and added
error checking / handling, let's keep it that way and ensure
no new drivers fail to stick with it.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c         | 6 +++---
 include/linux/genhd.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index a5a41628aa59..44c630e3377a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -394,8 +394,8 @@ static void disk_scan_partitions(struct gendisk *disk)
  * This function registers the partitioning information in @disk
  * with the kernel.
  */
-int device_add_disk(struct device *parent, struct gendisk *disk,
-		     const struct attribute_group **groups)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups)
 
 {
 	struct device *ddev = disk_to_dev(disk);
@@ -540,7 +540,7 @@ int device_add_disk(struct device *parent, struct gendisk *disk,
 out_free_ext_minor:
 	if (disk->major == BLOCK_EXT_MAJOR)
 		blk_free_ext_minor(disk->first_minor);
-	return WARN_ON_ONCE(ret); /* keep until all callers handle errors */
+	return ret;
 }
 EXPORT_SYMBOL(device_add_disk);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 5828ecda5c49..8d78d36c424e 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -214,9 +214,9 @@ static inline dev_t disk_devt(struct gendisk *disk)
 void disk_uevent(struct gendisk *disk, enum kobject_action action);
 
 /* block/genhd.c */
-int device_add_disk(struct device *parent, struct gendisk *disk,
-		const struct attribute_group **groups);
-static inline int add_disk(struct gendisk *disk)
+int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups);
+static inline int __must_check add_disk(struct gendisk *disk)
 {
 	return device_add_disk(NULL, disk, NULL);
 }
-- 
2.30.2

Re: [PATCH v2 1/2] block: make __register_blkdev() return an error

[Tetsuo Handa]

On 2021/09/28 7:03, Luis Chamberlain wrote:
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index 5dc9b3d32415..be0627345b21 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
>  
>  static DEFINE_MUTEX(ataflop_probe_lock);
>  
> -static void ataflop_probe(dev_t dev)
> +static int ataflop_probe(dev_t dev)
>  {
>  	int drive = MINOR(dev) & 3;
>  	int type  = MINOR(dev) >> 2;
> +	int err = 0;
>  
>  	if (type)
>  		type--;
>  
> -	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> -		return;
> +	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	mutex_lock(&ataflop_probe_lock);
>  	if (!unit[drive].disk[type]) {
> -		if (ataflop_alloc_disk(drive, type) == 0) {
> -			add_disk(unit[drive].disk[type]);
> +		err = ataflop_alloc_disk(drive, type);
> +		if (err == 0) {
> +			err = add_disk(unit[drive].disk[type]);
> +			if (err)
> +				blk_cleanup_disk(unit[drive].disk[type]);
>  			unit[drive].registered[type] = true;

Why setting registered to true despite add_disk() failed?
del_gendisk() without successful add_disk() sounds wrong.

Don't we need to undo ataflop_alloc_disk() because it sets
unit[drive].disk[type] to non-NULL ?

>  		}
>  	}
>  	mutex_unlock(&ataflop_probe_lock);
> +
> +out:
> +	return err;
>  }
>  
>  static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)



> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index c2bf4946f4e3..82a93044de95 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -426,10 +426,11 @@ static int brd_alloc(int i)
>  	return err;
>  }
>  
> -static void brd_probe(dev_t dev)
> +static int brd_probe(dev_t dev)
>  {
>  	int i = MINOR(dev) / max_part;
>  	struct brd_device *brd;
> +	int err = 0;
>  
>  	mutex_lock(&brd_devices_mutex);
>  	list_for_each_entry(brd, &brd_devices, brd_list) {
> @@ -437,9 +438,11 @@ static void brd_probe(dev_t dev)
>  			goto out_unlock;
>  	}
>  
> -	brd_alloc(i);
> +	err = brd_alloc(i);
>  out_unlock:
>  	mutex_unlock(&brd_devices_mutex);
> +
> +	return err;
>  }
>  
>  static void brd_del_one(struct brd_device *brd)

https://lkml.kernel.org/r/e205f13d-18ff-a49c-0988-7de6ea5ff823@i-love.sakura.ne.jp
will require this part to be updated.


> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 0434f28742e7..95a1c8ef62f7 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
>  
>  static DEFINE_MUTEX(floppy_probe_lock);
>  
> -static void floppy_probe(dev_t dev)
> +static int floppy_probe(dev_t dev)
>  {
>  	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
>  	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
> +	int err = 0;
>  
>  	if (drive >= N_DRIVE || !floppy_available(drive) ||
>  	    type >= ARRAY_SIZE(floppy_type))
> -		return;
> +		return -EINVAL;
>  
>  	mutex_lock(&floppy_probe_lock);
>  	if (!disks[drive][type]) {
> -		if (floppy_alloc_disk(drive, type) == 0)
> -			add_disk(disks[drive][type]);
> +		if (floppy_alloc_disk(drive, type) == 0) {
> +			err = add_disk(disks[drive][type]);
> +			if (err)
> +				blk_cleanup_disk(disks[drive][type]);

This makes future floppy_probe() no-op once add_disk() failed (or maybe a bad
thing happens somewhere else), for disks[drive][type] was set to non-NULL by
floppy_alloc_disk() but blk_cleanup_disk() does not reset it to NULL.

According to floppy_module_exit() which tries to cleanup it, implementing
undo might be complicated...

> +		}
>  	}
>  	mutex_unlock(&floppy_probe_lock);
> +
> +	return err;
>  }
>  
>  static int __init do_floppy_init(void)

Re: [PATCH v2 1/2] block: make __register_blkdev() return an error

[Finn Thain]

On Mon, 27 Sep 2021, Luis Chamberlain wrote:

> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index 5dc9b3d32415..be0627345b21 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
>  
>  static DEFINE_MUTEX(ataflop_probe_lock);
>  
> -static void ataflop_probe(dev_t dev)
> +static int ataflop_probe(dev_t dev)
>  {
>  	int drive = MINOR(dev) & 3;
>  	int type  = MINOR(dev) >> 2;
> +	int err = 0;
>  
>  	if (type)
>  		type--;
>  
> -	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> -		return;
> +	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	mutex_lock(&ataflop_probe_lock);
>  	if (!unit[drive].disk[type]) {
> -		if (ataflop_alloc_disk(drive, type) == 0) {
> -			add_disk(unit[drive].disk[type]);
> +		err = ataflop_alloc_disk(drive, type);
> +		if (err == 0) {
> +			err = add_disk(unit[drive].disk[type]);
> +			if (err)
> +				blk_cleanup_disk(unit[drive].disk[type]);
>  			unit[drive].registered[type] = true;
>  		}
>  	}
>  	mutex_unlock(&ataflop_probe_lock);
> +
> +out:
> +	return err;
>  }
>  
>  static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)

I think the change to ataflop_probe() would be more clear without adding 
an 'out' label, like your change to floppy.c:

> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 0434f28742e7..95a1c8ef62f7 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
>  
>  static DEFINE_MUTEX(floppy_probe_lock);
>  
> -static void floppy_probe(dev_t dev)
> +static int floppy_probe(dev_t dev)
>  {
>  	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
>  	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
> +	int err = 0;
>  
>  	if (drive >= N_DRIVE || !floppy_available(drive) ||
>  	    type >= ARRAY_SIZE(floppy_type))
> -		return;
> +		return -EINVAL;
>  
>  	mutex_lock(&floppy_probe_lock);
>  	if (!disks[drive][type]) {
> -		if (floppy_alloc_disk(drive, type) == 0)
> -			add_disk(disks[drive][type]);
> +		if (floppy_alloc_disk(drive, type) == 0) {
> +			err = add_disk(disks[drive][type]);
> +			if (err)
> +				blk_cleanup_disk(disks[drive][type]);
> +		}
>  	}
>  	mutex_unlock(&floppy_probe_lock);
> +
> +	return err;
>  }
>  
>  static int __init do_floppy_init(void)

In floppy_probe(), I think you should return the potential error result 
from floppy_alloc_disk(), like you did in ataflop.c.

Re: [PATCH v2 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Tue, Sep 28, 2021 at 09:19:47AM +0900, Tetsuo Handa wrote:
> On 2021/09/28 7:03, Luis Chamberlain wrote:
> > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> > index 5dc9b3d32415..be0627345b21 100644
> > --- a/drivers/block/ataflop.c
> > +++ b/drivers/block/ataflop.c
> > @@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
> >  
> >  static DEFINE_MUTEX(ataflop_probe_lock);
> >  
> > -static void ataflop_probe(dev_t dev)
> > +static int ataflop_probe(dev_t dev)
> >  {
> >  	int drive = MINOR(dev) & 3;
> >  	int type  = MINOR(dev) >> 2;
> > +	int err = 0;
> >  
> >  	if (type)
> >  		type--;
> >  
> > -	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> > -		return;
> > +	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >  	mutex_lock(&ataflop_probe_lock);
> >  	if (!unit[drive].disk[type]) {
> > -		if (ataflop_alloc_disk(drive, type) == 0) {
> > -			add_disk(unit[drive].disk[type]);
> > +		err = ataflop_alloc_disk(drive, type);
> > +		if (err == 0) {
> > +			err = add_disk(unit[drive].disk[type]);
> > +			if (err)
> > +				blk_cleanup_disk(unit[drive].disk[type]);
> >  			unit[drive].registered[type] = true;
> 
> Why setting registered to true despite add_disk() failed?
> del_gendisk() without successful add_disk() sounds wrong.

That was a mistake, fixed.

> Don't we need to undo ataflop_alloc_disk() because it sets
> unit[drive].disk[type] to non-NULL ?

ataflop_alloc_disk() just calls blk_mq_alloc_disk() for its
allocation, and so blk_cleanup_disk() does that for us. Please
let me know if I missed anything.

> > diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> > index c2bf4946f4e3..82a93044de95 100644
> > --- a/drivers/block/brd.c
> > +++ b/drivers/block/brd.c
> > @@ -426,10 +426,11 @@ static int brd_alloc(int i)
> >  	return err;
> >  }
> >  
> > -static void brd_probe(dev_t dev)
> > +static int brd_probe(dev_t dev)
> >  {
> >  	int i = MINOR(dev) / max_part;
> >  	struct brd_device *brd;
> > +	int err = 0;
> >  
> >  	mutex_lock(&brd_devices_mutex);
> >  	list_for_each_entry(brd, &brd_devices, brd_list) {
> > @@ -437,9 +438,11 @@ static void brd_probe(dev_t dev)
> >  			goto out_unlock;
> >  	}
> >  
> > -	brd_alloc(i);
> > +	err = brd_alloc(i);
> >  out_unlock:
> >  	mutex_unlock(&brd_devices_mutex);
> > +
> > +	return err;
> >  }
> >  
> >  static void brd_del_one(struct brd_device *brd)
> 
> https://lkml.kernel.org/r/e205f13d-18ff-a49c-0988-7de6ea5ff823@i-love.sakura.ne.jp
> will require this part to be updated.

Indeed, rebased, thanks for the heads up!

> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index 0434f28742e7..95a1c8ef62f7 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
> >  
> >  static DEFINE_MUTEX(floppy_probe_lock);
> >  
> > -static void floppy_probe(dev_t dev)
> > +static int floppy_probe(dev_t dev)
> >  {
> >  	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
> >  	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
> > +	int err = 0;
> >  
> >  	if (drive >= N_DRIVE || !floppy_available(drive) ||
> >  	    type >= ARRAY_SIZE(floppy_type))
> > -		return;
> > +		return -EINVAL;
> >  
> >  	mutex_lock(&floppy_probe_lock);
> >  	if (!disks[drive][type]) {
> > -		if (floppy_alloc_disk(drive, type) == 0)
> > -			add_disk(disks[drive][type]);
> > +		if (floppy_alloc_disk(drive, type) == 0) {
> > +			err = add_disk(disks[drive][type]);
> > +			if (err)
> > +				blk_cleanup_disk(disks[drive][type]);
> 
> This makes future floppy_probe() no-op once add_disk() failed (or maybe a bad
> thing happens somewhere else), for disks[drive][type] was set to non-NULL by
> floppy_alloc_disk() but blk_cleanup_disk() does not reset it to NULL.

Thanks!

I think just setting disks[drive][type] = NULL after the
blk_cleanup_disk() fixes that issue.

> According to floppy_module_exit() which tries to cleanup it, implementing
> undo might be complicated...

I can't see what would be missing from just setting disks[drive][type] = NULL.
Can you clarify?

  Luis

Re: [PATCH v2 1/2] block: make __register_blkdev() return an error

[Luis Chamberlain]

On Tue, Sep 28, 2021 at 10:57:18AM +1000, Finn Thain wrote:
> On Mon, 27 Sep 2021, Luis Chamberlain wrote:
> 
> > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> > index 5dc9b3d32415..be0627345b21 100644
> > --- a/drivers/block/ataflop.c
> > +++ b/drivers/block/ataflop.c
> > @@ -1989,24 +1989,34 @@ static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
> >  
> >  static DEFINE_MUTEX(ataflop_probe_lock);
> >  
> > -static void ataflop_probe(dev_t dev)
> > +static int ataflop_probe(dev_t dev)
> >  {
> >  	int drive = MINOR(dev) & 3;
> >  	int type  = MINOR(dev) >> 2;
> > +	int err = 0;
> >  
> >  	if (type)
> >  		type--;
> >  
> > -	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
> > -		return;
> > +	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS) {
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> >  	mutex_lock(&ataflop_probe_lock);
> >  	if (!unit[drive].disk[type]) {
> > -		if (ataflop_alloc_disk(drive, type) == 0) {
> > -			add_disk(unit[drive].disk[type]);
> > +		err = ataflop_alloc_disk(drive, type);
> > +		if (err == 0) {
> > +			err = add_disk(unit[drive].disk[type]);
> > +			if (err)
> > +				blk_cleanup_disk(unit[drive].disk[type]);
> >  			unit[drive].registered[type] = true;
> >  		}
> >  	}
> >  	mutex_unlock(&ataflop_probe_lock);
> > +
> > +out:
> > +	return err;
> >  }
> >  
> >  static void atari_cleanup_floppy_disk(struct atari_floppy_struct *fs)
> 
> I think the change to ataflop_probe() would be more clear without adding 
> an 'out' label, like your change to floppy.c:

Good point! Fixed.

> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index 0434f28742e7..95a1c8ef62f7 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4517,21 +4517,27 @@ static int floppy_alloc_disk(unsigned int drive, unsigned int type)
> >  
> >  static DEFINE_MUTEX(floppy_probe_lock);
> >  
> > -static void floppy_probe(dev_t dev)
> > +static int floppy_probe(dev_t dev)
> >  {
> >  	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
> >  	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
> > +	int err = 0;
> >  
> >  	if (drive >= N_DRIVE || !floppy_available(drive) ||
> >  	    type >= ARRAY_SIZE(floppy_type))
> > -		return;
> > +		return -EINVAL;
> >  
> >  	mutex_lock(&floppy_probe_lock);
> >  	if (!disks[drive][type]) {
> > -		if (floppy_alloc_disk(drive, type) == 0)
> > -			add_disk(disks[drive][type]);
> > +		if (floppy_alloc_disk(drive, type) == 0) {
> > +			err = add_disk(disks[drive][type]);
> > +			if (err)
> > +				blk_cleanup_disk(disks[drive][type]);
> > +		}
> >  	}
> >  	mutex_unlock(&floppy_probe_lock);
> > +
> > +	return err;
> >  }
> >  
> >  static int __init do_floppy_init(void)
> 
> In floppy_probe(), I think you should return the potential error result 
> from floppy_alloc_disk(), like you did in ataflop.c.

Indeed, thanks, fixed.

  Luis