Skip to contentSkip to conversation

void unregister_blkdev - do WARN_ON failure

PATCH In review Akinobu Mita 2 participants 9 messages Jun 30, 2007

Subsystems (5)
VFS Block layer s390 Device mapper +1 more subsystems
lore
Share & tools
lore

Akinobu Mita

author
#0 lore raw

PATCH 1/4 void unregister_blkdev - do WARN_ON failure

When unregister_blkdev() has failed, something wrong happened. This patch adds WARN_ON to notify such badness.

Cc Jens Axboe <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 block/genhd.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
block/genhd.c +3 -2
Index: 2.6-mm/block/genhd.c
===================================================================
@@ -120,9 +120,10 @@ int unregister_blkdev(unsigned int major
 	for (n = &major_names[index]; *n; n = &(*n)->next)
 		if ((*n)->major == major)
 			break;
-	if (!*n || strcmp((*n)->name, name))
+	if (!*n || strcmp((*n)->name, name)) {
+		WARN_ON(1);
 		ret = -EINVAL;
-	else {
+	} else {
 		p = *n;
 		*n = p->next;
 	}
author
#1 lore raw

PATCH 2/4 void unregister_blkdev - delete redundant messages

No need to warn unregister_blkdev() failure by the callers. (The previous patch makes unregister_blkdev() print error message in error case)

Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/block/acsi.c         |    4 +---
 drivers/block/loop.c         |    3 +--
 drivers/block/z2ram.c        |    4 +---
 drivers/cdrom/cdu31a.c       |    4 +---
 drivers/cdrom/mcdx.c         |   11 ++---------
 drivers/cdrom/sbpcd.c        |    5 +----
 drivers/cdrom/sjcd.c         |    9 ++++-----
 drivers/cdrom/sonycd535.c    |    6 ++----
 drivers/md/dm.c              |    4 +---
 drivers/s390/block/dcssblk.c |    7 +------
 drivers/sbus/char/jsflash.c  |    3 +--
 11 files changed, 16 insertions(+), 44 deletions(-)
drivers/block/acsi.c +1 -3
Index: 2.6-mm/drivers/block/acsi.c
===================================================================
@@ -1775,9 +1775,7 @@ void cleanup_module(void)
 	del_timer( &acsi_timer );
 	blk_cleanup_queue(acsi_queue);
 	atari_stram_free( acsi_buffer );
-
-	if (unregister_blkdev( ACSI_MAJOR, "ad" ) != 0)
-		printk( KERN_ERR "acsi: cleanup_module failed\n");
+	unregister_blkdev(ACSI_MAJOR, "ad");
 
 	for (i = 0; i < NDevices; i++) {
 		del_gendisk(acsi_gendisk[i]);
drivers/block/z2ram.c +1 -3
Index: 2.6-mm/drivers/block/z2ram.c
===================================================================
@@ -371,9 +371,7 @@ static void __exit z2_exit(void)
 {
     int i, j;
     blk_unregister_region(MKDEV(Z2RAM_MAJOR, 0), 256);
-    if ( unregister_blkdev( Z2RAM_MAJOR, DEVICE_NAME ) != 0 )
-	printk( KERN_ERR DEVICE_NAME ": unregister of device failed\n");
-
+    unregister_blkdev(Z2RAM_MAJOR, DEVICE_NAME);
     del_gendisk(z2ram_gendisk);
     put_disk(z2ram_gendisk);
     blk_cleanup_queue(z2_queue);
drivers/s390/block/dcssblk.c +1 -6
Index: 2.6-mm/drivers/s390/block/dcssblk.c
===================================================================
@@ -747,14 +747,9 @@ dcssblk_check_params(void)
 static void __exit
 dcssblk_exit(void)
 {
-	int rc;
-
 	PRINT_DEBUG("DCSSBLOCK EXIT...\n");
 	s390_root_dev_unregister(dcssblk_root_dev);
-	rc = unregister_blkdev(dcssblk_major, DCSSBLK_NAME);
-	if (rc) {
-		PRINT_ERR("unregister_blkdev() failed!\n");
-	}
+	unregister_blkdev(dcssblk_major, DCSSBLK_NAME);
 	PRINT_DEBUG("...finished!\n");
 }
drivers/sbus/char/jsflash.c +1 -2
Index: 2.6-mm/drivers/sbus/char/jsflash.c
===================================================================
@@ -619,8 +619,7 @@ static void __exit jsflash_cleanup_modul
 	jsf0.busy = 0;
 
 	misc_deregister(&jsf_dev);
-	if (unregister_blkdev(JSFD_MAJOR, "jsfd") != 0)
-		printk("jsfd: cleanup_module failed\n");
+	unregister_blkdev(JSFD_MAJOR, "jsfd");
 	blk_cleanup_queue(jsf_queue);
 }
drivers/cdrom/mcdx.c +2 -9
Index: 2.6-mm/drivers/cdrom/mcdx.c
===================================================================
@@ -1050,13 +1050,7 @@ static void __exit mcdx_exit(void)
 		kfree(stuffp);
 	}
 
-	if (unregister_blkdev(MAJOR_NR, "mcdx") != 0) {
-		xwarn("cleanup() unregister_blkdev() failed\n");
-	}
-#if !MCDX_QUIET
-	else
-	xinfo("cleanup() succeeded\n");
-#endif
+	unregister_blkdev(MAJOR_NR, "mcdx");
 	blk_cleanup_queue(mcdx_queue);
 }
 

@@ -1240,8 +1234,7 @@ static int __init mcdx_init_drive(int dr
 		release_region(stuffp->wreg_data, MCDX_IO_SIZE);
 		kfree(stuffp);
 		put_disk(disk);
-		if (unregister_blkdev(MAJOR_NR, "mcdx") != 0)
-			xwarn("cleanup() unregister_blkdev() failed\n");
+		unregister_blkdev(MAJOR_NR, "mcdx");
 		blk_cleanup_queue(mcdx_queue);
 		return 2;
 	}
drivers/cdrom/sjcd.c +4 -5
Index: 2.6-mm/drivers/cdrom/sjcd.c
===================================================================
@@ -1792,9 +1792,9 @@ out2:
 out1:
 	blk_cleanup_queue(sjcd_queue);
 out0:
-	if ((unregister_blkdev(MAJOR_NR, "sjcd") == -EINVAL))
-		printk("SJCD: cannot unregister device.\n");
-	return (-EIO);
+	unregister_blkdev(MAJOR_NR, "sjcd");
+
+	return -EIO;
 }
 
 static void __exit sjcd_exit(void)

@@ -1803,8 +1803,7 @@ static void __exit sjcd_exit(void)
 	put_disk(sjcd_disk);
 	release_region(sjcd_base, 4);
 	blk_cleanup_queue(sjcd_queue);
-	if ((unregister_blkdev(MAJOR_NR, "sjcd") == -EINVAL))
-		printk("SJCD: cannot unregister device.\n");
+	unregister_blkdev(MAJOR_NR, "sjcd");
 	printk(KERN_INFO "SJCD: module: removed.\n");
 }
drivers/md/dm.c +1 -3
Index: 2.6-mm/drivers/md/dm.c
===================================================================
@@ -165,9 +165,7 @@ static void local_exit(void)
 {
 	kmem_cache_destroy(_tio_cache);
 	kmem_cache_destroy(_io_cache);
-
-	if (unregister_blkdev(_major, _name) < 0)
-		DMERR("unregister_blkdev failed");
+	unregister_blkdev(_major, _name);
 
 	_major = 0;
drivers/block/loop.c +1 -2
Index: 2.6-mm/drivers/block/loop.c
===================================================================
@@ -1536,8 +1536,7 @@ static void __exit loop_exit(void)
 		loop_del_one(lo);
 
 	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
-	if (unregister_blkdev(LOOP_MAJOR, "loop"))
-		printk(KERN_WARNING "loop: cannot unregister blkdev\n");
+	unregister_blkdev(LOOP_MAJOR, "loop");
 }
 
 module_init(loop_init);
drivers/cdrom/cdu31a.c +1 -3
Index: 2.6-mm/drivers/cdrom/cdu31a.c
===================================================================
@@ -3210,9 +3210,7 @@ errout0:
 	printk(KERN_ERR PFX "Unable to register with Uniform cdrom driver\n");
 	put_disk(disk);
 errout1:
-	if (unregister_blkdev(MAJOR_NR, "cdu31a")) {
-		printk(KERN_WARNING PFX "Can't unregister block device\n");
-	}
+	unregister_blkdev(MAJOR_NR, "cdu31a");
 errout2:
 	release_region(cdu31a_port, 4);
 errout3:
drivers/cdrom/sbpcd.c +1 -4
Index: 2.6-mm/drivers/cdrom/sbpcd.c
===================================================================
@@ -5830,10 +5830,7 @@ int __init sbpcd_init(void)
 		if (!p->sbp_buf) {
 			msg(DBG_INF,"data buffer (%d frames) not available.\n",
 				buffers);
-			if ((unregister_blkdev(MAJOR_NR, major_name) == -EINVAL))
-			{
-				printk("Can't unregister %s\n", major_name);
-			}
+			unregister_blkdev(MAJOR_NR, major_name);
 			release_region(CDo_command,4);
 			blk_cleanup_queue(sbpcd_queue);
 			return -EIO;
drivers/cdrom/sonycd535.c +2 -4
Index: 2.6-mm/drivers/cdrom/sonycd535.c
===================================================================
@@ -1675,10 +1675,8 @@ sony535_exit(void)
 	del_gendisk(cdu_disk);
 	put_disk(cdu_disk);
 	blk_cleanup_queue(sonycd535_queue);
-	if (unregister_blkdev(MAJOR_NR, CDU535_HANDLE) == -EINVAL)
-		printk("Uh oh, couldn't unregister " CDU535_HANDLE "\n");
-	else
-		printk(KERN_INFO CDU535_HANDLE " module released\n");
+	unregister_blkdev(MAJOR_NR, CDU535_HANDLE);
+	printk(KERN_INFO CDU535_HANDLE " module released\n");
 }
 
 module_init(sony535_init);
author
#2 lore raw

PATCH 2/4 void unregister_blkdev - delete redundant message

No need to warn unregister_blkdev() failure by caller. (The previous patch makes unregister_blkdev() print error message in error case)

Cc Grant Likely <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/block/xsysace.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/block/xsysace.c +1 -3
Index: 2.6-mm/drivers/block/xsysace.c
===================================================================
@@ -1157,9 +1157,7 @@ static void __exit ace_exit(void)
 {
 	pr_debug("Unregistering Xilinx SystemACE driver\n");
 	driver_unregister(&ace_driver);
-	if (unregister_blkdev(ace_major, "xsysace"))
-		printk(KERN_WARNING "systemace unregister_blkdev(%i) failed\n",
-		       ace_major);
+	unregister_blkdev(ace_major, "xsysace");
 }
 
 module_init(ace_init);
Acked-by on v1 2/4
#3 lore raw

Re: [PATCH 2/4 -mm] void unregister_blkdev - delete redundant message

Quoted 6 lines · Akinobu Mita

No need to warn unregister_blkdev() failure by caller. (The previous patch makes unregister_blkdev() print error message in error case)

Cc Grant Likely <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
Acked-by Grant Likely <[email protected]>
Quoted 21 lines
---
 drivers/block/xsysace.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: 2.6-mm/drivers/block/xsysace.c
===================================================================

drivers/block/xsysace.c +1 -3
@@ -1157,9 +1157,7 @@ static void __exit ace_exit(void)
 {
        pr_debug("Unregistering Xilinx SystemACE driver\n");
        driver_unregister(&ace_driver);
-       if (unregister_blkdev(ace_major, "xsysace"))
-               printk(KERN_WARNING "systemace unregister_blkdev(%i) failed\n",
-                      ace_major);
+       unregister_blkdev(ace_major, "xsysace");
 }
 
 module_init(ace_init);
Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
#4 lore raw

Re: [PATCH 2/4] void unregister_blkdev - delete redundant messages

In reply to Akinobu Mita
Quoted 3 lines · Akinobu Mita

No need to warn unregister_blkdev() failure by the callers. (The previous patch makes unregister_blkdev() print error message in error case)

Seeing as none of the callers check the error code; why does the function return anything at all?

g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
author
#5 lore raw

Re: [PATCH 2/4] void unregister_blkdev - delete redundant messages

2007/6/30, Grant Likely <[email protected]>:

Quoted 7 lines · Akinobu Mita
Quoted 3 lines

No need to warn unregister_blkdev() failure by the callers. (The previous patch makes unregister_blkdev() print error message in error case)

Seeing as none of the callers check the error code; why does the function return anything at all?

Right. This series of patches is trying to make it return void.

http://lkml.org/lkml/2007/6/29/320

#6 lore raw

Re: [PATCH 2/4] void unregister_blkdev - delete redundant messages

Quoted 6 lines · Akinobu Mita
Quoted 2 lines

Seeing as none of the callers check the error code; why does the function return anything at all?

Right. This series of patches is trying to make it return void.

http://lkml.org/lkml/2007/6/29/320

Heh, cool. I should do more research before I reply blindly. :-)

g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
author
#7 lore raw

PATCH 3/4 void unregister_blkdev - ignore the return value

In reply to Akinobu Mita

Some cdrom drivers stop destruct operations in module_exit() when unregister_blkdev() failure happens.

But it can't help to stop unloading module. So it will not be good error handling. Furthermore any other block drivers don't have such handling and there is no special reason that only those cdrom drivers have to do.

This patch removes the return value checks for unregister_blkdev().

This change will not hide the bugs. Because unregister_blkdev() prints error message on failures by the previous patch.

Cc Eberhard Moenkeberg <[email protected]>
Cc Oliver Raupach <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 drivers/cdrom/aztcd.c  |    5 +----
 drivers/cdrom/cdu31a.c |    6 +-----
 drivers/cdrom/cm206.c  |    5 +----
 drivers/cdrom/gscd.c   |    5 +----
 drivers/cdrom/optcd.c  |    5 +----
 drivers/cdrom/sbpcd.c  |    8 ++------
 6 files changed, 7 insertions(+), 27 deletions(-)
drivers/cdrom/optcd.c +1 -4
Index: 2.6-mm/drivers/cdrom/optcd.c
===================================================================
@@ -2089,10 +2089,7 @@ static void __exit optcd_exit(void)
 {
 	del_gendisk(optcd_disk);
 	put_disk(optcd_disk);
-	if (unregister_blkdev(MAJOR_NR, "optcd") == -EINVAL) {
-		printk(KERN_ERR "optcd: what's that: can't unregister\n");
-		return;
-	}
+	unregister_blkdev(MAJOR_NR, "optcd");
 	blk_cleanup_queue(opt_queue);
 	release_region(optcd_port, 4);
 	printk(KERN_INFO "optcd: module released.\n");
drivers/cdrom/sbpcd.c +2 -6
Index: 2.6-mm/drivers/cdrom/sbpcd.c
===================================================================
@@ -5885,12 +5885,8 @@ int __init sbpcd_init(void)
 static void sbpcd_exit(void)
 {
 	int j;
-	
-	if ((unregister_blkdev(MAJOR_NR, major_name) == -EINVAL))
-	{
-		msg(DBG_INF, "What's that: can't unregister %s.\n", major_name);
-		return;
-	}
+
+	unregister_blkdev(MAJOR_NR, major_name);
 	release_region(CDo_command,4);
 	blk_cleanup_queue(sbpcd_queue);
 	for (j=0;j<NR_SBPCD;j++)
drivers/cdrom/cdu31a.c +1 -5
Index: 2.6-mm/drivers/cdrom/cdu31a.c
===================================================================
@@ -3227,11 +3227,7 @@ static void __exit cdu31a_exit(void)
 				"cdrom driver\n");
 		return;
 	}
-	if ((unregister_blkdev(MAJOR_NR, "cdu31a") == -EINVAL)) {
-		printk(KERN_WARNING PFX "Can't unregister\n");
-		return;
-	}
-
+	unregister_blkdev(MAJOR_NR, "cdu31a");
 	blk_cleanup_queue(cdu31a_queue);
 
 	if (cdu31a_irq > 0)
drivers/cdrom/gscd.c +1 -4
Index: 2.6-mm/drivers/cdrom/gscd.c
===================================================================
@@ -882,10 +882,7 @@ static void __exit gscd_exit(void)
 
 	del_gendisk(gscd_disk);
 	put_disk(gscd_disk);
-	if ((unregister_blkdev(MAJOR_NR, "gscd") == -EINVAL)) {
-		printk("What's that: can't unregister GoldStar-module\n");
-		return;
-	}
+	unregister_blkdev(MAJOR_NR, "gscd");
 	blk_cleanup_queue(gscd_queue);
 	release_region(gscd_port, GSCD_IO_EXTENT);
 	printk(KERN_INFO "GoldStar-module released.\n");
drivers/cdrom/aztcd.c +1 -4
Index: 2.6-mm/drivers/cdrom/aztcd.c
===================================================================
@@ -1941,10 +1941,7 @@ static void __exit aztcd_exit(void)
 {
 	del_gendisk(azt_disk);
 	put_disk(azt_disk);
-	if ((unregister_blkdev(MAJOR_NR, "aztcd") == -EINVAL)) {
-		printk("What's that: can't unregister aztcd\n");
-		return;
-	}
+	unregister_blkdev(MAJOR_NR, "aztcd");
 	blk_cleanup_queue(azt_queue);
 	if ((azt_port == 0x1f0) || (azt_port == 0x170)) {
 		SWITCH_IDE_MASTER;
drivers/cdrom/cm206.c +1 -4
Index: 2.6-mm/drivers/cdrom/cm206.c
===================================================================
@@ -1548,10 +1548,7 @@ static void __exit cm206_exit(void)
 		printk("Can't unregister cdrom cm206\n");
 		return;
 	}
-	if (unregister_blkdev(MAJOR_NR, "cm206")) {
-		printk("Can't unregister major cm206\n");
-		return;
-	}
+	unregister_blkdev(MAJOR_NR, "cm206");
 	blk_cleanup_queue(cm206_queue);
 	free_irq(cm206_irq, NULL);
 	kfree(cd);
author
#8 lore raw

PATCH 4/4 void unregister_blkdev - make void

In reply to Akinobu Mita

Put WARN_ON and fixed all callers of unregister_blkdev(). Now we can make unregister_blkdev return void.

Cc Jens Axboe <[email protected]>
Signed-off-by Akinobu Mita <[email protected]>
---
 block/genhd.c      |    7 +------
 include/linux/fs.h |    2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)
block/genhd.c +1 -6
Index: 2.6-mm/block/genhd.c
===================================================================
@@ -108,13 +108,11 @@ out:
 
 EXPORT_SYMBOL(register_blkdev);
 
-/* todo: make void - error printk here */
-int unregister_blkdev(unsigned int major, const char *name)
+void unregister_blkdev(unsigned int major, const char *name)
 {
 	struct blk_major_name **n;
 	struct blk_major_name *p = NULL;
 	int index = major_to_index(major);
-	int ret = 0;
 
 	mutex_lock(&block_subsys_lock);
 	for (n = &major_names[index]; *n; n = &(*n)->next)

@@ -122,15 +120,12 @@ int unregister_blkdev(unsigned int major
 			break;
 	if (!*n || strcmp((*n)->name, name)) {
 		WARN_ON(1);
-		ret = -EINVAL;
 	} else {
 		p = *n;
 		*n = p->next;
 	}
 	mutex_unlock(&block_subsys_lock);
 	kfree(p);
-
-	return ret;
 }
 
 EXPORT_SYMBOL(unregister_blkdev);
include/linux/fs.h +1 -1
Index: 2.6-mm/include/linux/fs.h
===================================================================
@@ -1553,7 +1553,7 @@ extern void putname(const char *name);
 
 #ifdef CONFIG_BLOCK
 extern int register_blkdev(unsigned int, const char *);
-extern int unregister_blkdev(unsigned int, const char *);
+extern void unregister_blkdev(unsigned int, const char *);
 extern struct block_device *bdget(dev_t);
 extern void bd_set_size(struct block_device *, loff_t size);
 extern void bd_forget(struct inode *inode);