[PATCH] sg_cmd_done - another oops

[PATCH] sg_cmd_done - another oops

[Brian King]

[-- Attachment #1: Type: text/plain, Size: 64 bytes --]


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: sg_cmd_done_oops_again.patch --]
[-- Type: text/plain, Size: 3404 bytes --]


The following patch fixes a race condition in sg of sg_cmd_done racing
with sg_release. I've seen this bug hit several times on test machines
and the following patch fixes it. The race is that if srp->done is set
and the waiting thread gets a spurious wakeup immediately afterwards,
then the waiting thread can end up executing and completing, then getting
closed, freeing sfp before the wake_up_interruptible is called, which
then will result in an oops. The oops is fixed by locking around the
setting srp->done to 1 and the wake_up, and also locking around the
checking of srp->done, which guarantees that the wake_up_interruptible
will always occur before the sleeping thread gets a chance to run.


---

 linux-2.6.7-rc2-bjking1/drivers/scsi/sg.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff -puN drivers/scsi/sg.c~sg_cmd_done_oops_again drivers/scsi/sg.c
--- linux-2.6.7-rc2/drivers/scsi/sg.c~sg_cmd_done_oops_again	2004-06-02 15:44:00.000000000 -0500
+++ linux-2.6.7-rc2-bjking1/drivers/scsi/sg.c	2004-06-02 15:45:01.000000000 -0500
@@ -723,6 +723,18 @@ sg_common_write(Sg_fd * sfp, Sg_request 
 }
 
 static int
+sg_srp_done(Sg_request *srp, Sg_fd *sfp)
+{
+	unsigned long iflags;
+	int done;
+
+	read_lock_irqsave(&sfp->rq_list_lock, iflags);
+	done = srp->done;
+	read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
+	return done;
+}
+
+static int
 sg_ioctl(struct inode *inode, struct file *filp,
 	 unsigned int cmd_in, unsigned long arg)
 {
@@ -761,7 +773,7 @@ sg_ioctl(struct inode *inode, struct fil
 			while (1) {
 				result = 0;	/* following macro to beat race condition */
 				__wait_event_interruptible(sfp->read_wait,
-					(sdp->detached || sfp->closed || srp->done),
+					(sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
 							   result);
 				if (sdp->detached)
 					return -ENODEV;
@@ -772,7 +784,9 @@ sg_ioctl(struct inode *inode, struct fil
 				srp->orphan = 1;
 				return result;	/* -ERESTARTSYS because signal hit process */
 			}
+			write_lock_irqsave(&sfp->rq_list_lock, iflags);
 			srp->done = 2;
+			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
 			return (result < 0) ? result : 0;
 		}
@@ -1215,6 +1229,7 @@ sg_cmd_done(Scsi_Cmnd * SCpnt)
 	Sg_device *sdp = NULL;
 	Sg_fd *sfp;
 	Sg_request *srp = NULL;
+	unsigned long iflags;
 
 	if (SCpnt && (SRpnt = SCpnt->sc_request))
 		srp = (Sg_request *) SRpnt->upper_private_data;
@@ -1303,8 +1318,10 @@ sg_cmd_done(Scsi_Cmnd * SCpnt)
 	if (sfp && srp) {
 		/* Now wake up any sg_read() that is waiting for this packet. */
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_IN);
+		write_lock_irqsave(&sfp->rq_list_lock, iflags);
 		srp->done = 1;
 		wake_up_interruptible(&sfp->read_wait);
+		write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 	}
 }
 
@@ -1490,7 +1507,7 @@ sg_remove(struct class_device *cl_dev)
 				tsfp = sfp->nextfp;
 				for (srp = sfp->headrp; srp; srp = tsrp) {
 					tsrp = srp->nextrp;
-					if (sfp->closed || (0 == srp->done))
+					if (sfp->closed || (0 == sg_srp_done(srp, sfp)))
 						sg_finish_rem_req(srp);
 				}
 				if (sfp->closed) {
@@ -2472,7 +2489,7 @@ sg_remove_sfp(Sg_device * sdp, Sg_fd * s
 
 	for (srp = sfp->headrp; srp; srp = tsrp) {
 		tsrp = srp->nextrp;
-		if (srp->done)
+		if (sg_srp_done(srp, sfp))
 			sg_finish_rem_req(srp);
 		else
 			++dirty;

_

Re: [PATCH] sg_cmd_done - another oops

[Guennadi Liakhovetski]

On Fri, 4 Jun 2004, Brian King wrote:

> static int
>+sg_srp_done(Sg_request *srp, Sg_fd *sfp)
>+{
>+       unsigned long iflags;
>+       int done;
>+
>+       read_lock_irqsave(&sfp->rq_list_lock, iflags);
>+       done = srp->done;
>+       read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
>+       return done;
>+}

Hm, maybe I don't understand something - why do you have to close local
interrupts here

>@@ -772,7 +784,9 @@ sg_ioctl(struct inode *inode, struct fil
>                                srp->orphan = 1;
>                                return result;  /* -ERESTARTSYS because signal
>hit process */
>                        }
>+                       write_lock_irqsave(&sfp->rq_list_lock, iflags);
>                        srp->done = 2;
>+                       write_unlock_irqrestore(&sfp->rq_list_lock, iflags);

and here? Wouldn't a read/write_lock() suffice?

Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] sg_cmd_done - another oops

[Jens Axboe]

On Sat, Jun 05 2004, Guennadi Liakhovetski wrote:
> On Fri, 4 Jun 2004, Brian King wrote:
> 
> > static int
> >+sg_srp_done(Sg_request *srp, Sg_fd *sfp)
> >+{
> >+       unsigned long iflags;
> >+       int done;
> >+
> >+       read_lock_irqsave(&sfp->rq_list_lock, iflags);
> >+       done = srp->done;
> >+       read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> >+       return done;
> >+}
> 
> Hm, maybe I don't understand something - why do you have to close local
> interrupts here
> 
> >@@ -772,7 +784,9 @@ sg_ioctl(struct inode *inode, struct fil
> >                                srp->orphan = 1;
> >                                return result;  /* -ERESTARTSYS because signal
> >hit process */
> >                        }
> >+                       write_lock_irqsave(&sfp->rq_list_lock, iflags);
> >                        srp->done = 2;
> >+                       write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> 
> and here? Wouldn't a read/write_lock() suffice?

interrupt -> sg_cmd_done -> sg_finish_rem_req -> write_lock on
rq_list_lock.

-- 
Jens Axboe

Re: [PATCH] sg_cmd_done - another oops

[Guennadi Liakhovetski]

On Sun, 6 Jun 2004, Jens Axboe wrote:

> interrupt -> sg_cmd_done -> sg_finish_rem_req -> write_lock on
> rq_list_lock.

I see, thanks!

Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] sg_cmd_done - another oops

[Douglas Gilbert]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Brian King wrote:
> The following patch fixes a race condition in sg 
 > of sg_cmd_done racing with sg_release. I've seen this
 > bug hit several times on test machines and the following
 > patch fixes it. The race is that if srp->done is set
 > and the waiting thread gets a spurious wakeup immediately
 > afterwards, then the waiting thread can end up executing
 > and completing, then getting closed, freeing sfp before
 > the wake_up_interruptible is called, which then will result
 > in an oops. The oops is fixed by locking around the
> setting srp->done to 1 and the wake_up, and also locking 
 > around the checking of srp->done, which guarantees that
 > the wake_up_interruptible will always occur before the
 > sleeping thread gets a chance to run.

Brian,
Thanks for this analysis; I'm still trying to get my head
around it :-) These "spurious wakeups" are a worry. Does
your testing produce them or do they occur randomly?

The critical part of your patch seems to be the write
lock in sg_cmd_done() making sure srp->done is set _and_
wake_up_interruptible() gets executed before the
__wait_event_interruptible macro in sg_read() or
sg_remove(_sfp) can proceed.

I thought about making srp->done atomic (of type atomic_t)
but that does not seem to be sufficient. So I'm happy for
James to apply this patch.


An attempt to consolidate... Almost all of the changes to
sg that were discussed in the "2.6.6-rc3 ia64
smp_call_function() called with interrupts disabled" thread
(about a month ago) are now in lk 2.6.7-rc2. The janitors
have also been at work with "__user" type modifiers.
Some changes that Patrick Mansfield proposed are not in
rc2:
   - up max number of devices to 32K
   - revert vmalloc()s to kmalloc()s since the former are
     problematic in big configurations

The attached patch merges Brian patches with those proposed
by Patrick. The patch is against lk 2.6.7-rc2 (and sg hasn't
changed between rc2 and rc2-bk5).

Doug Gilbert



[-- Attachment #2: sg267rc2.diff --]
[-- Type: text/x-patch, Size: 4732 bytes --]

--- linux/drivers/scsi/sg.c	2004-06-03 12:45:12.000000000 +1000
+++ linux/drivers/scsi/sg.c267rc2bk1pl	2004-06-07 12:29:52.879588688 +1000
@@ -42,7 +42,6 @@
 #include <linux/fcntl.h>
 #include <linux/init.h>
 #include <linux/poll.h>
-#include <linux/vmalloc.h>
 #include <linux/smp_lock.h>
 #include <linux/moduleparam.h>
 #include <linux/devfs_fs_kernel.h>
@@ -60,7 +59,7 @@
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_date = "20040513";
+static char *sg_version_date = "20040607";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);
@@ -73,7 +72,7 @@
 #define SG_ALLOW_DIO_DEF 0
 #define SG_ALLOW_DIO_CODE /* compile out by commenting this define */
 
-#define SG_MAX_DEVS 8192
+#define SG_MAX_DEVS 32768
 
 /*
  * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
@@ -723,6 +722,18 @@
 }
 
 static int
+sg_srp_done(Sg_request *srp, Sg_fd *sfp)
+{
+	unsigned long iflags;
+	int done;
+
+	read_lock_irqsave(&sfp->rq_list_lock, iflags);
+	done = srp->done;
+	read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
+	return done;
+}
+
+static int
 sg_ioctl(struct inode *inode, struct file *filp,
 	 unsigned int cmd_in, unsigned long arg)
 {
@@ -761,7 +772,7 @@
 			while (1) {
 				result = 0;	/* following macro to beat race condition */
 				__wait_event_interruptible(sfp->read_wait,
-					(sdp->detached || sfp->closed || srp->done),
+					(sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
 							   result);
 				if (sdp->detached)
 					return -ENODEV;
@@ -772,7 +783,9 @@
 				srp->orphan = 1;
 				return result;	/* -ERESTARTSYS because signal hit process */
 			}
+			write_lock_irqsave(&sfp->rq_list_lock, iflags);
 			srp->done = 2;
+			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
 			return (result < 0) ? result : 0;
 		}
@@ -1215,6 +1228,7 @@
 	Sg_device *sdp = NULL;
 	Sg_fd *sfp;
 	Sg_request *srp = NULL;
+	unsigned long iflags;
 
 	if (SCpnt && (SRpnt = SCpnt->sc_request))
 		srp = (Sg_request *) SRpnt->upper_private_data;
@@ -1303,8 +1317,10 @@
 	if (sfp && srp) {
 		/* Now wake up any sg_read() that is waiting for this packet. */
 		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_IN);
+		write_lock_irqsave(&sfp->rq_list_lock, iflags);
 		srp->done = 1;
 		wake_up_interruptible(&sfp->read_wait);
+		write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 	}
 }
 
@@ -1331,17 +1347,18 @@
 	void *old_sg_dev_arr = NULL;
 	int k, error;
 
-	sdp = vmalloc(sizeof(Sg_device));
-	if (!sdp)
+	sdp = kmalloc(sizeof(Sg_device), GFP_KERNEL);
+	if (!sdp) {
+		printk(KERN_WARNING "sg_alloc: kmalloc Sg_device failure\n");
 		return -ENOMEM;
-
+	}
 	write_lock_irqsave(&sg_dev_arr_lock, iflags);
 	if (unlikely(sg_nr_dev >= sg_dev_max)) {	/* try to resize */
 		Sg_device **tmp_da;
 		int tmp_dev_max = sg_nr_dev + SG_DEV_ARR_LUMP;
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
 
-		tmp_da = vmalloc(tmp_dev_max * sizeof(Sg_device *));
+		tmp_da = kmalloc(tmp_dev_max * sizeof(Sg_device *), GFP_KERNEL);
 		if (unlikely(!tmp_da))
 			goto expand_failed;
 
@@ -1375,8 +1392,8 @@
 
  out:
 	if (error < 0)
-		vfree(sdp);
-	vfree(old_sg_dev_arr);
+		kfree(sdp);
+	kfree(old_sg_dev_arr);
 	return error;
 
  expand_failed:
@@ -1404,14 +1421,18 @@
 	int error, k;
 
 	disk = alloc_disk(1);
-	if (!disk)
+	if (!disk) {
+		printk(KERN_WARNING "alloc_disk failed\n");
 		return -ENOMEM;
+	}
 	disk->major = SCSI_GENERIC_MAJOR;
 
 	error = -ENOMEM;
 	cdev = cdev_alloc();
-	if (!cdev)
+	if (!cdev) {
+		printk(KERN_WARNING "cdev_alloc failed\n");
 		goto out;
+	}
 	cdev->owner = THIS_MODULE;
 	cdev->ops = &sg_fops;
 
@@ -1490,7 +1511,7 @@
 				tsfp = sfp->nextfp;
 				for (srp = sfp->headrp; srp; srp = tsrp) {
 					tsrp = srp->nextrp;
-					if (sfp->closed || (0 == srp->done))
+					if (sfp->closed || (0 == sg_srp_done(srp, sfp)))
 						sg_finish_rem_req(srp);
 				}
 				if (sfp->closed) {
@@ -1525,7 +1546,7 @@
 		put_disk(sdp->disk);
 		sdp->disk = NULL;
 		if (NULL == sdp->headfp)
-			vfree((char *) sdp);
+			kfree((char *) sdp);
 	}
 
 	if (delay)
@@ -1590,7 +1611,7 @@
 	unregister_chrdev_region(MKDEV(SCSI_GENERIC_MAJOR, 0),
 				 SG_MAX_DEVS);
 	if (sg_dev_arr != NULL) {
-		vfree((char *) sg_dev_arr);
+		kfree((char *) sg_dev_arr);
 		sg_dev_arr = NULL;
 	}
 	sg_dev_max = 0;
@@ -2472,7 +2493,7 @@
 
 	for (srp = sfp->headrp; srp; srp = tsrp) {
 		tsrp = srp->nextrp;
-		if (srp->done)
+		if (sg_srp_done(srp, sfp))
 			sg_finish_rem_req(srp);
 		else
 			++dirty;
@@ -2492,7 +2513,7 @@
 			}
 			if (k < maxd)
 				sg_dev_arr[k] = NULL;
-			vfree((char *) sdp);
+			kfree((char *) sdp);
 			res = 1;
 		}
 		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);

Re: [PATCH] sg_cmd_done - another oops

[Brian King]

Douglas Gilbert wrote:
> Brian,
> Thanks for this analysis; I'm still trying to get my head
> around it :-) These "spurious wakeups" are a worry. Does
> your testing produce them or do they occur randomly?

They occur randomly.

> The critical part of your patch seems to be the write
> lock in sg_cmd_done() making sure srp->done is set _and_
> wake_up_interruptible() gets executed before the
> __wait_event_interruptible macro in sg_read() or
> sg_remove(_sfp) can proceed.

That is correct.

> The attached patch merges Brian patches with those proposed
> by Patrick. The patch is against lk 2.6.7-rc2 (and sg hasn't
> changed between rc2 and rc2-bk5).

The patch looks good.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH] sg_cmd_done - another oops

[Brian King]

James,

This patch appears to have been dropped.

-Brian

Douglas Gilbert wrote:
> Brian King wrote:
> 
>> The following patch fixes a race condition in sg 
> 
>  > of sg_cmd_done racing with sg_release. I've seen this
>  > bug hit several times on test machines and the following
>  > patch fixes it. The race is that if srp->done is set
>  > and the waiting thread gets a spurious wakeup immediately
>  > afterwards, then the waiting thread can end up executing
>  > and completing, then getting closed, freeing sfp before
>  > the wake_up_interruptible is called, which then will result
>  > in an oops. The oops is fixed by locking around the
> 
>> setting srp->done to 1 and the wake_up, and also locking 
> 
>  > around the checking of srp->done, which guarantees that
>  > the wake_up_interruptible will always occur before the
>  > sleeping thread gets a chance to run.
> 
> Brian,
> Thanks for this analysis; I'm still trying to get my head
> around it :-) These "spurious wakeups" are a worry. Does
> your testing produce them or do they occur randomly?
> 
> The critical part of your patch seems to be the write
> lock in sg_cmd_done() making sure srp->done is set _and_
> wake_up_interruptible() gets executed before the
> __wait_event_interruptible macro in sg_read() or
> sg_remove(_sfp) can proceed.
> 
> I thought about making srp->done atomic (of type atomic_t)
> but that does not seem to be sufficient. So I'm happy for
> James to apply this patch.
> 
> 
> An attempt to consolidate... Almost all of the changes to
> sg that were discussed in the "2.6.6-rc3 ia64
> smp_call_function() called with interrupts disabled" thread
> (about a month ago) are now in lk 2.6.7-rc2. The janitors
> have also been at work with "__user" type modifiers.
> Some changes that Patrick Mansfield proposed are not in
> rc2:
>   - up max number of devices to 32K
>   - revert vmalloc()s to kmalloc()s since the former are
>     problematic in big configurations
> 
> The attached patch merges Brian patches with those proposed
> by Patrick. The patch is against lk 2.6.7-rc2 (and sg hasn't
> changed between rc2 and rc2-bk5).
> 
> Doug Gilbert
> 
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux/drivers/scsi/sg.c	2004-06-03 12:45:12.000000000 +1000
> +++ linux/drivers/scsi/sg.c267rc2bk1pl	2004-06-07 12:29:52.879588688 +1000
> @@ -42,7 +42,6 @@
>  #include <linux/fcntl.h>
>  #include <linux/init.h>
>  #include <linux/poll.h>
> -#include <linux/vmalloc.h>
>  #include <linux/smp_lock.h>
>  #include <linux/moduleparam.h>
>  #include <linux/devfs_fs_kernel.h>
> @@ -60,7 +59,7 @@
>  
>  #ifdef CONFIG_SCSI_PROC_FS
>  #include <linux/proc_fs.h>
> -static char *sg_version_date = "20040513";
> +static char *sg_version_date = "20040607";
>  
>  static int sg_proc_init(void);
>  static void sg_proc_cleanup(void);
> @@ -73,7 +72,7 @@
>  #define SG_ALLOW_DIO_DEF 0
>  #define SG_ALLOW_DIO_CODE /* compile out by commenting this define */
>  
> -#define SG_MAX_DEVS 8192
> +#define SG_MAX_DEVS 32768
>  
>  /*
>   * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
> @@ -723,6 +722,18 @@
>  }
>  
>  static int
> +sg_srp_done(Sg_request *srp, Sg_fd *sfp)
> +{
> +	unsigned long iflags;
> +	int done;
> +
> +	read_lock_irqsave(&sfp->rq_list_lock, iflags);
> +	done = srp->done;
> +	read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> +	return done;
> +}
> +
> +static int
>  sg_ioctl(struct inode *inode, struct file *filp,
>  	 unsigned int cmd_in, unsigned long arg)
>  {
> @@ -761,7 +772,7 @@
>  			while (1) {
>  				result = 0;	/* following macro to beat race condition */
>  				__wait_event_interruptible(sfp->read_wait,
> -					(sdp->detached || sfp->closed || srp->done),
> +					(sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
>  							   result);
>  				if (sdp->detached)
>  					return -ENODEV;
> @@ -772,7 +783,9 @@
>  				srp->orphan = 1;
>  				return result;	/* -ERESTARTSYS because signal hit process */
>  			}
> +			write_lock_irqsave(&sfp->rq_list_lock, iflags);
>  			srp->done = 2;
> +			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
>  			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
>  			return (result < 0) ? result : 0;
>  		}
> @@ -1215,6 +1228,7 @@
>  	Sg_device *sdp = NULL;
>  	Sg_fd *sfp;
>  	Sg_request *srp = NULL;
> +	unsigned long iflags;
>  
>  	if (SCpnt && (SRpnt = SCpnt->sc_request))
>  		srp = (Sg_request *) SRpnt->upper_private_data;
> @@ -1303,8 +1317,10 @@
>  	if (sfp && srp) {
>  		/* Now wake up any sg_read() that is waiting for this packet. */
>  		kill_fasync(&sfp->async_qp, SIGPOLL, POLL_IN);
> +		write_lock_irqsave(&sfp->rq_list_lock, iflags);
>  		srp->done = 1;
>  		wake_up_interruptible(&sfp->read_wait);
> +		write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
>  	}
>  }
>  
> @@ -1331,17 +1347,18 @@
>  	void *old_sg_dev_arr = NULL;
>  	int k, error;
>  
> -	sdp = vmalloc(sizeof(Sg_device));
> -	if (!sdp)
> +	sdp = kmalloc(sizeof(Sg_device), GFP_KERNEL);
> +	if (!sdp) {
> +		printk(KERN_WARNING "sg_alloc: kmalloc Sg_device failure\n");
>  		return -ENOMEM;
> -
> +	}
>  	write_lock_irqsave(&sg_dev_arr_lock, iflags);
>  	if (unlikely(sg_nr_dev >= sg_dev_max)) {	/* try to resize */
>  		Sg_device **tmp_da;
>  		int tmp_dev_max = sg_nr_dev + SG_DEV_ARR_LUMP;
>  		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);
>  
> -		tmp_da = vmalloc(tmp_dev_max * sizeof(Sg_device *));
> +		tmp_da = kmalloc(tmp_dev_max * sizeof(Sg_device *), GFP_KERNEL);
>  		if (unlikely(!tmp_da))
>  			goto expand_failed;
>  
> @@ -1375,8 +1392,8 @@
>  
>   out:
>  	if (error < 0)
> -		vfree(sdp);
> -	vfree(old_sg_dev_arr);
> +		kfree(sdp);
> +	kfree(old_sg_dev_arr);
>  	return error;
>  
>   expand_failed:
> @@ -1404,14 +1421,18 @@
>  	int error, k;
>  
>  	disk = alloc_disk(1);
> -	if (!disk)
> +	if (!disk) {
> +		printk(KERN_WARNING "alloc_disk failed\n");
>  		return -ENOMEM;
> +	}
>  	disk->major = SCSI_GENERIC_MAJOR;
>  
>  	error = -ENOMEM;
>  	cdev = cdev_alloc();
> -	if (!cdev)
> +	if (!cdev) {
> +		printk(KERN_WARNING "cdev_alloc failed\n");
>  		goto out;
> +	}
>  	cdev->owner = THIS_MODULE;
>  	cdev->ops = &sg_fops;
>  
> @@ -1490,7 +1511,7 @@
>  				tsfp = sfp->nextfp;
>  				for (srp = sfp->headrp; srp; srp = tsrp) {
>  					tsrp = srp->nextrp;
> -					if (sfp->closed || (0 == srp->done))
> +					if (sfp->closed || (0 == sg_srp_done(srp, sfp)))
>  						sg_finish_rem_req(srp);
>  				}
>  				if (sfp->closed) {
> @@ -1525,7 +1546,7 @@
>  		put_disk(sdp->disk);
>  		sdp->disk = NULL;
>  		if (NULL == sdp->headfp)
> -			vfree((char *) sdp);
> +			kfree((char *) sdp);
>  	}
>  
>  	if (delay)
> @@ -1590,7 +1611,7 @@
>  	unregister_chrdev_region(MKDEV(SCSI_GENERIC_MAJOR, 0),
>  				 SG_MAX_DEVS);
>  	if (sg_dev_arr != NULL) {
> -		vfree((char *) sg_dev_arr);
> +		kfree((char *) sg_dev_arr);
>  		sg_dev_arr = NULL;
>  	}
>  	sg_dev_max = 0;
> @@ -2472,7 +2493,7 @@
>  
>  	for (srp = sfp->headrp; srp; srp = tsrp) {
>  		tsrp = srp->nextrp;
> -		if (srp->done)
> +		if (sg_srp_done(srp, sfp))
>  			sg_finish_rem_req(srp);
>  		else
>  			++dirty;
> @@ -2492,7 +2513,7 @@
>  			}
>  			if (k < maxd)
>  				sg_dev_arr[k] = NULL;
> -			vfree((char *) sdp);
> +			kfree((char *) sdp);
>  			res = 1;
>  		}
>  		write_unlock_irqrestore(&sg_dev_arr_lock, iflags);

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center