cdrom: remove ifdef CONFIG_SYSCTL

PATCH In review Akinobu Mita 5 participants 13 messages Mar 22, 2008 – Mar 25, 2008

Subsystems (3)

Documentation Library code Uniform cdrom driver

lore

Share & tools

lore

Akinobu Mita

author

2008-03-22 03:18 UTC #0 lore raw

PATCH 1/5 cdrom: remove ifdef CONFIG_SYSCTL

This patch removes #ifdef for CONFIG_SYSCTL by defining empty cdrom_sysctl_register and cdrom_sysctl_unregister when CONFIG_SYSCTL is not defined.

Signed-off-by Akinobu Mita <[email protected]>

Cc Jens Axboe <[email protected]>

---
 drivers/cdrom/cdrom.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

drivers/cdrom/cdrom.c +12 -8

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
@@ -360,9 +360,8 @@ static int cdrom_mrw_exit(struct cdrom_d
 
 static int cdrom_get_disc_info(struct cdrom_device_info *cdi, disc_information *di);
 
-#ifdef CONFIG_SYSCTL
 static void cdrom_sysctl_register(void);
-#endif /* CONFIG_SYSCTL */ 
+
 static struct cdrom_device_info *topCdromPtr;
 
 static int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,

@@ -398,9 +397,7 @@ int register_cdrom(struct cdrom_device_i
 	if (!banner_printed) {
 		printk(KERN_INFO "Uniform CD-ROM driver " REVISION "\n");
 		banner_printed = 1;
-#ifdef CONFIG_SYSCTL
 		cdrom_sysctl_register();
-#endif /* CONFIG_SYSCTL */ 
 	}
 
 	ENSURE(drive_status, CDC_DRIVE_STATUS );

@@ -3571,22 +3568,29 @@ static void cdrom_sysctl_unregister(void
 		unregister_sysctl_table(cdrom_sysctl_header);
 }
 
+#else /* CONFIG_SYSCTL */
+
+static void cdrom_sysctl_register(void)
+{
+}
+
+static void cdrom_sysctl_unregister(void)
+{
+}
+
 #endif /* CONFIG_SYSCTL */
 
 static int __init cdrom_init(void)
 {
-#ifdef CONFIG_SYSCTL
 	cdrom_sysctl_register();
-#endif
+
 	return 0;
 }
 
 static void __exit cdrom_exit(void)
 {
 	printk(KERN_INFO "Uniform CD-ROM driver unloaded\n");
-#ifdef CONFIG_SYSCTL
 	cdrom_sysctl_unregister();
-#endif
 }
 
 module_init(cdrom_init);

Akinobu Mita

author

2008-03-22 03:19 UTC #1 lore raw

PATCH 2/5 cdrom: cleanup hardcoded error-code

This patch eliminates hardcoded return value of register_cdrom().

It also changes the return value to -EINVAL.
It is more appropriate than -2 (-ENOENT) because it is only happen invalid usage of register_cdrom() by broken cdrom driver.

Signed-off-by Akinobu Mita <[email protected]>

Cc Jens Axboe <[email protected]>

---
 drivers/cdrom/cdrom.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cdrom/cdrom.c +1 -1

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
@@ -393,7 +393,7 @@ int register_cdrom(struct cdrom_device_i
 	cdinfo(CD_OPEN, "entering register_cdrom\n"); 
 
 	if (cdo->open == NULL || cdo->release == NULL)
-		return -2;
+		return -EINVAL;
 	if (!banner_printed) {
 		printk(KERN_INFO "Uniform CD-ROM driver " REVISION "\n");
 		banner_printed = 1;

Akinobu Mita

author

2008-03-22 03:20 UTC #2 lore raw

PATCH 3/5 cdrom: protect cdrom_device_info list by mutex

This patch protects the list of cdrom_device_info by cdrom_mutex when the file in /proc/sys/dev/cdrom/ is written.

Signed-off-by Akinobu Mita <[email protected]>

Cc Jens Axboe <[email protected]>

---
 drivers/cdrom/cdrom.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/cdrom/cdrom.c +2 -0

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
@@ -3427,6 +3427,7 @@ static void cdrom_update_settings(void)
 {
 	struct cdrom_device_info *cdi;
 
+	mutex_lock(&cdrom_mutex);
 	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next) {
 		if (autoclose && CDROM_CAN(CDC_CLOSE_TRAY))
 			cdi->options |= CDO_AUTO_CLOSE;

@@ -3445,6 +3446,7 @@ static void cdrom_update_settings(void)
 		else
 			cdi->options &= ~CDO_CHECK_TYPE;
 	}
+	mutex_unlock(&cdrom_mutex);
 }
 
 static int cdrom_sysctl_handler(ctl_table *ctl, int write, struct file * filp,

Akinobu Mita

author

2008-03-22 03:21 UTC #3 lore raw

PATCH 4/5 cdrom: use list_head for cdrom_device_info list

Use list_head for cdrom_device_info list instead of opencoded singly list handling.

Signed-off-by Akinobu Mita <[email protected]>

Cc Jens Axboe <[email protected]>

---
 drivers/cdrom/cdrom.c |   29 ++++++-----------------------
 include/linux/cdrom.h |    3 ++-
 2 files changed, 8 insertions(+), 24 deletions(-)

drivers/cdrom/cdrom.c +6 ~~-23~~

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
@@ -362,7 +362,7 @@ static int cdrom_get_disc_info(struct cd
 
 static void cdrom_sysctl_register(void);
 
-static struct cdrom_device_info *topCdromPtr;
+static LIST_HEAD(cdrom_list);
 
 static int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
 				      struct packet_command *cgc)

@@ -436,35 +436,18 @@ int register_cdrom(struct cdrom_device_i
 
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
 	mutex_lock(&cdrom_mutex);
-	cdi->next = topCdromPtr; 	
-	topCdromPtr = cdi;
+	list_add(&cdi->list, &cdrom_list);
 	mutex_unlock(&cdrom_mutex);
 	return 0;
 }
 #undef ENSURE
 
-int unregister_cdrom(struct cdrom_device_info *unreg)
+int unregister_cdrom(struct cdrom_device_info *cdi)
 {
-	struct cdrom_device_info *cdi, *prev;
 	cdinfo(CD_OPEN, "entering unregister_cdrom\n"); 
 
-	prev = NULL;
 	mutex_lock(&cdrom_mutex);
-	cdi = topCdromPtr;
-	while (cdi && cdi != unreg) {
-		prev = cdi;
-		cdi = cdi->next;
-	}
-
-	if (cdi == NULL) {
-		mutex_unlock(&cdrom_mutex);
-		return -2;
-	}
-	if (prev)
-		prev->next = cdi->next;
-	else
-		topCdromPtr = cdi->next;
-
+	list_del(&cdi->list);
 	mutex_unlock(&cdrom_mutex);
 
 	if (cdi->exit)

@@ -3306,7 +3289,7 @@ static int cdrom_print_info(const char *
 
 	*pos += ret;
 
-	for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
+	list_for_each_entry(cdi, &cdrom_list, list) {
 		switch (option) {
 		case CTL_NAME:
 			ret = scnprintf(info + *pos, max_size - *pos,

@@ -3428,7 +3411,7 @@ static void cdrom_update_settings(void)
 	struct cdrom_device_info *cdi;
 
 	mutex_lock(&cdrom_mutex);
-	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next) {
+	list_for_each_entry(cdi, &cdrom_list, list) {
 		if (autoclose && CDROM_CAN(CDC_CLOSE_TRAY))
 			cdi->options |= CDO_AUTO_CLOSE;
 		else if (!autoclose)

include/linux/cdrom.h +2 -1

Index: 2.6-git/include/linux/cdrom.h
===================================================================
@@ -910,6 +910,7 @@ struct mode_page_header {
 #ifdef __KERNEL__
 #include <linux/fs.h>		/* not really needed, later.. */
 #include <linux/device.h>
+#include <linux/list.h>
 
 struct packet_command
 {

@@ -934,7 +935,7 @@ struct packet_command
 /* Uniform cdrom data structures for cdrom.c */
 struct cdrom_device_info {
 	struct cdrom_device_ops  *ops;  /* link to device_ops */
-	struct cdrom_device_info *next; /* next device_info for this major */
+	struct list_head list;		/* linked list of all device_info */
 	struct gendisk *disk;		/* matching block layer disk */
 	void *handle;		        /* driver-dependent data */
 /* specifications */

Akinobu Mita

author

2008-03-22 03:22 UTC #4 lore raw

PATCH 5/5 cdrom: make unregister_cdrom() return void

Now unregister_cdrom() always returns 0.
Make it return void and update all callers that check the return value.

Signed-off-by Akinobu Mita <[email protected]>

Cc Adrian McMenamin <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Jens Axboe <[email protected]>

---
 Documentation/cdrom/cdrom-standard.tex |    2 +-
 drivers/cdrom/cdrom.c                  |    3 +--
 drivers/cdrom/gdrom.c                  |    4 +++-
 drivers/cdrom/viocd.c                  |    5 +----
 drivers/ide/ide-cd.c                   |    5 ++---
 include/linux/cdrom.h                  |    2 +-
 6 files changed, 9 insertions(+), 12 deletions(-)

drivers/cdrom/cdrom.c +1 -2

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
@@ -442,7 +442,7 @@ int register_cdrom(struct cdrom_device_i
 }
 #undef ENSURE
 
-int unregister_cdrom(struct cdrom_device_info *cdi)
+void unregister_cdrom(struct cdrom_device_info *cdi)
 {
 	cdinfo(CD_OPEN, "entering unregister_cdrom\n");

@@ -455,7 +455,6 @@ int unregister_cdrom(struct cdrom_device
 
 	cdi->ops->n_minors--;
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" unregistered\n", cdi->name);
-	return 0;
 }
 
 int cdrom_get_media_event(struct cdrom_device_info *cdi,

drivers/cdrom/gdrom.c +3 -1

Index: 2.6-git/drivers/cdrom/gdrom.c
===================================================================
@@ -827,7 +827,9 @@ static int __devexit remove_gdrom(struct
 	del_gendisk(gd.disk);
 	if (gdrom_major)
 		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
-	return unregister_cdrom(gd.cd_info);
+	unregister_cdrom(gd.cd_info);
+
+	return 0;
 }
 
 static struct platform_driver gdrom_driver = {

drivers/cdrom/viocd.c +1 -4

Index: 2.6-git/drivers/cdrom/viocd.c
===================================================================
@@ -650,10 +650,7 @@ static int viocd_remove(struct vio_dev *
 {
 	struct disk_info *d = &viocd_diskinfo[vdev->unit_address];
 
-	if (unregister_cdrom(&d->viocd_info) != 0)
-		printk(VIOCD_KERN_WARNING
-				"Cannot unregister viocd CD-ROM %s!\n",
-				d->viocd_info.name);
+	unregister_cdrom(&d->viocd_info);
 	del_gendisk(d->viocd_disk);
 	blk_cleanup_queue(d->viocd_disk->queue);
 	put_disk(d->viocd_disk);

include/linux/cdrom.h +1 -1

Index: 2.6-git/include/linux/cdrom.h
===================================================================
@@ -995,7 +995,7 @@ extern int cdrom_ioctl(struct file *file
 extern int cdrom_media_changed(struct cdrom_device_info *);
 
 extern int register_cdrom(struct cdrom_device_info *cdi);
-extern int unregister_cdrom(struct cdrom_device_info *cdi);
+extern void unregister_cdrom(struct cdrom_device_info *cdi);
 
 typedef struct {
     int data;

Documentation/cdrom/cdrom-standard.tex +1 -1

Index: 2.6-git/Documentation/cdrom/cdrom-standard.tex
===================================================================
@@ -777,7 +777,7 @@ Note that a driver must have one static 
 it may have as many structures $<device>_info$ as there are minor devices
 active. $Register_cdrom()$ builds a linked list from these. 
 
-\subsection{$Int\ unregister_cdrom(struct\ cdrom_device_info * cdi)$}
+\subsection{$Void\ unregister_cdrom(struct\ cdrom_device_info * cdi)$}
 
 Unregistering device $cdi$ with minor number $MINOR(cdi\to dev)$ removes
 the minor device from the list. If it was the last registered minor for

drivers/ide/ide-cd.c +2 -3

Index: 2.6-git/drivers/ide/ide-cd.c
===================================================================
@@ -2030,9 +2030,8 @@ static void ide_cd_release(struct kref *
 
 	kfree(info->buffer);
 	kfree(info->toc);
-	if (devinfo->handle == drive && unregister_cdrom(devinfo))
-		printk(KERN_ERR "%s: %s failed to unregister device from the cdrom "
-				"driver.\n", __FUNCTION__, drive->name);
+	if (devinfo->handle == drive)
+		unregister_cdrom(devinfo);
 	drive->dsc_overlap = 0;
 	drive->driver_data = NULL;
 	blk_queue_prep_rq(drive->queue, NULL);

Jens Axboe

2008-03-22 12:57 UTC #5 lore raw

Re: [PATCH 5/5] cdrom: make unregister_cdrom() return void

Quoted 2 lines · Akinobu Mita

Now unregister_cdrom() always returns 0.
Make it return void and update all callers that check the return value.

Patches 1-3 and 5 are fine, thanks. If you can rediff #4 with the list init part added, it's good to go.

Signature · 1 line

Jens Axboe

Borislav Petkov

Acked-by on v1 5/5

2008-03-22 15:46 UTC #6 lore raw

Re: [PATCH 5/5] cdrom: make unregister_cdrom() return void

In reply to Akinobu Mita

Quoted 7 lines · Akinobu Mita

Now unregister_cdrom() always returns 0.
Make it return void and update all callers that check the return value.
Signed-off-by Akinobu Mita <[email protected]>
Cc Adrian McMenamin <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Jens Axboe <[email protected]>

Quoted 8 lines

---
 Documentation/cdrom/cdrom-standard.tex |    2 +-
 drivers/cdrom/cdrom.c                  |    3 +--
 drivers/cdrom/gdrom.c                  |    4 +++-
 drivers/cdrom/viocd.c                  |    5 +----
 drivers/ide/ide-cd.c                   |    5 ++---
 include/linux/cdrom.h                  |    2 +-
 6 files changed, 9 insertions(+), 12 deletions(-)

Acked-by Borislav Petkov <[email protected]>

Adrian McMenamin

Acked-by on v1 5/5

2008-03-22 16:43 UTC #7 lore raw

Re: [PATCH 5/5] cdrom: make unregister_cdrom() return void

In reply to Akinobu Mita

Quoted 7 lines · Akinobu Mita

Now unregister_cdrom() always returns 0.
Make it return void and update all callers that check the return value.
Signed-off-by Akinobu Mita <[email protected]>
Cc Adrian McMenamin <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Jens Axboe <[email protected]>

Quoted 8 lines

---
 Documentation/cdrom/cdrom-standard.tex |    2 +-
 drivers/cdrom/cdrom.c                  |    3 +--
 drivers/cdrom/gdrom.c                  |    4 +++-
 drivers/cdrom/viocd.c                  |    5 +----
 drivers/ide/ide-cd.c                   |    5 ++---
 include/linux/cdrom.h                  |    2 +-
 6 files changed, 9 insertions(+), 12 deletions(-)

I'm not sure if I need to ack this - but in case anybody is waiting :)

Acked-by Adrian McMenamin <[email protected]>

Jens Axboe

2008-03-22 12:55 UTC #8 lore raw

Re: [PATCH 4/5] cdrom: use list_head for cdrom_device_info list

In reply to Akinobu Mita

Quoted 2 lines · Akinobu Mita

Use list_head for cdrom_device_info list instead of opencoded singly list handling.

Looks good, but you don't seem to be initializing ->list anywhere. Did you test this?

I'd suggest just adding an INIT_LIST_HEAD() before the list_add() in register_cdrom()

You also seem to remove the !cdi check, which would be an unrelated change.

Signature · 1 line

Jens Axboe

Jens Axboe

2008-03-22 12:56 UTC #9 lore raw

Re: [PATCH 4/5] cdrom: use list_head for cdrom_device_info list

Quoted 2 lines · Jens Axboe

You also seem to remove the !cdi check, which would be an unrelated change.

THat's of course fine, it was a relic of the old list search. So no problem with that part.

Signature · 1 line

Jens Axboe

Akinobu Mita

author

2008-03-22 15:10 UTC #10 lore raw

Re: [PATCH 4/5] cdrom: use list_head for cdrom_device_info list

In reply to Jens Axboe

2008/3/22, Jens Axboe <[email protected]>:

Quoted 10 lines · Akinobu Mita

Quoted 2 lines
Use list_head for cdrom_device_info list instead of opencoded singly list handling.
Looks good, but you don't seem to be initializing ->list anywhere. Did
you test this?
I'd suggest just adding an INIT_LIST_HEAD() before the list_add() in register_cdrom()

It seems that current list_add() implementation doesn't need initalized new entry with/without CONFIG_DEBUG_LIST.
And the following change will show too many warnings with CONFIG_DEBUG_LIST.

Is it recommended to add an INIT_LIST_HEAD() before the list_add()?

lib/list_debug.c +2 -0

Index: 2.6-git/lib/list_debug.c
===================================================================
@@ -8,6 +8,7 @@
 
 #include <linux/module.h>
 #include <linux/list.h>
+#include <linux/bug.h>
 
 /*
  * Insert a new entry between two known consecutive entries.

@@ -49,6 +50,7 @@ EXPORT_SYMBOL(__list_add);
  */
 void list_add(struct list_head *new, struct list_head *head)
 {
+	WARN_ON(!list_empty(new));
 	__list_add(new, head, head->next);
 }
 EXPORT_SYMBOL(list_add);

Christoph Hellwig

2008-03-22 17:58 UTC #11 lore raw

Re: [PATCH 4/5] cdrom: use list_head for cdrom_device_info list

Quoted 6 lines · Akinobu Mita

2008/3/22, Jens Axboe <[email protected]>:
Quoted 5 lines · Akinobu Mita
Quoted 2 lines
Use list_head for cdrom_device_info list instead of opencoded singly list handling.

Quoted 8 lines

Quoted 5 lines
Looks good, but you don't seem to be initializing ->list anywhere. Did
you test this?
I'd suggest just adding an INIT_LIST_HEAD() before the list_add() in register_cdrom()
It seems that current list_add() implementation doesn't need initalized new entry with/without CONFIG_DEBUG_LIST.

it never did and never should. only the list head needs to be initialized.

your patch is fine in that respect.

Jens Axboe

2008-03-25 19:05 UTC #12 lore raw

Re: [PATCH 4/5] cdrom: use list_head for cdrom_device_info list

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · Akinobu Mita
2008/3/22, Jens Axboe <[email protected]>:
Quoted 10 lines · Akinobu Mita
Quoted 2 lines
Use list_head for cdrom_device_info list instead of opencoded singly list handling.
Looks good, but you don't seem to be initializing ->list anywhere. Did
you test this?
I'd suggest just adding an INIT_LIST_HEAD() before the list_add() in register_cdrom()

Quoted 7 lines

Quoted 2 lines
It seems that current list_add() implementation doesn't need initalized new entry with/without CONFIG_DEBUG_LIST.
it never did and never should. only the list head needs to be initialized.
your patch is fine in that respect.

It is, my mistake.

Signature · 1 line

Jens Axboe