[RFC] migrate ch.c away from semaphores

[RFC] migrate ch.c away from semaphores

[Jes Sorensen]

Hi,

I have been migrating some files away from semaphores to the new mutex
code and had a go at ch.c as well. I made a few other changes which I
was hoping someone would comment upon, or at least yell if they
disagree.

Gerd I am not sure if you still maintain this driver but since you're
listed as the author you get the first blame or chance to yell ;-)

If nobody complains then I'll pass it on to Ingo.

Cheers,
Jes

Migrate drivers/scsi/ch.c to use a mutex rather than a semaphore. Fix
up allocations to be GFP_KERNEL and not GFP_DMA as it's really the job
of the device driver to figure out if it can handle the buffer or not,
not a mid layer protocol driver. Last, fix case where loop would
continue even though put_user() fails.

Signed-off-by: Jes Sorensen <jes@sgi.com>

----

 drivers/scsi/ch.c |   51 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 28 insertions(+), 23 deletions(-)

Index: linux-2.6.15-rc7-quilt/drivers/scsi/ch.c
===================================================================
--- linux-2.6.15-rc7-quilt.orig/drivers/scsi/ch.c
+++ linux-2.6.15-rc7-quilt/drivers/scsi/ch.c
@@ -19,7 +19,7 @@
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
-#include <linux/completion.h>
+#include <linux/mutex.h>
 #include <linux/ioctl32.h>
 #include <linux/compat.h>
 #include <linux/chio.h>			/* here are all the ioctls */
@@ -112,7 +112,7 @@
 	u_int               counts[CH_TYPES];
 	u_int               unit_attention;
 	u_int		    voltags;
-	struct semaphore    lock;
+	struct mutex	    lock;
 } scsi_changer;
 
 static LIST_HEAD(ch_devlist);
@@ -263,7 +263,7 @@
 	u_char  *buffer;
 	int     result;
 	
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if(!buffer)
 		return -ENOMEM;
 	
@@ -320,7 +320,7 @@
 	int     result,id,lun,i;
 	u_int   elem;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 	memset(buffer,0,512);
@@ -563,17 +563,21 @@
 static int ch_gstatus(scsi_changer *ch, int type, unsigned char __user *dest)
 {
 	int retval = 0;
-	u_char data[16];
+	u8 data[16];
 	unsigned int i;
-	
-	down(&ch->lock);
+
+	mutex_lock(&ch->lock);
 	for (i = 0; i < ch->counts[type]; i++) {
 		if (0 != ch_read_element_status
 		    (ch, ch->firsts[type]+i,data)) {
 			retval = -EIO;
 			break;
 		}
-		put_user(data[2], dest+i);
+		if (put_user(data[2], dest+i)) {
+			retval = -EFAULT;
+			goto out;
+		}
+			
 		if (data[2] & CESTATUS_EXCEPT)
 			vprintk("element 0x%x: asc=0x%x, ascq=0x%x\n",
 				ch->firsts[type]+i,
@@ -583,7 +587,8 @@
 		if (0 != retval)
 			break;
 	}
-	up(&ch->lock);
+ out:
+	mutex_unlock(&ch->lock);
 	return retval;
 }
 
@@ -688,11 +693,11 @@
 			dprintk("CHIOPOSITION: invalid parameter\n");
 			return -EBADSLT;
 		}
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_position(ch,0,
 				     ch->firsts[pos.cp_type] + pos.cp_unit,
 				     pos.cp_flags & CP_INVERT);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 	
@@ -709,12 +714,12 @@
 			return -EBADSLT;
 		}
 		
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_move(ch,0,
 				 ch->firsts[mv.cm_fromtype] + mv.cm_fromunit,
 				 ch->firsts[mv.cm_totype]   + mv.cm_tounit,
 				 mv.cm_flags & CM_INVERT);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -732,14 +737,14 @@
 			return -EBADSLT;
 		}
 		
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_exchange
 			(ch,0,
 			 ch->firsts[mv.ce_srctype]  + mv.ce_srcunit,
 			 ch->firsts[mv.ce_fdsttype] + mv.ce_fdstunit,
 			 ch->firsts[mv.ce_sdsttype] + mv.ce_sdstunit,
 			 mv.ce_flags & CE_INVERT1, mv.ce_flags & CE_INVERT2);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -770,10 +775,10 @@
 			return -EINVAL;
 		elem = ch->firsts[cge.cge_type] + cge.cge_unit;
 		
-		buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+		buffer = kmalloc(512, GFP_KERNEL);
 		if (!buffer)
 			return -ENOMEM;
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		
 	voltag_retry:
 		memset(cmd,0,sizeof(cmd));
@@ -823,8 +828,8 @@
 			vprintk("device has no volume tag support\n");
 			goto voltag_retry;
 		}
+		mutex_unlock(&ch->lock);
 		kfree(buffer);
-		up(&ch->lock);
 		
 		if (copy_to_user(argp, &cge, sizeof (cge)))
 			return -EFAULT;
@@ -833,9 +838,9 @@
 
 	case CHIOINITELEM:
 	{
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_init_elem(ch);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 		
@@ -852,12 +857,12 @@
 			return -EBADSLT;
 		}
 		elem = ch->firsts[csv.csv_type] + csv.csv_unit;
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_set_voltag(ch, elem,
 				       csv.csv_flags & CSV_AVOLTAG,
 				       csv.csv_flags & CSV_CLEARTAG,
 				       csv.csv_voltag);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -930,7 +935,7 @@
 	memset(ch,0,sizeof(*ch));
 	ch->minor = ch_devcount;
 	sprintf(ch->name,"ch%d",ch->minor);
-	init_MUTEX(&ch->lock);
+	mutex_init(&ch->lock);
 	ch->device = sd;
 	ch_readconfig(ch);
 	if (init)

Re: [RFC] migrate ch.c away from semaphores

[Jes Sorensen]

>>>>> "Jes" == Jes Sorensen <jes@trained-monkey.org> writes:

Jes> Hi, I have been migrating some files away from semaphores to the
Jes> new mutex code and had a go at ch.c as well. I made a few other
Jes> changes which I was hoping someone would comment upon, or at
Jes> least yell if they disagree.

Jes> Gerd I am not sure if you still maintain this driver but since
Jes> you're listed as the author you get the first blame or chance to
Jes> yell ;-)

Hmmmm James moaned at me over the GFP_DMA change so I am pulling that
out for now. Here's the current patch.

Jes

Migrate drivers/scsi/ch.c to use a mutex rather than a
semaphore. Last, fix case where loop would continue even though
put_user() fails.

Signed-off-by: Jes Sorensen <jes@sgi.com>

----

 drivers/scsi/ch.c |   47 ++++++++++++++++++++++++++---------------------
 1 files changed, 26 insertions(+), 21 deletions(-)

Index: linux-2.6.15-rc7-quilt/drivers/scsi/ch.c
===================================================================
--- linux-2.6.15-rc7-quilt.orig/drivers/scsi/ch.c
+++ linux-2.6.15-rc7-quilt/drivers/scsi/ch.c
@@ -19,7 +19,7 @@
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
-#include <linux/completion.h>
+#include <linux/mutex.h>
 #include <linux/ioctl32.h>
 #include <linux/compat.h>
 #include <linux/chio.h>			/* here are all the ioctls */
@@ -112,7 +112,7 @@
 	u_int               counts[CH_TYPES];
 	u_int               unit_attention;
 	u_int		    voltags;
-	struct semaphore    lock;
+	struct mutex	    lock;
 } scsi_changer;
 
 static LIST_HEAD(ch_devlist);
@@ -563,17 +563,21 @@
 static int ch_gstatus(scsi_changer *ch, int type, unsigned char __user *dest)
 {
 	int retval = 0;
-	u_char data[16];
+	u8 data[16];
 	unsigned int i;
-	
-	down(&ch->lock);
+
+	mutex_lock(&ch->lock);
 	for (i = 0; i < ch->counts[type]; i++) {
 		if (0 != ch_read_element_status
 		    (ch, ch->firsts[type]+i,data)) {
 			retval = -EIO;
 			break;
 		}
-		put_user(data[2], dest+i);
+		if (put_user(data[2], dest+i)) {
+			retval = -EFAULT;
+			goto out;
+		}
+
 		if (data[2] & CESTATUS_EXCEPT)
 			vprintk("element 0x%x: asc=0x%x, ascq=0x%x\n",
 				ch->firsts[type]+i,
@@ -583,7 +587,8 @@
 		if (0 != retval)
 			break;
 	}
-	up(&ch->lock);
+ out:
+	mutex_unlock(&ch->lock);
 	return retval;
 }
 
@@ -688,11 +693,11 @@
 			dprintk("CHIOPOSITION: invalid parameter\n");
 			return -EBADSLT;
 		}
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_position(ch,0,
 				     ch->firsts[pos.cp_type] + pos.cp_unit,
 				     pos.cp_flags & CP_INVERT);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 	
@@ -709,12 +714,12 @@
 			return -EBADSLT;
 		}
 		
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_move(ch,0,
 				 ch->firsts[mv.cm_fromtype] + mv.cm_fromunit,
 				 ch->firsts[mv.cm_totype]   + mv.cm_tounit,
 				 mv.cm_flags & CM_INVERT);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -732,14 +737,14 @@
 			return -EBADSLT;
 		}
 		
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_exchange
 			(ch,0,
 			 ch->firsts[mv.ce_srctype]  + mv.ce_srcunit,
 			 ch->firsts[mv.ce_fdsttype] + mv.ce_fdstunit,
 			 ch->firsts[mv.ce_sdsttype] + mv.ce_sdstunit,
 			 mv.ce_flags & CE_INVERT1, mv.ce_flags & CE_INVERT2);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -773,8 +778,8 @@
 		buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
 		if (!buffer)
 			return -ENOMEM;
-		down(&ch->lock);
-		
+		mutex_lock(&ch->lock);
+
 	voltag_retry:
 		memset(cmd,0,sizeof(cmd));
 		cmd[0] = READ_ELEMENT_STATUS;
@@ -823,8 +828,8 @@
 			vprintk("device has no volume tag support\n");
 			goto voltag_retry;
 		}
+		mutex_unlock(&ch->lock);
 		kfree(buffer);
-		up(&ch->lock);
 		
 		if (copy_to_user(argp, &cge, sizeof (cge)))
 			return -EFAULT;
@@ -833,9 +838,9 @@
 
 	case CHIOINITELEM:
 	{
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_init_elem(ch);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 		
@@ -852,12 +857,12 @@
 			return -EBADSLT;
 		}
 		elem = ch->firsts[csv.csv_type] + csv.csv_unit;
-		down(&ch->lock);
+		mutex_lock(&ch->lock);
 		retval = ch_set_voltag(ch, elem,
 				       csv.csv_flags & CSV_AVOLTAG,
 				       csv.csv_flags & CSV_CLEARTAG,
 				       csv.csv_voltag);
-		up(&ch->lock);
+		mutex_unlock(&ch->lock);
 		return retval;
 	}
 
@@ -930,7 +935,7 @@
 	memset(ch,0,sizeof(*ch));
 	ch->minor = ch_devcount;
 	sprintf(ch->name,"ch%d",ch->minor);
-	init_MUTEX(&ch->lock);
+	mutex_init(&ch->lock);
 	ch->device = sd;
 	ch_readconfig(ch);
 	if (init)

Re: [RFC] migrate ch.c away from semaphores

[Gerd Hoffmann]

> Gerd I am not sure if you still maintain this driver but since you're
> listed as the author you get the first blame or chance to yell ;-)

Looks ok to me.

cheers,

   Gerd

-- 
Gerd 'just married' Hoffmann <kraxel@suse.de>
I'm the hacker formerly known as Gerd Knorr.