Re: [patch] 2.4 fix for sg driver cmd_done oops

[Douglas Gilbert <dougg@torque.net>]

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

Tony Battersby wrote:
>>This problem has also been seen on 2.4.  The attached patch is a
>>straight-forward port of Brian's patch to 2.4.28-pre3. Are there any
>>2.4-specific issues that this does not address?
> 
> 
> Try this patch instead, which deletes one line containing "srp->done =
> 1;" compared to your patch.  The 2.6 sg.c moved this line down to fix a
> race condition and then later added the locks around it; your patch adds
> the line after it was moved but doesn't delete the original line.

Tom,
Could you confirm whether this patch from Tony fixes the
problem that you reported and if so we can ask Marcelo
to apply it.

Doug Gilbert

[-- Attachment #2: sg_cmd_done_race_2.4.28-pre3.patch --]
[-- Type: text/x-patch, Size: 2564 bytes --]

--- linux-2.4.28-pre3/drivers/scsi/sg.c.orig	Thu Sep 30 17:39:10 2004
+++ linux-2.4.28-pre3/drivers/scsi/sg.c	Thu Sep 30 17:39:37 2004
@@ -752,6 +752,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)
 {
@@ -786,7 +798,8 @@
 	    while (1) {
 		result = 0;  /* following macro to beat race condition */
 		__wait_event_interruptible(sfp->read_wait,
-		       (sdp->detached || sfp->closed || srp->done), result);
+		       (sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
+					   result);
 		if (sdp->detached)
 		    return -ENODEV;
 		if (sfp->closed)
@@ -796,7 +809,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, (char *)arg, SZ_SG_IO_HDR, srp);
 	    return (result < 0) ? result : 0;
 	}
@@ -1207,6 +1222,7 @@
     Sg_device * sdp = NULL;
     Sg_fd * sfp;
     Sg_request * srp = NULL;
+    unsigned long iflags;
 
     read_lock(&sg_dev_arr_lock);
     if (sg_dev_arr && (dev >= 0)) {
@@ -1253,7 +1269,6 @@
     SRpnt->sr_request.rq_dev = MKDEV(0, 0);  /* "sg" _disowns_ request blk */
 
     srp->my_cmdp = NULL;
-    srp->done = 1;
     read_unlock(&sg_dev_arr_lock);
 
     SCSI_LOG_TIMEOUT(4, printk("sg...bh: dev=%d, pack_id=%d, res=0x%x\n",
@@ -1316,8 +1331,11 @@
     }
     if (sfp && srp) {
 	/* Now wake up any sg_read() that is waiting for this packet. */
-	wake_up_interruptible(&sfp->read_wait);
 	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);
     }
 }
 
@@ -1507,7 +1525,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) {
@@ -2425,7 +2443,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;