Skip to contentSkip to conversation

Take 2: cdev_unmap()

PATCH In review Jonathan Corbet 2 participants 2 messages Feb 5, 2004

Subsystems (5)
VFS SCSI Char and misc drivers SCSI SG driver +1 more subsystems
lore
Share & tools
lore

Jonathan Corbet

author
#0 lore raw

PATCH 1/1 Take 2: cdev_unmap()

I didn't get any responses to my last posting on cdev_unmap(); I take it that means nobody objects :)

To recap my argument: the current cdev implementation keeps an uncounted reference to every cdev in cdev_map. Creators of cdevs must know to call cdev_unmap() with the same arguments they passed to cdev_add() before releasing the device, or that reference will remain and will oops the kernel should user space attempt to open the (missing) device. It's an easy mistake to make, and, IMO, entirely unnecessary; the cdev code should be able to do its own bookkeeping.

So, does anybody have a reason why this shouldn't go in? Al, have I missed something?

[This is against 2.6.2-mm1].

jon

drivers/char/tty_io.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/char/tty_io.c 2.6.2-mm1/drivers/char/tty_io.c
@@ -2264,7 +2264,6 @@ int tty_unregister_driver(struct tty_dri
 	if (driver->refcount)
 		return -EBUSY;
 
-	cdev_unmap(MKDEV(driver->major, driver->minor_start), driver->num);
 	unregister_chrdev_region(MKDEV(driver->major, driver->minor_start),
 				driver->num);
drivers/ieee1394/amdtp.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/ieee1394/amdtp.c 2.6.2-mm1/drivers/ieee1394/amdtp.c
@@ -1308,7 +1308,6 @@ static void __exit amdtp_exit_module (vo
 
         hpsb_unregister_highlevel(&amdtp_highlevel);
 	devfs_remove("amdtp");
-	cdev_unmap(IEEE1394_AMDTP_DEV, 16);
 	cdev_del(&amdtp_cdev);
 
 	HPSB_INFO("Unloaded AMDTP driver");
drivers/ieee1394/dv1394.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/ieee1394/dv1394.c 2.6.2-mm1/drivers/ieee1394/dv1394.c
@@ -2609,7 +2609,6 @@ static void __exit dv1394_exit_module(vo
 	hpsb_unregister_protocol(&dv1394_driver);
 
 	hpsb_unregister_highlevel(&dv1394_highlevel);
-	cdev_unmap(IEEE1394_DV1394_DEV, 16);
 	cdev_del(&dv1394_cdev);
 	devfs_remove("ieee1394/dv");
 }
drivers/ieee1394/raw1394.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/ieee1394/raw1394.c 2.6.2-mm1/drivers/ieee1394/raw1394.c
@@ -2682,7 +2682,6 @@ static int __init init_raw1394(void)
 static void __exit cleanup_raw1394(void)
 {
 	hpsb_unregister_protocol(&raw1394_driver);
-	cdev_unmap(IEEE1394_RAW1394_DEV, 1);
 	cdev_del(&raw1394_cdev);
         devfs_remove(RAW1394_DEVICE_NAME);
         hpsb_unregister_highlevel(&raw1394_highlevel);
drivers/ieee1394/video1394.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/ieee1394/video1394.c 2.6.2-mm1/drivers/ieee1394/video1394.c
@@ -1447,7 +1447,6 @@ static void __exit video1394_exit_module
 	hpsb_unregister_highlevel(&video1394_highlevel);
 
 	devfs_remove(VIDEO1394_DRIVER_NAME);
-	cdev_unmap(IEEE1394_VIDEO1394_DEV, 16);
 	cdev_del(&video1394_cdev);
 
 	PRINT_G(KERN_INFO, "Removed " VIDEO1394_DRIVER_NAME " module");
drivers/scsi/sg.c +0 -1
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/scsi/sg.c 2.6.2-mm1/drivers/scsi/sg.c
@@ -1521,7 +1521,6 @@ sg_remove(struct class_device *cl_dev)
 	if (sdp) {
 		sysfs_remove_link(&scsidp->sdev_gendev.kobj, "generic");
 		class_simple_device_remove(MKDEV(SCSI_GENERIC_MAJOR, k));
-		cdev_unmap(MKDEV(SCSI_GENERIC_MAJOR, k), 1);
 		cdev_del(sdp->cdev);
 		sdp->cdev = NULL;
 		devfs_remove("%s/generic", scsidp->devfs_name);
drivers/scsi/st.c +0 -4
diff -urNp -X dontdiff 2.6.2-mm1-v/drivers/scsi/st.c 2.6.2-mm1/drivers/scsi/st.c
@@ -3946,8 +3946,6 @@ out_free_tape:
 				if (cdev == STm->cdevs[j])
 					cdev = NULL;
 				sysfs_remove_link(&STm->cdevs[j]->kobj, "device");
-				cdev_unmap(MKDEV(SCSI_TAPE_MAJOR,
-						 TAPE_MINOR(dev_num, mode, j)), 1);
 				cdev_del(STm->cdevs[j]);
 			}
 		}

@@ -3990,8 +3988,6 @@ static int st_remove(struct device *dev)
 				for (j=0; j < 2; j++) {
 					sysfs_remove_link(&tpnt->modes[mode].cdevs[j]->kobj,
 							  "device");
-					cdev_unmap(MKDEV(SCSI_TAPE_MAJOR,
-							 TAPE_MINOR(i, mode, j)), 1);
 					cdev_del(tpnt->modes[mode].cdevs[j]);
 					tpnt->modes[mode].cdevs[j] = NULL;
 				}
fs/char_dev.c +4 -3
diff -urNp -X dontdiff 2.6.2-mm1-v/fs/char_dev.c 2.6.2-mm1/fs/char_dev.c
@@ -240,7 +240,6 @@ void unregister_chrdev_region(dev_t from
 int unregister_chrdev(unsigned int major, const char *name)
 {
 	struct char_device_struct *cd;
-	cdev_unmap(MKDEV(major, 0), 256);
 	cd = __unregister_chrdev_region(major, 0, 256);
 	if (cd && cd->cdev)
 		cdev_del(cd->cdev);

@@ -347,16 +346,19 @@ int cdev_add(struct cdev *p, dev_t dev, 
 	err = kobj_map(cdev_map, dev, count, NULL, exact_match, exact_lock, p);
 	if (err)
 		kobject_del(&p->kobj);
+	p->dev = dev;
+	p->count = count;
 	return err;
 }
 
-void cdev_unmap(dev_t dev, unsigned count)
+static void cdev_unmap(dev_t dev, unsigned count)
 {
 	kobj_unmap(cdev_map, dev, count);
 }
 
 void cdev_del(struct cdev *p)
 {
+	cdev_unmap(p->dev, p->count);
 	kobject_del(&p->kobj);
 	kobject_put(&p->kobj);
 }

@@ -458,6 +460,5 @@ EXPORT_SYMBOL(cdev_get);
 EXPORT_SYMBOL(cdev_put);
 EXPORT_SYMBOL(cdev_del);
 EXPORT_SYMBOL(cdev_add);
-EXPORT_SYMBOL(cdev_unmap);
 EXPORT_SYMBOL(register_chrdev);
 EXPORT_SYMBOL(unregister_chrdev);
include/linux/cdev.h +2 -2
diff -urNp -X dontdiff 2.6.2-mm1-v/include/linux/cdev.h 2.6.2-mm1/include/linux/cdev.h
@@ -7,6 +7,8 @@ struct cdev {
 	struct module *owner;
 	struct file_operations *ops;
 	struct list_head list;
+	dev_t dev;
+	unsigned int count;
 };
 
 void cdev_init(struct cdev *, struct file_operations *);

@@ -21,8 +23,6 @@ int cdev_add(struct cdev *, dev_t, unsig
 
 void cdev_del(struct cdev *);
 
-void cdev_unmap(dev_t, unsigned);
-
 void cd_forget(struct inode *);
 
 #endif
#1 lore raw
Quoted 11 lines · Jonathan Corbet

I didn't get any responses to my last posting on cdev_unmap(); I take it that means nobody objects :)

To recap my argument: the current cdev implementation keeps an uncounted reference to every cdev in cdev_map. Creators of cdevs must know to call cdev_unmap() with the same arguments they passed to cdev_add() before releasing the device, or that reference will remain and will oops the kernel should user space attempt to open the (missing) device. It's an easy mistake to make, and, IMO, entirely unnecessary; the cdev code should be able to do its own bookkeeping.

Quoted 2 lines

So, does anybody have a reason why this shouldn't go in? Al, have I missed something?

I sure like it, it starts to make the cdev interface easier to use.

Al, unless you object, I'll add this to my driver bk tree which will get sucked into -mm and then send it off to Linus after a bit of testing.

thanks,

greg k-h