sg: remove unnecessary indentation

PATCH In review Jörn Engel 2 participants 26 messages Apr 12, 2012 – May 16, 2012

Subsystems (2)

SCSI SCSI SG driver

lore

Share & tools

lore

Jörn Engel

author

2012-04-12 23:28 UTC #0 lore raw

PATCH 1/10 sg: remove unnecessary indentation

blocking is de-facto a constant and the now-removed comment wasn't all that useful either. Without them and the resulting indentation the code is a bit nicer to read.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   53 ++++++++++++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 29 deletions(-)

drivers/scsi/sg.c +24 ~~-29~~

index eacd46b..14de843 100644
@@ -791,40 +791,35 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 
 	switch (cmd_in) {
 	case SG_IO:
-		{
-			int blocking = 1;	/* ignore O_NONBLOCK flag */
-
+		if (sdp->detached)
+			return -ENODEV;
+		if (!scsi_block_when_processing_errors(sdp->device))
+			return -ENXIO;
+		if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
+			return -EFAULT;
+		result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
+				 1, read_only, 1, &srp);
+		if (result < 0)
+			return result;
+		while (1) {
+			result = 0;	/* following macro to beat race condition */
+			__wait_event_interruptible(sfp->read_wait,
+				(srp->done || sdp->detached),
+				result);
 			if (sdp->detached)
 				return -ENODEV;
-			if (!scsi_block_when_processing_errors(sdp->device))
-				return -ENXIO;
-			if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
-				return -EFAULT;
-			result =
-			    sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
-					 blocking, read_only, 1, &srp);
-			if (result < 0)
-				return result;
-			while (1) {
-				result = 0;	/* following macro to beat race condition */
-				__wait_event_interruptible(sfp->read_wait,
-					(srp->done || sdp->detached),
-					result);
-				if (sdp->detached)
-					return -ENODEV;
-				write_lock_irq(&sfp->rq_list_lock);
-				if (srp->done) {
-					srp->done = 2;
-					write_unlock_irq(&sfp->rq_list_lock);
-					break;
-				}
-				srp->orphan = 1;
+			write_lock_irq(&sfp->rq_list_lock);
+			if (srp->done) {
+				srp->done = 2;
 				write_unlock_irq(&sfp->rq_list_lock);
-				return result;	/* -ERESTARTSYS because signal hit process */
+				break;
 			}
-			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
-			return (result < 0) ? result : 0;
+			srp->orphan = 1;
+			write_unlock_irq(&sfp->rq_list_lock);
+			return result;	/* -ERESTARTSYS because signal hit process */
 		}
+		result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
+		return (result < 0) ? result : 0;
 	case SG_SET_TIMEOUT:
 		result = get_user(val, ip);
 		if (result)

Signature · 1 line

1.7.9.1

Jörn Engel

author

2012-04-12 23:29 UTC #1 lore raw

PATCH 2/10 sg: remove while (1) non-loop

The while (1) construct isn't actually a loop at all. So let's not pretent and obfuscate the code.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)

drivers/scsi/sg.c +13 ~~-17~~

index 14de843..f9e89b2 100644
@@ -801,25 +801,21 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 				 1, read_only, 1, &srp);
 		if (result < 0)
 			return result;
-		while (1) {
-			result = 0;	/* following macro to beat race condition */
-			__wait_event_interruptible(sfp->read_wait,
-				(srp->done || sdp->detached),
-				result);
-			if (sdp->detached)
-				return -ENODEV;
-			write_lock_irq(&sfp->rq_list_lock);
-			if (srp->done) {
-				srp->done = 2;
-				write_unlock_irq(&sfp->rq_list_lock);
-				break;
-			}
-			srp->orphan = 1;
+		result = 0;	/* following macro to beat race condition */
+		__wait_event_interruptible(sfp->read_wait,
+			(srp->done || sdp->detached), result);
+		if (sdp->detached)
+			return -ENODEV;
+		write_lock_irq(&sfp->rq_list_lock);
+		if (srp->done) {
+			srp->done = 2;
 			write_unlock_irq(&sfp->rq_list_lock);
-			return result;	/* -ERESTARTSYS because signal hit process */
+			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
+			return (result < 0) ? result : 0;
 		}
-		result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
-		return (result < 0) ? result : 0;
+		srp->orphan = 1;
+		write_unlock_irq(&sfp->rq_list_lock);
+		return result;	/* -ERESTARTSYS because signal hit process */
 	case SG_SET_TIMEOUT:
 		result = get_user(val, ip);
 		if (result)

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 2/10

2012-05-16 21:57 UTC #2 lore raw

Re: [PATCH 02/10] sg: remove while (1) non-loop

Quoted 4 lines · Jörn Engel

The while (1) construct isn't actually a loop at all. So let's not pretent and obfuscate the code.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 37 lines

---
  drivers/scsi/sg.c |   30 +++++++++++++-----------------
  1 files changed, 13 insertions(+), 17 deletions(-)

drivers/scsi/sg.c +9 ~~-15~~

index 14de843..f9e89b2 100644
@@ -801,25 +801,21 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  				 1, read_only, 1,&srp);
  		if (result<  0)
  			return result;
-		while (1) {
-			result = 0;	/* following macro to beat race condition */
-			__wait_event_interruptible(sfp->read_wait,
-				(srp->done || sdp->detached),
-				result);
-			if (sdp->detached)
-				return -ENODEV;
-			write_lock_irq(&sfp->rq_list_lock);
-			if (srp->done) {
-				srp->done = 2;
-				write_unlock_irq(&sfp->rq_list_lock);
-				break;
-			}
-			srp->orphan = 1;
+		result = 0;	/* following macro to beat race condition */
+		__wait_event_interruptible(sfp->read_wait,
+			(srp->done || sdp->detached), result);
+		if (sdp->detached)
+			return -ENODEV;
+		write_lock_irq(&sfp->rq_list_lock);
+		if (srp->done) {
+			srp->done = 2;
  			write_unlock_irq(&sfp->rq_list_lock);
-			return result;	/* -ERESTARTSYS because signal hit process */
+			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);

drivers/scsi/sg.c sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg) · line 821

Quoted 10 lines

drivers/scsi/sg.c +4 -2

+			return (result<  0) ? result : 0;
  		}
-		result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
-		return (result<  0) ? result : 0;
+		srp->orphan = 1;
+		write_unlock_irq(&sfp->rq_list_lock);
+		return result;	/* -ERESTARTSYS because signal hit process */
  	case SG_SET_TIMEOUT:
  		result = get_user(val, ip);
  		if (result)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:29 UTC #3 lore raw

PATCH 3/10 sg: remove while (1) non-loop

In reply to Jörn Engel

The while (1) construct isn't actually a loop at all. So let's not pretent and obfuscate the code.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

drivers/scsi/sg.c +9 ~~-13~~

index f9e89b2..be812e0 100644
@@ -398,19 +398,15 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 			retval = -EAGAIN;
 			goto free_old_hdr;
 		}
-		while (1) {
-			retval = 0; /* following macro beats race condition */
-			__wait_event_interruptible(sfp->read_wait,
-				(sdp->detached ||
-				(srp = sg_get_rq_mark(sfp, req_pack_id))), 
-				retval);
-			if (sdp->detached) {
-				retval = -ENODEV;
-				goto free_old_hdr;
-			}
-			if (0 == retval)
-				break;
-
+		retval = 0; /* following macro beats race condition */
+		__wait_event_interruptible(sfp->read_wait,
+			(sdp->detached ||
+			(srp = sg_get_rq_mark(sfp, req_pack_id))), retval);
+		if (sdp->detached) {
+			retval = -ENODEV;
+			goto free_old_hdr;
+		}
+		if (retval) {
 			/* -ERESTARTSYS as signal hit process */
 			goto free_old_hdr;
 		}

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 3/10

2012-05-16 21:57 UTC #4 lore raw

Re: [PATCH 03/10] sg: remove while (1) non-loop

Quoted 4 lines · Jörn Engel

The while (1) construct isn't actually a loop at all. So let's not pretent and obfuscate the code.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 27 lines

---
  drivers/scsi/sg.c |   22 +++++++++-------------
  1 files changed, 9 insertions(+), 13 deletions(-)

drivers/scsi/sg.c +2 ~~-13~~

index f9e89b2..be812e0 100644
@@ -398,19 +398,15 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
  			retval = -EAGAIN;
  			goto free_old_hdr;
  		}
-		while (1) {
-			retval = 0; /* following macro beats race condition */
-			__wait_event_interruptible(sfp->read_wait,
-				(sdp->detached ||
-				(srp = sg_get_rq_mark(sfp, req_pack_id))),
-				retval);
-			if (sdp->detached) {
-				retval = -ENODEV;
-				goto free_old_hdr;
-			}
-			if (0 == retval)
-				break;
-
+		retval = 0; /* following macro beats race condition */
+		__wait_event_interruptible(sfp->read_wait,

drivers/scsi/sg.c sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos) · line 412

Quoted 10 lines

drivers/scsi/sg.c +7 -0

+			(sdp->detached ||
+			(srp = sg_get_rq_mark(sfp, req_pack_id))), retval);
+		if (sdp->detached) {
+			retval = -ENODEV;
+			goto free_old_hdr;
+		}
+		if (retval) {
  			/* -ERESTARTSYS as signal hit process */
  			goto free_old_hdr;
  		}

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:30 UTC #5 lore raw

PATCH 4/10 sg: use wait_event_interruptible()

In reply to Jörn Engel

Afaics the use of __wait_event_interruptible() as opposed to wait_event_interruptible() is purely historic. So let's follow the rest of the kernel and check the condition before prepare_to_wait() - and also make the code a bit nicer.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

drivers/scsi/sg.c +7 ~~-12~~

index be812e0..1a0be4f 100644
@@ -268,9 +268,8 @@ sg_open(struct inode *inode, struct file *filp)
 			retval = -EBUSY;
 			goto error_out;
 		}
-		res = 0;
-		__wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)), res);
+		res = wait_event_interruptible(sdp->o_excl_wait,
+					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -280,9 +279,7 @@ sg_open(struct inode *inode, struct file *filp)
 			retval = -EBUSY;
 			goto error_out;
 		}
-		res = 0;
-		__wait_event_interruptible(sdp->o_excl_wait, (!sdp->exclude),
-					   res);
+		res = wait_event_interruptible(sdp->o_excl_wait, !sdp->exclude);
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -398,10 +395,9 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 			retval = -EAGAIN;
 			goto free_old_hdr;
 		}
-		retval = 0; /* following macro beats race condition */
-		__wait_event_interruptible(sfp->read_wait,
+		retval = wait_event_interruptible(sfp->read_wait,
 			(sdp->detached ||
-			(srp = sg_get_rq_mark(sfp, req_pack_id))), retval);
+			(srp = sg_get_rq_mark(sfp, req_pack_id))));
 		if (sdp->detached) {
 			retval = -ENODEV;
 			goto free_old_hdr;

@@ -797,9 +793,8 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 				 1, read_only, 1, &srp);
 		if (result < 0)
 			return result;
-		result = 0;	/* following macro to beat race condition */
-		__wait_event_interruptible(sfp->read_wait,
-			(srp->done || sdp->detached), result);
+		result = wait_event_interruptible(sfp->read_wait,
+			(srp->done || sdp->detached));
 		if (sdp->detached)
 			return -ENODEV;
 		write_lock_irq(&sfp->rq_list_lock);

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 4/10

2012-05-16 21:57 UTC #6 lore raw

Re: [PATCH 04/10] sg: use wait_event_interruptible()

Quoted 6 lines · Jörn Engel

Afaics the use of __wait_event_interruptible() as opposed to wait_event_interruptible() is purely historic. So let's follow the rest of the kernel and check the condition before prepare_to_wait() - and also make the code a bit nicer.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 46 lines

---
  drivers/scsi/sg.c |   19 +++++++------------
  1 files changed, 7 insertions(+), 12 deletions(-)

drivers/scsi/sg.c +5 -9

index be812e0..1a0be4f 100644
@@ -268,9 +268,8 @@ sg_open(struct inode *inode, struct file *filp)
  			retval = -EBUSY;
  			goto error_out;
  		}
-		res = 0;
-		__wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)), res);
+		res = wait_event_interruptible(sdp->o_excl_wait,
+					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
  		if (res) {
  			retval = res;	/* -ERESTARTSYS because signal hit process */
  			goto error_out;

@@ -280,9 +279,7 @@ sg_open(struct inode *inode, struct file *filp)
  			retval = -EBUSY;
  			goto error_out;
  		}
-		res = 0;
-		__wait_event_interruptible(sdp->o_excl_wait, (!sdp->exclude),
-					   res);
+		res = wait_event_interruptible(sdp->o_excl_wait, !sdp->exclude);
  		if (res) {
  			retval = res;	/* -ERESTARTSYS because signal hit process */
  			goto error_out;

@@ -398,10 +395,9 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
  			retval = -EAGAIN;
  			goto free_old_hdr;
  		}
-		retval = 0; /* following macro beats race condition */
-		__wait_event_interruptible(sfp->read_wait,
+		retval = wait_event_interruptible(sfp->read_wait,
  			(sdp->detached ||
-			(srp = sg_get_rq_mark(sfp, req_pack_id))), retval);
+			(srp = sg_get_rq_mark(sfp, req_pack_id))));
  		if (sdp->detached) {
  			retval = -ENODEV;
  			goto free_old_hdr;

@@ -797,9 +793,8 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  				 1, read_only, 1,&srp);

drivers/scsi/sg.c sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg) · line 800

Quoted 10 lines

drivers/scsi/sg.c +2 -3

  		if (result<  0)
  			return result;
-		result = 0;	/* following macro to beat race condition */
-		__wait_event_interruptible(sfp->read_wait,
-			(srp->done || sdp->detached), result);
+		result = wait_event_interruptible(sfp->read_wait,
+			(srp->done || sdp->detached));
  		if (sdp->detached)
  			return -ENODEV;
  		write_lock_irq(&sfp->rq_list_lock);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:30 UTC #7 lore raw

PATCH 5/10 sg: remove closed flag

In reply to Jörn Engel

After sg_release() has been called, noone should be able to actually use that filedescriptor anymore. So if closed ever made a difference in the past five years or so, it would have meant a bug. Remove it.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +3 -7

index 1a0be4f..ba657a9 100644
@@ -157,7 +157,6 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
 	char low_dma;		/* as in parent but possibly overridden to 1 */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
-	volatile char closed;	/* 1 -> fd closed but request(s) outstanding */
 	char cmd_q;		/* 1 -> allow command queuing, 0 -> don't */
 	char next_cmd_len;	/* 0 -> automatic (def), >0 -> use on next write() */
 	char keep_orphan;	/* 0 -> drop orphan (def), 1 -> keep for read() */

@@ -329,8 +328,6 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	sfp->closed = 1;
-
 	sdp->exclude = 0;
 	wake_up_interruptible(&sdp->o_excl_wait);

@@ -1118,8 +1115,7 @@ sg_poll(struct file *filp, poll_table * wait)
 	int count = 0;
 	unsigned long iflags;
 
-	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))
-	    || sfp->closed)
+	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return POLLERR;
 	poll_wait(filp, &sfp->read_wait, wait);
 	read_lock_irqsave(&sfp->rq_list_lock, iflags);

@@ -2515,9 +2511,9 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 			   fp->reserve.bufflen,
 			   (int) fp->reserve.k_use_sg,
 			   (int) fp->low_dma);
-		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=%d\n",
+		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=0\n",
 			   (int) fp->cmd_q, (int) fp->force_packid,
-			   (int) fp->keep_orphan, (int) fp->closed);
+			   (int) fp->keep_orphan);
 		for (m = 0, srp = fp->headrp;
 				srp != NULL;
 				++m, srp = srp->nextrp) {

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 5/10

2012-05-16 21:57 UTC #8 lore raw

Re: [PATCH 05/10] sg: remove closed flag

Quoted 5 lines · Jörn Engel

After sg_release() has been called, noone should be able to actually use that filedescriptor anymore. So if closed ever made a difference in the past five years or so, it would have meant a bug. Remove it.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 37 lines

---
  drivers/scsi/sg.c |   10 +++-------
  1 files changed, 3 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +1 -5

index 1a0be4f..ba657a9 100644
@@ -157,7 +157,6 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
  	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
  	char low_dma;		/* as in parent but possibly overridden to 1 */
  	char force_packid;	/* 1 ->  pack_id input to read(), 0 ->  ignored */
-	volatile char closed;	/* 1 ->  fd closed but request(s) outstanding */
  	char cmd_q;		/* 1 ->  allow command queuing, 0 ->  don't */
  	char next_cmd_len;	/* 0 ->  automatic (def),>0 ->  use on next write() */
  	char keep_orphan;	/* 0 ->  drop orphan (def), 1 ->  keep for read() */

@@ -329,8 +328,6 @@ sg_release(struct inode *inode, struct file *filp)
  		return -ENXIO;
  	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	sfp->closed = 1;
-
  	sdp->exclude = 0;
  	wake_up_interruptible(&sdp->o_excl_wait);

@@ -1118,8 +1115,7 @@ sg_poll(struct file *filp, poll_table * wait)
  	int count = 0;
  	unsigned long iflags;
 
-	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))
-	    || sfp->closed)
+	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
  		return POLLERR;
  	poll_wait(filp,&sfp->read_wait, wait);
  	read_lock_irqsave(&sfp->rq_list_lock, iflags);

@@ -2515,9 +2511,9 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
  			   fp->reserve.bufflen,

drivers/scsi/sg.c static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp) · line 2519

Quoted 10 lines

drivers/scsi/sg.c +2 -2

  			   (int) fp->reserve.k_use_sg,
  			   (int) fp->low_dma);
-		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=%d\n",
+		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=0\n",
  			   (int) fp->cmd_q, (int) fp->force_packid,
-			   (int) fp->keep_orphan, (int) fp->closed);
+			   (int) fp->keep_orphan);
  		for (m = 0, srp = fp->headrp;
  				srp != NULL;
  				++m, srp = srp->nextrp) {

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:30 UTC #9 lore raw

PATCH 6/10 sg: prevent unwoken sleep

In reply to Jörn Engel

srp->done is protected by sfp->rq_list_lock everywhere, except for this one case. Result can be that the wake-up happens before the cacheline with the changed srp->done has arrived, so the waiter can go back to sleep and never be woken up again.

The wait_event_interruptible() means that anyone trying to debug this unlikely race will likely notice everything working fine again, as the next signal will unwedge things. Evil.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

drivers/scsi/sg.c +14 -2

index ba657a9..758e7b4 100644
@@ -137,7 +137,8 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 	char res_used;		/* 1 -> using reserve buffer, 0 -> not ... */
 	char orphan;		/* 1 -> drop on sight, 0 -> normal */
 	char sg_io_owned;	/* 1 -> packet belongs to SG_IO */
-	volatile char done;	/* 0->before bh, 1->before read, 2->read */
+	/* done protected by rq_list_lock */
+	char done;		/* 0->before bh, 1->before read, 2->read */
 	struct request *rq;
 	struct bio *bio;
 	struct execute_work ew;

@@ -760,6 +761,17 @@ sg_common_write(Sg_fd * sfp, Sg_request * srp,
 	return 0;
 }
 
+static int srp_done(Sg_fd *sfp, Sg_request *srp)
+{
+	unsigned long flags;
+	int ret;
+
+	read_lock_irqsave(&sfp->rq_list_lock, flags);
+	ret = srp->done;
+	read_unlock_irqrestore(&sfp->rq_list_lock, flags);
+	return ret;
+}
+
 static int
 sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {

@@ -791,7 +803,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 		if (result < 0)
 			return result;
 		result = wait_event_interruptible(sfp->read_wait,
-			(srp->done || sdp->detached));
+			(srp_done(sfp, srp) || sdp->detached));
 		if (sdp->detached)
 			return -ENODEV;
 		write_lock_irq(&sfp->rq_list_lock);

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 6/10

2012-05-16 21:57 UTC #10 lore raw

Re: [PATCH 06/10] sg: prevent unwoken sleep

Quoted 10 lines · Jörn Engel

srp->done is protected by sfp->rq_list_lock everywhere, except for this one case. Result can be that the wake-up happens before the cacheline with the changed srp->done has arrived, so the waiter can go back to sleep and never be woken up again.
The wait_event_interruptible() means that anyone trying to debug this unlikely race will likely notice everything working fine again, as the next signal will unwedge things. Evil.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 37 lines

---
  drivers/scsi/sg.c |   16 ++++++++++++++--
  1 files changed, 14 insertions(+), 2 deletions(-)

drivers/scsi/sg.c +13 -1

index ba657a9..758e7b4 100644
@@ -137,7 +137,8 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
  	char res_used;		/* 1 ->  using reserve buffer, 0 ->  not ... */
  	char orphan;		/* 1 ->  drop on sight, 0 ->  normal */
  	char sg_io_owned;	/* 1 ->  packet belongs to SG_IO */
-	volatile char done;	/* 0->before bh, 1->before read, 2->read */
+	/* done protected by rq_list_lock */
+	char done;		/* 0->before bh, 1->before read, 2->read */
  	struct request *rq;
  	struct bio *bio;
  	struct execute_work ew;

@@ -760,6 +761,17 @@ sg_common_write(Sg_fd * sfp, Sg_request * srp,
  	return 0;
  }
 
+static int srp_done(Sg_fd *sfp, Sg_request *srp)
+{
+	unsigned long flags;
+	int ret;
+
+	read_lock_irqsave(&sfp->rq_list_lock, flags);
+	ret = srp->done;
+	read_unlock_irqrestore(&sfp->rq_list_lock, flags);
+	return ret;
+}
+
  static int
  sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  {

drivers/scsi/sg.c sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg) · line 809

Quoted 8 lines

drivers/scsi/sg.c +1 -1

@@ -791,7 +803,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  		if (result<  0)
  			return result;
  		result = wait_event_interruptible(sfp->read_wait,
-			(srp->done || sdp->detached));
+			(srp_done(sfp, srp) || sdp->detached));
  		if (sdp->detached)
  			return -ENODEV;
  		write_lock_irq(&sfp->rq_list_lock);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:30 UTC #11 lore raw

PATCH 7/10 sg: protect sdp->exclude

In reply to Jörn Engel

sdp->exclude was previously protected by the BKL. The sg_mutex, which replaced the BKL, only semi-protected it, as it was missing from sg_release() and sg_proc_seq_show_debug(). Take an explicit spinlock for it.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   36 +++++++++++++++++++++++++++++-------
 1 files changed, 29 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +29 -7

index 758e7b4..a70018e 100644
@@ -105,6 +105,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_MUTEX(sg_mutex);
+static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock

@@ -173,7 +174,8 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	u32 index;		/* device index number */
 	struct list_head sfds;
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	volatile char exclude;	/* opened for exclusive access */
+	/* exclude protected by sg_open_exclusive_lock */
+	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
 	struct cdev * cdev;	/* char_dev [sysfs: /sys/cdev/major/sg<n>] */

@@ -221,6 +223,26 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
 }
 
+static int get_exclude(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	ret = sdp->exclude;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return ret;
+}
+
+static void set_exclude(Sg_device *sdp, char val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	sdp->exclude = val;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+}
+
 static int
 sg_open(struct inode *inode, struct file *filp)
 {

@@ -269,17 +291,17 @@ sg_open(struct inode *inode, struct file *filp)
 			goto error_out;
 		}
 		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
+					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1), 1));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;
 		}
-	} else if (sdp->exclude) {	/* some other fd has an exclusive lock on dev */
+	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
 		if (flags & O_NONBLOCK) {
 			retval = -EBUSY;
 			goto error_out;
 		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !sdp->exclude);
+		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -298,7 +320,7 @@ sg_open(struct inode *inode, struct file *filp)
 		filp->private_data = sfp;
 	else {
 		if (flags & O_EXCL) {
-			sdp->exclude = 0;	/* undo if error */
+			set_exclude(sdp, 0);	/* undo if error */
 			wake_up_interruptible(&sdp->o_excl_wait);
 		}
 		retval = -ENOMEM;

@@ -329,7 +351,7 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	sdp->exclude = 0;
+	set_exclude(sdp, 0);
 	wake_up_interruptible(&sdp->o_excl_wait);
 
 	scsi_autopm_put_device(sdp->device);

@@ -2602,7 +2624,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
+			   sdp->sg_tablesize, get_exclude(sdp));
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer

2012-04-23 22:14 UTC #12 lore raw

Re: [PATCH 07/10] sg: protect sdp->exclude

Quoted 60 lines · Jörn Engel

Signed-off-by Joern Engel<[email protected]>

---
  drivers/scsi/sg.c |   36 +++++++++++++++++++++++++++++-------
  1 files changed, 29 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +23 -1

index 758e7b4..a70018e 100644
@@ -105,6 +105,7 @@ static int sg_add(struct device *, struct class_interface *);
  static void sg_remove(struct device *, struct class_interface *);
 
  static DEFINE_MUTEX(sg_mutex);
+static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
  static DEFINE_IDR(sg_index_idr);
  static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock

@@ -173,7 +174,8 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
  	u32 index;		/* device index number */
  	struct list_head sfds;
  	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	volatile char exclude;	/* opened for exclusive access */
+	/* exclude protected by sg_open_exclusive_lock */
+	char exclude;		/* opened for exclusive access */
  	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10->  all devs */
  	struct gendisk *disk;
  	struct cdev * cdev;	/* char_dev [sysfs: /sys/cdev/major/sg<n>] */

@@ -221,6 +223,26 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
  	return blk_verify_command(cmd, filp->f_mode&  FMODE_WRITE);
  }
 
+static int get_exclude(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	ret = sdp->exclude;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return ret;
+}
+
+static void set_exclude(Sg_device *sdp, char val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	sdp->exclude = val;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+}
+
  static int
  sg_open(struct inode *inode, struct file *filp)
  {

drivers/scsi/sg.c sg_open(struct inode *inode, struct file *filp) · line 294

Quoted 5 lines

drivers/scsi/sg.c +1 -1

@@ -269,17 +291,17 @@ sg_open(struct inode *inode, struct file *filp)
  			goto error_out;
  		}
  		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
+					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1), 1));

I have gone through the rest of this series of patches (10 of them posted 12 April) and they look fine. The above line worries me and I raised it with the author but have received no response.

A small test program suggests that the second argument to wait_event_interruptible [a condition] will always be true due to the trailing comma operator (i.e. the ", 1"). I suspect another set of parentheses is needed:
res = wait_event_interruptible(sdp->o_excl_wait,

             ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ?
		      0 : (set_exclude(sdp, 1), 1)));

Doug Gilbert

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-24 22:12 UTC #13 lore raw

PATCH v2 7/10 sg: protect sdp->exclude

Changes since v1: set_exclude now returns the new value, which gets rid of the comma expression and the operator precedence bug. Thanks to Douglas for spotting it.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +30 -7

index 758e7b4..e04b2a5 100644
@@ -105,6 +105,7 @@ static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
 static DEFINE_MUTEX(sg_mutex);
+static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
 static DEFINE_IDR(sg_index_idr);
 static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock

@@ -173,7 +174,8 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	u32 index;		/* device index number */
 	struct list_head sfds;
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	volatile char exclude;	/* opened for exclusive access */
+	/* exclude protected by sg_open_exclusive_lock */
+	char exclude;		/* opened for exclusive access */
 	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10-> all devs */
 	struct gendisk *disk;
 	struct cdev * cdev;	/* char_dev [sysfs: /sys/cdev/major/sg<n>] */

@@ -221,6 +223,27 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return blk_verify_command(cmd, filp->f_mode & FMODE_WRITE);
 }
 
+static int get_exclude(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	ret = sdp->exclude;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return ret;
+}
+
+static int set_exclude(Sg_device *sdp, char val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	sdp->exclude = val;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return val;
+}
+
 static int
 sg_open(struct inode *inode, struct file *filp)
 {

@@ -269,17 +292,17 @@ sg_open(struct inode *inode, struct file *filp)
 			goto error_out;
 		}
 		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
+					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;
 		}
-	} else if (sdp->exclude) {	/* some other fd has an exclusive lock on dev */
+	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
 		if (flags & O_NONBLOCK) {
 			retval = -EBUSY;
 			goto error_out;
 		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !sdp->exclude);
+		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -298,7 +321,7 @@ sg_open(struct inode *inode, struct file *filp)
 		filp->private_data = sfp;
 	else {
 		if (flags & O_EXCL) {
-			sdp->exclude = 0;	/* undo if error */
+			set_exclude(sdp, 0);	/* undo if error */
 			wake_up_interruptible(&sdp->o_excl_wait);
 		}
 		retval = -ENOMEM;

@@ -329,7 +352,7 @@ sg_release(struct inode *inode, struct file *filp)
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	sdp->exclude = 0;
+	set_exclude(sdp, 0);
 	wake_up_interruptible(&sdp->o_excl_wait);
 
 	scsi_autopm_put_device(sdp->device);

@@ -2602,7 +2625,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
 			     scsidp->lun,
 			     scsidp->host->hostt->emulated);
 		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
+			   sdp->sg_tablesize, get_exclude(sdp));
 		sg_proc_debug_helper(s, sdp);
 	}
 	read_unlock_irqrestore(&sg_index_lock, iflags);

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v2 7/10

2012-05-16 21:58 UTC #14 lore raw

Re: [PATCH v2 07/10] sg: protect sdp->exclude

Quoted 10 lines · Jörn Engel

Changes since v1: set_exclude now returns the new value, which gets rid of the comma expression and the operator precedence bug. Thanks to Douglas for spotting it.
sdp->exclude was previously protected by the BKL. The sg_mutex, which replaced the BKL, only semi-protected it, as it was missing from sg_release() and sg_proc_seq_show_debug(). Take an explicit spinlock for it.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 94 lines

---
  drivers/scsi/sg.c |   37 ++++++++++++++++++++++++++++++-------
  1 files changed, 30 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +29 -6

index 758e7b4..e04b2a5 100644
@@ -105,6 +105,7 @@ static int sg_add(struct device *, struct class_interface *);
  static void sg_remove(struct device *, struct class_interface *);
 
  static DEFINE_MUTEX(sg_mutex);
+static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
  static DEFINE_IDR(sg_index_idr);
  static DEFINE_RWLOCK(sg_index_lock);	/* Also used to lock

@@ -173,7 +174,8 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
  	u32 index;		/* device index number */
  	struct list_head sfds;
  	volatile char detached;	/* 0->attached, 1->detached pending removal */
-	volatile char exclude;	/* opened for exclusive access */
+	/* exclude protected by sg_open_exclusive_lock */
+	char exclude;		/* opened for exclusive access */
  	char sgdebug;		/* 0->off, 1->sense, 9->dump dev, 10->  all devs */
  	struct gendisk *disk;
  	struct cdev * cdev;	/* char_dev [sysfs: /sys/cdev/major/sg<n>] */

@@ -221,6 +223,27 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
  	return blk_verify_command(cmd, filp->f_mode&  FMODE_WRITE);
  }
 
+static int get_exclude(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	ret = sdp->exclude;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return ret;
+}
+
+static int set_exclude(Sg_device *sdp, char val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sg_open_exclusive_lock, flags);
+	sdp->exclude = val;
+	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
+	return val;
+}
+
  static int
  sg_open(struct inode *inode, struct file *filp)
  {

@@ -269,17 +292,17 @@ sg_open(struct inode *inode, struct file *filp)
  			goto error_out;
  		}
  		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || sdp->exclude) ? 0 : (sdp->exclude = 1)));
+					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
  		if (res) {
  			retval = res;	/* -ERESTARTSYS because signal hit process */
  			goto error_out;
  		}
-	} else if (sdp->exclude) {	/* some other fd has an exclusive lock on dev */
+	} else if (get_exclude(sdp)) {	/* some other fd has an exclusive lock on dev */
  		if (flags&  O_NONBLOCK) {
  			retval = -EBUSY;
  			goto error_out;
  		}
-		res = wait_event_interruptible(sdp->o_excl_wait, !sdp->exclude);
+		res = wait_event_interruptible(sdp->o_excl_wait, !get_exclude(sdp));
  		if (res) {
  			retval = res;	/* -ERESTARTSYS because signal hit process */
  			goto error_out;

@@ -298,7 +321,7 @@ sg_open(struct inode *inode, struct file *filp)
  		filp->private_data = sfp;
  	else {
  		if (flags&  O_EXCL) {
-			sdp->exclude = 0;	/* undo if error */
+			set_exclude(sdp, 0);	/* undo if error */
  			wake_up_interruptible(&sdp->o_excl_wait);
  		}
  		retval = -ENOMEM;

@@ -329,7 +352,7 @@ sg_release(struct inode *inode, struct file *filp)
  		return -ENXIO;
  	SCSI_LOG_TIMEOUT(3, printk("sg_release: %s\n", sdp->disk->disk_name));
 
-	sdp->exclude = 0;
+	set_exclude(sdp, 0);
  	wake_up_interruptible(&sdp->o_excl_wait);
 
  	scsi_autopm_put_device(sdp->device);

drivers/scsi/sg.c static int sg_proc_seq_show_debug(struct seq_file *s, void *v) · line 2631

Quoted 8 lines

drivers/scsi/sg.c +1 -1

@@ -2602,7 +2625,7 @@ static int sg_proc_seq_show_debug(struct seq_file *s, void *v)
  			     scsidp->lun,
  			     scsidp->host->hostt->emulated);
  		seq_printf(s, " sg_tablesize=%d excl=%d\n",
-			   sdp->sg_tablesize, sdp->exclude);
+			   sdp->sg_tablesize, get_exclude(sdp));
  		sg_proc_debug_helper(s, sdp);
  	}
  	read_unlock_irqrestore(&sg_index_lock, iflags);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-24 22:16 UTC #15 lore raw

Re: [PATCH 07/10] sg: protect sdp->exclude

In reply to Douglas Gilbert

Quoted 5 lines · Douglas Gilbert

I have gone through the rest of this series of patches (10 of them posted 12 April) and they look fine. The above line worries me and I raised it with the author but have received no response.

Quoted 7 lines

A small test program suggests that the second argument to wait_event_interruptible [a condition] will always be true due to the trailing comma operator (i.e. the ", 1"). I suspect another set of parentheses is needed:
res = wait_event_interruptible(sdp->o_excl_wait,
             ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ?
		      0 : (set_exclude(sdp, 1), 1)));

You are right. I made set_exclude() return the new value, which simplifies this condition a bit. Any simplification here is a good thing, as I just proved that it is too complicated for my brain to fully understand.

Thanks for spotting this, Doug!

Jörn
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:31 UTC #16 lore raw

PATCH 8/10 sg: completely protect sfds

In reply to Jörn Engel

sfds is protected by sg_index_lock - except for sg_open(), where it isn't. Change that and add some documentation.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/scsi/sg.c +16 -3

index a70018e..a40b814 100644
@@ -146,6 +146,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
+	/* sfd_siblings is protected by sg_index_lock */
 	struct list_head sfd_siblings;
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */

@@ -172,6 +173,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
+	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */

@@ -243,6 +245,17 @@ static void set_exclude(Sg_device *sdp, char val)
 	spin_unlock_irqrestore(&sg_open_exclusive_lock, flags);
 }
 
+static int sfds_list_empty(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	read_lock_irqsave(&sg_index_lock, flags);
+	ret = list_empty(&sdp->sfds);
+	read_unlock_irqrestore(&sg_index_lock, flags);
+	return ret;
+}
+
 static int
 sg_open(struct inode *inode, struct file *filp)
 {

@@ -286,12 +299,12 @@ sg_open(struct inode *inode, struct file *filp)
 			retval = -EPERM; /* Can't lock it with read only access */
 			goto error_out;
 		}
-		if (!list_empty(&sdp->sfds) && (flags & O_NONBLOCK)) {
+		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
 			retval = -EBUSY;
 			goto error_out;
 		}
 		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1), 1));
+					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1), 1));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -311,7 +324,7 @@ sg_open(struct inode *inode, struct file *filp)
 		retval = -ENODEV;
 		goto error_out;
 	}
-	if (list_empty(&sdp->sfds)) {	/* no existing opens on this device */
+	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);

Signature · 1 line

1.7.9.1

Jörn Engel

author

2012-04-25 17:17 UTC #17 lore raw

PATCH v2 8/10 sg: completely protect sfds

sfds is protected by sg_index_lock - except for sg_open(), where it isn't. Change that and add some documentation.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/scsi/sg.c +16 -3

index e04b2a5..bb0514d 100644
@@ -146,6 +146,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
 } Sg_request;
 
 typedef struct sg_fd {		/* holds the state of a file descriptor */
+	/* sfd_siblings is protected by sg_index_lock */
 	struct list_head sfd_siblings;
 	struct sg_device *parentdp;	/* owning device */
 	wait_queue_head_t read_wait;	/* queue read until command done */

@@ -172,6 +173,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
 	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
 	int sg_tablesize;	/* adapter's max scatter-gather table size */
 	u32 index;		/* device index number */
+	/* sfds is protected by sg_index_lock */
 	struct list_head sfds;
 	volatile char detached;	/* 0->attached, 1->detached pending removal */
 	/* exclude protected by sg_open_exclusive_lock */

@@ -244,6 +246,17 @@ static int set_exclude(Sg_device *sdp, char val)
 	return val;
 }
 
+static int sfds_list_empty(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	read_lock_irqsave(&sg_index_lock, flags);
+	ret = list_empty(&sdp->sfds);
+	read_unlock_irqrestore(&sg_index_lock, flags);
+	return ret;
+}
+
 static int
 sg_open(struct inode *inode, struct file *filp)
 {

@@ -287,12 +300,12 @@ sg_open(struct inode *inode, struct file *filp)
 			retval = -EPERM; /* Can't lock it with read only access */
 			goto error_out;
 		}
-		if (!list_empty(&sdp->sfds) && (flags & O_NONBLOCK)) {
+		if (!sfds_list_empty(sdp) && (flags & O_NONBLOCK)) {
 			retval = -EBUSY;
 			goto error_out;
 		}
 		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
+					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
 		if (res) {
 			retval = res;	/* -ERESTARTSYS because signal hit process */
 			goto error_out;

@@ -312,7 +325,7 @@ sg_open(struct inode *inode, struct file *filp)
 		retval = -ENODEV;
 		goto error_out;
 	}
-	if (list_empty(&sdp->sfds)) {	/* no existing opens on this device */
+	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
 		sdp->sgdebug = 0;
 		q = sdp->device->request_queue;
 		sdp->sg_tablesize = queue_max_segments(q);

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v2 8/10

2012-05-16 21:58 UTC #18 lore raw

Re: [PATCH v2 08/10] sg: completely protect sfds

Quoted 4 lines · Jörn Engel

sfds is protected by sg_index_lock - except for sg_open(), where it isn't. Change that and add some documentation.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 58 lines

---
  drivers/scsi/sg.c |   19 ++++++++++++++++---
  1 files changed, 16 insertions(+), 3 deletions(-)

drivers/scsi/sg.c +15 -2

index e04b2a5..bb0514d 100644
@@ -146,6 +146,7 @@ typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
  } Sg_request;
 
  typedef struct sg_fd {		/* holds the state of a file descriptor */
+	/* sfd_siblings is protected by sg_index_lock */
  	struct list_head sfd_siblings;
  	struct sg_device *parentdp;	/* owning device */
  	wait_queue_head_t read_wait;	/* queue read until command done */

@@ -172,6 +173,7 @@ typedef struct sg_device { /* holds the state of each scsi generic device */
  	wait_queue_head_t o_excl_wait;	/* queue open() when O_EXCL in use */
  	int sg_tablesize;	/* adapter's max scatter-gather table size */
  	u32 index;		/* device index number */
+	/* sfds is protected by sg_index_lock */
  	struct list_head sfds;
  	volatile char detached;	/* 0->attached, 1->detached pending removal */
  	/* exclude protected by sg_open_exclusive_lock */

@@ -244,6 +246,17 @@ static int set_exclude(Sg_device *sdp, char val)
  	return val;
  }
 
+static int sfds_list_empty(Sg_device *sdp)
+{
+	unsigned long flags;
+	int ret;
+
+	read_lock_irqsave(&sg_index_lock, flags);
+	ret = list_empty(&sdp->sfds);
+	read_unlock_irqrestore(&sg_index_lock, flags);
+	return ret;
+}
+
  static int
  sg_open(struct inode *inode, struct file *filp)
  {

@@ -287,12 +300,12 @@ sg_open(struct inode *inode, struct file *filp)
  			retval = -EPERM; /* Can't lock it with read only access */
  			goto error_out;
  		}
-		if (!list_empty(&sdp->sfds)&&  (flags&  O_NONBLOCK)) {
+		if (!sfds_list_empty(sdp)&&  (flags&  O_NONBLOCK)) {
  			retval = -EBUSY;
  			goto error_out;
  		}
  		res = wait_event_interruptible(sdp->o_excl_wait,
-					   ((!list_empty(&sdp->sfds) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
+					   ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)));
  		if (res) {
  			retval = res;	/* -ERESTARTSYS because signal hit process */
  			goto error_out;

drivers/scsi/sg.c sg_open(struct inode *inode, struct file *filp) · line 331

Quoted 8 lines

drivers/scsi/sg.c +1 -1

@@ -312,7 +325,7 @@ sg_open(struct inode *inode, struct file *filp)
  		retval = -ENODEV;
  		goto error_out;
  	}
-	if (list_empty(&sdp->sfds)) {	/* no existing opens on this device */
+	if (sfds_list_empty(sdp)) {	/* no existing opens on this device */
  		sdp->sgdebug = 0;
  		q = sdp->device->request_queue;
  		sdp->sg_tablesize = queue_max_segments(q);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:31 UTC #19 lore raw

PATCH 9/10 sg: remove sg_mutex

In reply to Jörn Engel

With the exception of the detached field, sg_mutex no longer adds any locking. detached handling has been broken before and is still broken and this patch does not seem to make things worse than they were to begin with.

However, I have observed cases of tasks being blocked for >200s waiting for sg_mutex. So the removal clearly adds value for very little cost.

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

drivers/scsi/sg.c +2 ~~-17~~

index a40b814..0c646f2 100644
@@ -104,7 +104,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 static int sg_add(struct device *, struct class_interface *);
 static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_MUTEX(sg_mutex);
 static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
 static DEFINE_IDR(sg_index_idr);

@@ -267,7 +266,6 @@ sg_open(struct inode *inode, struct file *filp)
 	int res;
 	int retval;
 
-	mutex_lock(&sg_mutex);
 	nonseekable_open(inode, filp);
 	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
 	sdp = sg_get_dev(dev);

@@ -349,7 +347,6 @@ sdp_put:
 sg_put:
 	if (sdp)
 		sg_put_dev(sdp);
-	mutex_unlock(&sg_mutex);
 	return retval;
 }

@@ -807,7 +804,7 @@ static int srp_done(Sg_fd *sfp, Sg_request *srp)
 	return ret;
 }
 
-static int
+static long
 sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
 	void __user *p = (void __user *)arg;

@@ -1117,18 +1114,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 	}
 }
 
-static long
-sg_unlocked_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
-{
-	int ret;
-
-	mutex_lock(&sg_mutex);
-	ret = sg_ioctl(filp, cmd_in, arg);
-	mutex_unlock(&sg_mutex);
-
-	return ret;
-}
-
 #ifdef CONFIG_COMPAT
 static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {

@@ -1372,7 +1357,7 @@ static const struct file_operations sg_fops = {
 	.read = sg_read,
 	.write = sg_write,
 	.poll = sg_poll,
-	.unlocked_ioctl = sg_unlocked_ioctl,
+	.unlocked_ioctl = sg_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = sg_compat_ioctl,
 #endif

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 9/10

2012-05-16 21:58 UTC #20 lore raw

Re: [PATCH 09/10] sg: remove sg_mutex

Quoted 9 lines · Jörn Engel

With the exception of the detached field, sg_mutex no longer adds any locking. detached handling has been broken before and is still broken and this patch does not seem to make things worse than they were to begin with.
However, I have observed cases of tasks being blocked for>200s waiting for sg_mutex. So the removal clearly adds value for very little cost.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 61 lines

---
  drivers/scsi/sg.c |   19 ++-----------------
  1 files changed, 2 insertions(+), 17 deletions(-)

drivers/scsi/sg.c +1 ~~-16~~

index a40b814..0c646f2 100644
@@ -104,7 +104,6 @@ static int scatter_elem_sz_prev = SG_SCATTER_SZ;
  static int sg_add(struct device *, struct class_interface *);
  static void sg_remove(struct device *, struct class_interface *);
 
-static DEFINE_MUTEX(sg_mutex);
  static DEFINE_SPINLOCK(sg_open_exclusive_lock);
 
  static DEFINE_IDR(sg_index_idr);

@@ -267,7 +266,6 @@ sg_open(struct inode *inode, struct file *filp)
  	int res;
  	int retval;
 
-	mutex_lock(&sg_mutex);
  	nonseekable_open(inode, filp);
  	SCSI_LOG_TIMEOUT(3, printk("sg_open: dev=%d, flags=0x%x\n", dev, flags));
  	sdp = sg_get_dev(dev);

@@ -349,7 +347,6 @@ sdp_put:
  sg_put:
  	if (sdp)
  		sg_put_dev(sdp);
-	mutex_unlock(&sg_mutex);
  	return retval;
  }

@@ -807,7 +804,7 @@ static int srp_done(Sg_fd *sfp, Sg_request *srp)
  	return ret;
  }
 
-static int
+static long
  sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  {
  	void __user *p = (void __user *)arg;

@@ -1117,18 +1114,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  	}
  }
 
-static long
-sg_unlocked_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
-{
-	int ret;
-
-	mutex_lock(&sg_mutex);
-	ret = sg_ioctl(filp, cmd_in, arg);
-	mutex_unlock(&sg_mutex);
-
-	return ret;
-}
-
  #ifdef CONFIG_COMPAT
  static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
  {

drivers/scsi/sg.c static const struct file_operations sg_fops = { · line 1363

Quoted 8 lines

drivers/scsi/sg.c +1 -1

@@ -1372,7 +1357,7 @@ static const struct file_operations sg_fops = {
  	.read = sg_read,
  	.write = sg_write,
  	.poll = sg_poll,
-	.unlocked_ioctl = sg_unlocked_ioctl,
+	.unlocked_ioctl = sg_ioctl,
  #ifdef CONFIG_COMPAT
  	.compat_ioctl = sg_compat_ioctl,
  #endif

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-04-12 23:32 UTC #21 lore raw

PATCH 10/10 sg: constify sg_proc_leaf_arr

In reply to Jörn Engel

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/sg.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/sg.c +2 -2

index 0c646f2..5790358 100644
@@ -2322,7 +2322,7 @@ struct sg_proc_leaf {
 	const struct file_operations * fops;
 };
 
-static struct sg_proc_leaf sg_proc_leaf_arr[] = {
+static const struct sg_proc_leaf sg_proc_leaf_arr[] = {
 	{"allow_dio", &adio_fops},
 	{"debug", &debug_fops},
 	{"def_reserved_size", &dressz_fops},

@@ -2342,7 +2342,7 @@ sg_proc_init(void)
 	if (!sg_proc_sgp)
 		return 1;
 	for (k = 0; k < num_leaves; ++k) {
-		struct sg_proc_leaf *leaf = &sg_proc_leaf_arr[k];
+		const struct sg_proc_leaf *leaf = &sg_proc_leaf_arr[k];
 		umode_t mask = leaf->fops->write ? S_IRUGO | S_IWUSR : S_IRUGO;
 		proc_create(leaf->name, mask, sg_proc_sgp, leaf->fops);
 	}

Signature · 1 line

1.7.9.1

Douglas Gilbert

maintainer Acked-by on v1 10/10

2012-05-16 21:58 UTC #22 lore raw

Re: [PATCH 10/10] sg: constify sg_proc_leaf_arr

Quoted 1 line · Jörn Engel

Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 18 lines

---
  drivers/scsi/sg.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/sg.c +1 -1

index 0c646f2..5790358 100644
@@ -2322,7 +2322,7 @@ struct sg_proc_leaf {
  	const struct file_operations * fops;
  };
 
-static struct sg_proc_leaf sg_proc_leaf_arr[] = {
+static const struct sg_proc_leaf sg_proc_leaf_arr[] = {
  	{"allow_dio",&adio_fops},
  	{"debug",&debug_fops},
  	{"def_reserved_size",&dressz_fops},

drivers/scsi/sg.c sg_proc_init(void) · line 2348

Quoted 8 lines

drivers/scsi/sg.c +1 -1

@@ -2342,7 +2342,7 @@ sg_proc_init(void)
  	if (!sg_proc_sgp)
  		return 1;
  	for (k = 0; k<  num_leaves; ++k) {
-		struct sg_proc_leaf *leaf =&sg_proc_leaf_arr[k];
+		const struct sg_proc_leaf *leaf =&sg_proc_leaf_arr[k];
  		umode_t mask = leaf->fops->write ? S_IRUGO | S_IWUSR : S_IRUGO;
  		proc_create(leaf->name, mask, sg_proc_sgp, leaf->fops);
  	}

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-05-07 21:47 UTC #23 lore raw

In reply to Jörn Engel

Quoted 4 lines · Jörn Engel

blocking is de-facto a constant and the now-removed comment wasn't all that useful either. Without them and the resulting indentation the code is a bit nicer to read.

Ping.

This patchset, amongst other things, solves the issue of tasks hanging for >200s. Strictly those hung tasks were a regression introduced when Arnd removed the BKL. So someone should merge them.

James, would that someone be you?

Jörn

--
tglx1 thinks that joern should get a (TM) for "Thinking Is Hard" -- Thomas Gleixner
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2012-05-11 16:47 UTC #24 lore raw

Quoted 3 lines · Jörn Engel

On Thu, 12 April 2012 17:32:17 -0400, Jörn Engel wrote:
Ping.

Ping^2

The patchset has been acked (in private mail, sadly) by Doug and fixes a regression introduced in c45d15d24e. I would really like this to go in through whatever path.

James, are you interested in this? Should I resend directly to Linus?

Jörn

--
If you're willing to restrict the flexibility of your approach, you can almost always do something better.
-- John Carmack
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Douglas Gilbert

maintainer Acked-by on v1 1/10

2012-05-16 21:57 UTC #25 lore raw

In reply to Jörn Engel

Quoted 5 lines · Jörn Engel

blocking is de-facto a constant and the now-removed comment wasn't all that useful either. Without them and the resulting indentation the code is a bit nicer to read.
Signed-off-by Joern Engel<[email protected]>

Acked-by Douglas Gilbert <[email protected]>

Quoted 63 lines

---
  drivers/scsi/sg.c |   53 ++++++++++++++++++++++++-----------------------------
  1 files changed, 24 insertions(+), 29 deletions(-)

drivers/scsi/sg.c +19 ~~-28~~

index eacd46b..14de843 100644
@@ -791,40 +791,35 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 
  	switch (cmd_in) {
  	case SG_IO:
-		{
-			int blocking = 1;	/* ignore O_NONBLOCK flag */
-
+		if (sdp->detached)
+			return -ENODEV;
+		if (!scsi_block_when_processing_errors(sdp->device))
+			return -ENXIO;
+		if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
+			return -EFAULT;
+		result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
+				 1, read_only, 1,&srp);
+		if (result<  0)
+			return result;
+		while (1) {
+			result = 0;	/* following macro to beat race condition */
+			__wait_event_interruptible(sfp->read_wait,
+				(srp->done || sdp->detached),
+				result);
  			if (sdp->detached)
  				return -ENODEV;
-			if (!scsi_block_when_processing_errors(sdp->device))
-				return -ENXIO;
-			if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
-				return -EFAULT;
-			result =
-			    sg_new_write(sfp, filp, p, SZ_SG_IO_HDR,
-					 blocking, read_only, 1,&srp);
-			if (result<  0)
-				return result;
-			while (1) {
-				result = 0;	/* following macro to beat race condition */
-				__wait_event_interruptible(sfp->read_wait,
-					(srp->done || sdp->detached),
-					result);
-				if (sdp->detached)
-					return -ENODEV;
-				write_lock_irq(&sfp->rq_list_lock);
-				if (srp->done) {
-					srp->done = 2;
-					write_unlock_irq(&sfp->rq_list_lock);
-					break;
-				}
-				srp->orphan = 1;
+			write_lock_irq(&sfp->rq_list_lock);
+			if (srp->done) {
+				srp->done = 2;
  				write_unlock_irq(&sfp->rq_list_lock);
-				return result;	/* -ERESTARTSYS because signal hit process */
+				break;
  			}
-			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);

drivers/scsi/sg.c sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg) · line 825

Quoted 10 lines

drivers/scsi/sg.c +5 -1

-			return (result<  0) ? result : 0;
+			srp->orphan = 1;
+			write_unlock_irq(&sfp->rq_list_lock);
+			return result;	/* -ERESTARTSYS because signal hit process */
  		}
+		result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
+		return (result<  0) ? result : 0;
  	case SG_SET_TIMEOUT:
  		result = get_user(val, ip);
  		if (result)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html