Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

[Andre Noll]

On 17:40, Andi Kleen wrote:
 
> Here's a proposal for some useful code transformations the kernel janitors
> could do as opposed to running checkpatch.pl.

Here's my take on drivers/scsi/sg.c. It's only compile-tested on x86-64.

Please review.
Andre
---

Convert sg.c to the new unlocked_ioctl entry point.

Signed-off-by: Andre Noll <maan@xxxxxxxxxxxxxxx>

---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f1871ea..3063307 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -48,6 +48,7 @@ static int sg_version_num = 30534;	/* 2 digits for each component */
 #include <linux/blkdev.h>
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
+#include <linux/smp_lock.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -764,20 +765,22 @@ sg_srp_done(Sg_request *srp, Sg_fd *sfp)
 	return done;
 }
 
-static int
-sg_ioctl(struct inode *inode, struct file *filp,
-	 unsigned int cmd_in, unsigned long arg)
+static long
+sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
 {
 	void __user *p = (void __user *)arg;
 	int __user *ip = p;
-	int result, val, read_only;
+	int val, read_only;
 	Sg_device *sdp;
 	Sg_fd *sfp;
 	Sg_request *srp;
 	unsigned long iflags;
+	long result;
 
+	lock_kernel();
+	result = -ENXIO;
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
-		return -ENXIO;
+		goto out;
 	SCSI_LOG_TIMEOUT(3, printk("sg_ioctl: %s, cmd=0x%x\n",
 				   sdp->disk->disk_name, (int) cmd_in));
 	read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
@@ -787,57 +790,66 @@ sg_ioctl(struct inode *inode, struct file *filp,
 		{
 			int blocking = 1;	/* ignore O_NONBLOCK flag */
 
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
+			result = -ENXIO;
 			if (!scsi_block_when_processing_errors(sdp->device))
-				return -ENXIO;
+				goto out;
+			result = -EFAULT;
 			if (!access_ok(VERIFY_WRITE, p, SZ_SG_IO_HDR))
-				return -EFAULT;
-			result =
-			    sg_new_write(sfp, p, SZ_SG_IO_HDR,
-					 blocking, read_only, &srp);
+				goto out;
+			result = sg_new_write(sfp, p, SZ_SG_IO_HDR, blocking,
+				read_only, &srp);
 			if (result < 0)
-				return result;
+				goto out;
 			srp->sg_io_owned = 1;
 			while (1) {
 				result = 0;	/* following macro to beat race condition */
 				__wait_event_interruptible(sfp->read_wait,
 					(sdp->detached || sfp->closed || sg_srp_done(srp, sfp)),
 							   result);
-				if (sdp->detached)
-					return -ENODEV;
+				if (sdp->detached) {
+					result = -ENODEV;
+					goto out;
+				}
 				if (sfp->closed)
-					return 0;	/* request packet dropped already */
+					goto out;	/* request packet dropped already */
 				if (0 == result)
 					break;
 				srp->orphan = 1;
-				return result;	/* -ERESTARTSYS because signal hit process */
+				goto out;	/* -ERESTARTSYS because signal hit process */
 			}
 			write_lock_irqsave(&sfp->rq_list_lock, iflags);
 			srp->done = 2;
 			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 			result = sg_new_read(sfp, p, SZ_SG_IO_HDR, srp);
-			return (result < 0) ? result : 0;
+			if (result > 0)
+				result = 0;
+			goto out;
 		}
 	case SG_SET_TIMEOUT:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
+		result = -EIO;
 		if (val < 0)
-			return -EIO;
-		if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
-		    val = MULDIV (INT_MAX, USER_HZ, HZ);
+			goto out;
+		if (val >= MULDIV(INT_MAX, USER_HZ, HZ))
+			val = MULDIV(INT_MAX, USER_HZ, HZ);
 		sfp->timeout_user = val;
 		sfp->timeout = MULDIV (val, HZ, USER_HZ);
 
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */
 				/* strange ..., for backward compatibility */
-		return sfp->timeout_user;
+		result = sfp->timeout_user;
+		goto out;
 	case SG_SET_FORCE_LOW_DMA:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		if (val) {
 			sfp->low_dma = 1;
 			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
@@ -846,21 +858,26 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				sg_build_reserve(sfp, val);
 			}
 		} else {
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
 			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
 		}
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_LOW_DMA:
-		return put_user((int) sfp->low_dma, ip);
+		result = put_user((int) sfp->low_dma, ip);
+		goto out;
 	case SG_GET_SCSI_ID:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
-			return -EFAULT;
-		else {
+			goto out;
+		{
 			sg_scsi_id_t __user *sg_idp = p;
 
+			result = -ENODEV;
 			if (sdp->detached)
-				return -ENODEV;
+				goto out;
 			__put_user((int) sdp->device->host->host_no,
 				   &sg_idp->host_no);
 			__put_user((int) sdp->device->channel,
@@ -874,29 +891,33 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				   &sg_idp->d_queue_depth);
 			__put_user(0, &sg_idp->unused[0]);
 			__put_user(0, &sg_idp->unused[1]);
-			return 0;
+			result = 0;
+			goto out;
 		}
 	case SG_SET_FORCE_PACK_ID:
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		sfp->force_packid = val ? 1 : 0;
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_PACK_ID:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, ip, sizeof (int)))
-			return -EFAULT;
+			goto out;
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
+		result = 0;
 		for (srp = sfp->headrp; srp; srp = srp->nextrp) {
 			if ((1 == srp->done) && (!srp->sg_io_owned)) {
 				read_unlock_irqrestore(&sfp->rq_list_lock,
 						       iflags);
 				__put_user(srp->header.pack_id, ip);
-				return 0;
+				goto out;
 			}
 		}
 		read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 		__put_user(-1, ip);
-		return 0;
+		goto out;
 	case SG_GET_NUM_WAITING:
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
 		for (val = 0, srp = sfp->headrp; srp; srp = srp->nextrp) {
@@ -904,67 +925,76 @@ sg_ioctl(struct inode *inode, struct file *filp,
 				++val;
 		}
 		read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_GET_SG_TABLESIZE:
-		return put_user(sdp->sg_tablesize, ip);
+		result = put_user(sdp->sg_tablesize, ip);
+		goto out;
 	case SG_SET_RESERVED_SIZE:
 		result = get_user(val, ip);
 		if (result)
-			return result;
-                if (val < 0)
-                        return -EINVAL;
+			goto out;
+		result = -EINVAL;
+		if (val < 0)
+			goto out;
 		val = min_t(int, val,
 				sdp->device->request_queue->max_sectors * 512);
 		if (val != sfp->reserve.bufflen) {
+			result = -EBUSY;
 			if (sg_res_in_use(sfp) || sfp->mmap_called)
-				return -EBUSY;
+				goto out;
 			sg_remove_scat(&sfp->reserve);
 			sg_build_reserve(sfp, val);
 		}
-		return 0;
+		result = 0;
+		goto out;
 	case SG_GET_RESERVED_SIZE:
 		val = min_t(int, sfp->reserve.bufflen,
 				sdp->device->request_queue->max_sectors * 512);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_SET_COMMAND_Q:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->cmd_q = val ? 1 : 0;
-		return 0;
+		if (!result)
+			sfp->cmd_q = val ? 1 : 0;
+		goto out;
 	case SG_GET_COMMAND_Q:
-		return put_user((int) sfp->cmd_q, ip);
+		result = put_user((int) sfp->cmd_q, ip);
+		goto out;
 	case SG_SET_KEEP_ORPHAN:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->keep_orphan = val;
-		return 0;
+		if (!result)
+			sfp->keep_orphan = val;
+		goto out;
 	case SG_GET_KEEP_ORPHAN:
-		return put_user((int) sfp->keep_orphan, ip);
+		result = put_user((int) sfp->keep_orphan, ip);
+		goto out;
 	case SG_NEXT_CMD_LEN:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sfp->next_cmd_len = (val > 0) ? val : 0;
-		return 0;
+		if (!result)
+			sfp->next_cmd_len = (val > 0) ? val : 0;
+		goto out;
 	case SG_GET_VERSION_NUM:
-		return put_user(sg_version_num, ip);
+		result = put_user(sg_version_num, ip);
+		goto out;
 	case SG_GET_ACCESS_COUNT:
 		/* faked - we don't have a real access count anymore */
 		val = (sdp->device ? 1 : 0);
-		return put_user(val, ip);
+		result = put_user(val, ip);
+		goto out;
 	case SG_GET_REQUEST_TABLE:
+		result = -EFAULT;
 		if (!access_ok(VERIFY_WRITE, p, SZ_SG_REQ_INFO * SG_MAX_QUEUE))
-			return -EFAULT;
-		else {
+			goto out;
+		{
 			sg_req_info_t *rinfo;
 			unsigned int ms;
 
 			rinfo = kmalloc(SZ_SG_REQ_INFO * SG_MAX_QUEUE,
 								GFP_KERNEL);
+			result = -ENOMEM;
 			if (!rinfo)
-				return -ENOMEM;
+				goto out;
 			read_lock_irqsave(&sfp->rq_list_lock, iflags);
 			for (srp = sfp->headrp, val = 0; val < SG_MAX_QUEUE;
 			     ++val, srp = srp ? srp->nextrp : srp) {
@@ -998,25 +1028,29 @@ sg_ioctl(struct inode *inode, struct file *filp,
 						SZ_SG_REQ_INFO * SG_MAX_QUEUE);
 			result = result ? -EFAULT : 0;
 			kfree(rinfo);
-			return result;
 		}
+		goto out;
 	case SG_EMULATED_HOST:
 		if (sdp->detached)
-			return -ENODEV;
-		return put_user(sdp->device->host->hostt->emulated, ip);
+			result = -ENODEV;
+		else
+			result = put_user(sdp->device->host->hostt->emulated, ip);
+		goto out;
 	case SG_SCSI_RESET:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
+			goto out;
+		result = -EBUSY;
 		if (filp->f_flags & O_NONBLOCK) {
 			if (scsi_host_in_recovery(sdp->device->host))
-				return -EBUSY;
+				goto out;
 		} else if (!scsi_block_when_processing_errors(sdp->device))
-			return -EBUSY;
+			goto out;
 		result = get_user(val, ip);
 		if (result)
-			return result;
+			goto out;
 		if (SG_SCSI_RESET_NOTHING == val)
-			return 0;
+			goto out;
 		switch (val) {
 		case SG_SCSI_RESET_DEVICE:
 			val = SCSI_TRY_RESET_DEVICE;
@@ -1028,46 +1062,60 @@ sg_ioctl(struct inode *inode, struct file *filp,
 			val = SCSI_TRY_RESET_HOST;
 			break;
 		default:
-			return -EINVAL;
+			result = -EINVAL;
+			goto out;
 		}
+		result = -EACCES;
 		if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
-			return -EACCES;
-		return (scsi_reset_provider(sdp->device, val) ==
-			SUCCESS) ? 0 : -EIO;
+			goto out;
+		result = scsi_reset_provider(sdp->device, val) == SUCCESS?
+			0 : -EIO;
+		goto out;
 	case SCSI_IOCTL_SEND_COMMAND:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
+			goto out;
 		if (read_only) {
 			unsigned char opcode = WRITE_6;
 			Scsi_Ioctl_Command __user *siocp = p;
 
+			result = -EFAULT;
 			if (copy_from_user(&opcode, siocp->data, 1))
-				return -EFAULT;
+				goto out;
+			result = -EPERM;
 			if (!sg_allow_access(opcode, sdp->device->type))
-				return -EPERM;
+				goto out;
 		}
-		return sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+		result = sg_scsi_ioctl(filp, sdp->device->request_queue, NULL, p);
+		goto out;
 	case SG_SET_DEBUG:
 		result = get_user(val, ip);
-		if (result)
-			return result;
-		sdp->sgdebug = (char) val;
-		return 0;
+		if (!result)
+			sdp->sgdebug = (char) val;
+		goto out;
 	case SCSI_IOCTL_GET_IDLUN:
 	case SCSI_IOCTL_GET_BUS_NUMBER:
 	case SCSI_IOCTL_PROBE_HOST:
 	case SG_GET_TRANSFORM:
+		result = -ENODEV;
 		if (sdp->detached)
-			return -ENODEV;
-		return scsi_ioctl(sdp->device, cmd_in, p);
+			goto out;
+		result = scsi_ioctl(sdp->device, cmd_in, p);
+		goto out;
 	case BLKSECTGET:
-		return put_user(sdp->device->request_queue->max_sectors * 512,
+		result = put_user(sdp->device->request_queue->max_sectors * 512,
 				ip);
+		goto out;
 	default:
+		result = -EPERM;	/* don't know so take safe approach */
 		if (read_only)
-			return -EPERM;	/* don't know so take safe approach */
-		return scsi_ioctl(sdp->device, cmd_in, p);
+			goto out;
+		result = scsi_ioctl(sdp->device, cmd_in, p);
+		goto out;
 	}
+out:
+	unlock_kernel();
+	return result;
 }
 
 #ifdef CONFIG_COMPAT
@@ -1314,7 +1362,7 @@ static struct file_operations sg_fops = {
 	.read = sg_read,
 	.write = sg_write,
 	.poll = sg_poll,
-	.ioctl = sg_ioctl,
+	.unlocked_ioctl = sg_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = sg_compat_ioctl,
 #endif
-- 
The only person who always got his work done by Friday was Robinson Crusoe

Attachment: signature.asc
Description: Digital signature