[PATCH 1/5] cdrom: remove ifdef CONFIG_SYSCTL

[PATCH 1/5] cdrom: remove ifdef CONFIG_SYSCTL

[Akinobu Mita]

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 <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/cdrom/cdrom.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/cdrom.c
+++ 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);

[PATCH 2/5] cdrom: cleanup hardcoded error-code

[Akinobu Mita]

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 <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/cdrom/cdrom.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/cdrom.c
+++ 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;

[PATCH 3/5] cdrom: protect cdrom_device_info list by mutex

[Akinobu Mita]

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 <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/cdrom/cdrom.c |    2 ++
 1 file changed, 2 insertions(+)

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/cdrom.c
+++ 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,

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

[Akinobu Mita]

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

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/cdrom/cdrom.c |   29 ++++++-----------------------
 include/linux/cdrom.h |    3 ++-
 2 files changed, 8 insertions(+), 24 deletions(-)

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/cdrom.c
+++ 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)
Index: 2.6-git/include/linux/cdrom.h
===================================================================
--- 2.6-git.orig/include/linux/cdrom.h
+++ 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 */

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

[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 <akinobu.mita@gmail.com>
Cc: Adrian McMenamin <adrian@mcmen.demon.co.uk>
Cc: Borislav Petkov <petkovbb@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>
---
 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(-)

Index: 2.6-git/drivers/cdrom/cdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/cdrom.c
+++ 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,
Index: 2.6-git/drivers/cdrom/gdrom.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/gdrom.c
+++ 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 = {
Index: 2.6-git/drivers/cdrom/viocd.c
===================================================================
--- 2.6-git.orig/drivers/cdrom/viocd.c
+++ 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);
Index: 2.6-git/include/linux/cdrom.h
===================================================================
--- 2.6-git.orig/include/linux/cdrom.h
+++ 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;
Index: 2.6-git/Documentation/cdrom/cdrom-standard.tex
===================================================================
--- 2.6-git.orig/Documentation/cdrom/cdrom-standard.tex
+++ 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
Index: 2.6-git/drivers/ide/ide-cd.c
===================================================================
--- 2.6-git.orig/drivers/ide/ide-cd.c
+++ 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);

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

[Jens Axboe]

On Sat, Mar 22 2008, Akinobu Mita wrote:
> 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.

-- 
Jens Axboe

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

[Jens Axboe]

On Sat, Mar 22 2008, Jens Axboe wrote:
> 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.

-- 
Jens Axboe

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

[Jens Axboe]

On Sat, Mar 22 2008, Akinobu Mita wrote:
> 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.

-- 
Jens Axboe

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

[Akinobu Mita]

2008/3/22, Jens Axboe <jens.axboe@oracle.com>:
> On Sat, Mar 22 2008, Akinobu Mita wrote:
>  > 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()?

Index: 2.6-git/lib/list_debug.c
===================================================================
--- 2.6-git.orig/lib/list_debug.c
+++ 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);

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

[Borislav Petkov]

On Sat, Mar 22, 2008 at 12:14:14PM +0900, Akinobu Mita wrote:
> Now unregister_cdrom() always returns 0.
> Make it return void and update all callers that check the return value.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Adrian McMenamin <adrian@mcmen.demon.co.uk>
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>  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 <petkovbb@gmail.com>

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

[Adrian McMenamin]

On Sat, 2008-03-22 at 12:14 +0900, Akinobu Mita wrote:
> Now unregister_cdrom() always returns 0.
> Make it return void and update all callers that check the return value.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Adrian McMenamin <adrian@mcmen.demon.co.uk>
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
>  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 <adrian@mcmen.demon.co.uk>

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

[Christoph Hellwig]

On Sun, Mar 23, 2008 at 12:10:45AM +0900, Akinobu Mita wrote:
> 2008/3/22, Jens Axboe <jens.axboe@oracle.com>:
> > On Sat, Mar 22 2008, Akinobu Mita wrote:
> >  > 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.

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

your patch is fine in that respect.

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

[Jens Axboe]

On Sat, Mar 22 2008, Christoph Hellwig wrote:
> On Sun, Mar 23, 2008 at 12:10:45AM +0900, Akinobu Mita wrote:
> > 2008/3/22, Jens Axboe <jens.axboe@oracle.com>:
> > > On Sat, Mar 22 2008, Akinobu Mita wrote:
> > >  > 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.
> 
> 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.

-- 
Jens Axboe