[PATCH 01/10] sg: remove unnecessary indentation

[PATCH 01/10] sg: remove unnecessary indentation

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   53 ++++++++++++++++++++++++-----------------------------
 1 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index eacd46b..14de843 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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)
-- 
1.7.9.1

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 14de843..f9e89b2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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)
-- 
1.7.9.1

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f9e89b2..be812e0 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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;
 		}
-- 
1.7.9.1

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index be812e0..1a0be4f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

[PATCH 05/10] sg: remove closed flag

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 1a0be4f..ba657a9 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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) {
-- 
1.7.9.1

[PATCH 06/10] sg: prevent unwoken sleep

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ba657a9..758e7b4 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   36 +++++++++++++++++++++++++++++-------
 1 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 758e7b4..a70018e 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

[PATCH 08/10] sg: completely protect sfds

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index a70018e..a40b814 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

[PATCH 09/10] sg: remove sg_mutex

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index a40b814..0c646f2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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
-- 
1.7.9.1

[PATCH 10/10] sg: constify sg_proc_leaf_arr

[Jörn Engel]

Signed-off-by: Joern Engel <joern@logfs.org>
---
 drivers/scsi/sg.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 0c646f2..5790358 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
 	}
-- 
1.7.9.1

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

[Douglas Gilbert]

On 12-04-12 05:34 PM, Jörn Engel wrote:
> 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<joern@logfs.org>
> ---
>   drivers/scsi/sg.c |   36 +++++++++++++++++++++++++++++-------
>   1 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 758e7b4..a70018e 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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));

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 758e7b4..e04b2a5 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

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

[Jörn Engel]

On Mon, 23 April 2012 18:13:58 -0400, Douglas Gilbert wrote:
> 
> 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)));

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 <joern@logfs.org>
---
 drivers/scsi/sg.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index e04b2a5..bb0514d 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -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);
-- 
1.7.9.1

Re: [PATCH 01/10] sg: remove unnecessary indentation

[Jörn Engel]

On Thu, 12 April 2012 17:32:17 -0400, Jörn Engel wrote:
> 
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] sg: remove unnecessary indentation

[Jörn Engel]

On Mon, 7 May 2012 15:44:50 -0400, Jörn Engel wrote:
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] sg: remove unnecessary indentation

[Douglas Gilbert]

On 12-04-12 05:32 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   53 ++++++++++++++++++++++++-----------------------------
>   1 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index eacd46b..14de843 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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)

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

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

[Douglas Gilbert]

On 12-04-12 05:32 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   30 +++++++++++++-----------------
>   1 files changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 14de843..f9e89b2 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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)

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

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

[Douglas Gilbert]

On 12-04-12 05:33 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   22 +++++++++-------------
>   1 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index f9e89b2..be812e0 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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;
>   		}

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

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

[Douglas Gilbert]

On 12-04-12 05:33 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   19 +++++++------------
>   1 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index be812e0..1a0be4f 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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);

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

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

[Douglas Gilbert]

On 12-04-12 05:33 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   10 +++-------
>   1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 1a0be4f..ba657a9 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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) {

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

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

[Douglas Gilbert]

On 12-04-12 05:33 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   16 ++++++++++++++--
>   1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index ba657a9..758e7b4 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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);

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

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

[Douglas Gilbert]

On 12-04-24 04:13 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   37 ++++++++++++++++++++++++++++++-------
>   1 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 758e7b4..e04b2a5 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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);

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

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

[Douglas Gilbert]

On 12-04-25 11:17 AM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   19 ++++++++++++++++---
>   1 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index e04b2a5..bb0514d 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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);

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

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

[Douglas Gilbert]

On 12-04-12 05:35 PM, Jörn Engel wrote:
> 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<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |   19 ++-----------------
>   1 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index a40b814..0c646f2 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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

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

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

[Douglas Gilbert]

On 12-04-12 05:35 PM, Jörn Engel wrote:
> Signed-off-by: Joern Engel<joern@logfs.org>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 0c646f2..5790358 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -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);
>   	}

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