[PATCH 1/5] dasd: cleanup dasd_ioctl

[PATCH 1/5] dasd: cleanup dasd_ioctl

[Christoph Hellwig]

Handle ioctls implemented in dasd_ioctl through the normal switch
statement that most drivers use instead of the awkward
dasd_ioctl_no_register routine.   This avoids searching a linear list
on every call to dasd_ioctl(), and allows to give the various ioctl
implementation functions sane prototypes, aswell as moving the
check for bdev->bd_disk->private_data from the individual functions
to dasd_ioctl.  (I think it can't actually every be NULL, but let's keep
that for later)


 dasd.c       |    4 
 dasd_int.h   |    2 
 dasd_ioctl.c |  241 +++++++++++++++++++++--------------------------------------
 3 files changed, 86 insertions(+), 161 deletions(-)

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6.16-rc2-mm1/drivers/s390/block/dasd_int.h
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/s390/block/dasd_int.h	2006-02-11 17:32:51.000000000 +0100
+++ linux-2.6.16-rc2-mm1/drivers/s390/block/dasd_int.h	2006-02-11 17:33:47.000000000 +0100
@@ -558,8 +558,6 @@
 void dasd_destroy_partitions(struct dasd_device *);
 
 /* externals in dasd_ioctl.c */
-int  dasd_ioctl_init(void);
-void dasd_ioctl_exit(void);
 int  dasd_ioctl_no_register(struct module *, int, dasd_ioctl_fn_t);
 int  dasd_ioctl_no_unregister(struct module *, int, dasd_ioctl_fn_t);
 int  dasd_ioctl(struct inode *, struct file *, unsigned int, unsigned long);
Index: linux-2.6.16-rc2-mm1/drivers/s390/block/dasd_ioctl.c
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/s390/block/dasd_ioctl.c	2006-02-11 17:32:51.000000000 +0100
+++ linux-2.6.16-rc2-mm1/drivers/s390/block/dasd_ioctl.c	2006-02-11 17:34:03.000000000 +0100
@@ -77,62 +77,11 @@
 	return 0;
 }
 
-int
-dasd_ioctl(struct inode *inp, struct file *filp,
-	   unsigned int no, unsigned long data)
-{
-	struct block_device *bdev = inp->i_bdev;
-	struct dasd_device *device = bdev->bd_disk->private_data;
-	struct dasd_ioctl *ioctl;
-	const char *dir;
-	int rc;
-
-	if ((_IOC_DIR(no) != _IOC_NONE) && (data == 0)) {
-		PRINT_DEBUG("empty data ptr");
-		return -EINVAL;
-	}
-	dir = _IOC_DIR (no) == _IOC_NONE ? "0" :
-		_IOC_DIR (no) == _IOC_READ ? "r" :
-		_IOC_DIR (no) == _IOC_WRITE ? "w" : 
-		_IOC_DIR (no) == (_IOC_READ | _IOC_WRITE) ? "rw" : "u";
-	DBF_DEV_EVENT(DBF_DEBUG, device,
-		      "ioctl 0x%08x %s'0x%x'%d(%d) with data %8lx", no,
-		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
-	/* Search for ioctl no in the ioctl list. */
-	list_for_each_entry(ioctl, &dasd_ioctl_list, list) {
-		if (ioctl->no == no) {
-			/* Found a matching ioctl. Call it. */
-			if (!try_module_get(ioctl->owner))
-				continue;
-			rc = ioctl->handler(bdev, no, data);
-			module_put(ioctl->owner);
-			return rc;
-		}
-	}
-	/* No ioctl with number no. */
-	DBF_DEV_EVENT(DBF_INFO, device,
-		      "unknown ioctl 0x%08x=%s'0x%x'%d(%d) data %8lx", no,
-		      dir, _IOC_TYPE(no), _IOC_NR(no), _IOC_SIZE(no), data);
-	return -EINVAL;
-}
-
-long
-dasd_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
-	int rval;
-
-	lock_kernel();
-	rval = dasd_ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
-	unlock_kernel();
-
-	return (rval == -EINVAL) ? -ENOIOCTLCMD : rval;
-}
-
 static int
-dasd_ioctl_api_version(struct block_device *bdev, int no, long args)
+dasd_ioctl_api_version(void __user *argp)
 {
 	int ver = DASD_API_VERSION;
-	return put_user(ver, (int __user *) args);
+	return put_user(ver, (int *)argp);
 }
 
 /*
@@ -140,15 +89,13 @@
  * used by dasdfmt after BIODASDDISABLE to retrigger blocksize detection
  */
 static int
-dasd_ioctl_enable(struct block_device *bdev, int no, long args)
+dasd_ioctl_enable(struct block_device *bdev)
 {
-	struct dasd_device *device;
+	struct dasd_device *device = bdev->bd_disk->private_data;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+
 	dasd_enable_device(device);
 	/* Formatting the dasd device can change the capacity. */
 	mutex_lock(&bdev->bd_mutex);
@@ -162,15 +109,13 @@
  * Used by dasdfmt. Disable I/O operations but allow ioctls.
  */
 static int
-dasd_ioctl_disable(struct block_device *bdev, int no, long args)
+dasd_ioctl_disable(struct block_device *bdev)
 {
-	struct dasd_device *device;
+	struct dasd_device *device = bdev->bd_disk->private_data;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
+
 	/*
 	 * Man this is sick. We don't do a real disable but only downgrade
 	 * the device to DASD_STATE_BASIC. The reason is that dasdfmt uses
@@ -194,18 +139,13 @@
  * Quiesce device.
  */
 static int
-dasd_ioctl_quiesce(struct block_device *bdev, int no, long args)
+dasd_ioctl_quiesce(struct dasd_device *device)
 {
-	struct dasd_device *device;
 	unsigned long flags;
 	
 	if (!capable (CAP_SYS_ADMIN))
 		return -EACCES;
 	
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
-	
 	DEV_MESSAGE (KERN_DEBUG, device, "%s",
 		     "Quiesce IO on device");
 	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
@@ -219,18 +159,13 @@
  * Quiesce device.
  */
 static int
-dasd_ioctl_resume(struct block_device *bdev, int no, long args)
+dasd_ioctl_resume(struct dasd_device *device)
 {
-	struct dasd_device *device;
 	unsigned long flags;
 	
 	if (!capable (CAP_SYS_ADMIN)) 
 		return -EACCES;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
-
 	DEV_MESSAGE (KERN_DEBUG, device, "%s",
 		     "resume IO on device");
 	
@@ -302,25 +237,19 @@
  * Format device.
  */
 static int
-dasd_ioctl_format(struct block_device *bdev, int no, long args)
+dasd_ioctl_format(struct block_device *bdev, void __user *argp)
 {
-	struct dasd_device *device;
+	struct dasd_device *device = bdev->bd_disk->private_data;
 	struct format_data_t fdata;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
-	if (!args)
+	if (!argp)
 		return -EINVAL;
-	/* fdata == NULL is no longer a valid arg to dasd_format ! */
-	device = bdev->bd_disk->private_data;
-
-	if (device == NULL)
-		return -ENODEV;
 
 	if (device->features & DASD_FEATURE_READONLY)
 		return -EROFS;
-	if (copy_from_user(&fdata, (void __user *) args,
-			   sizeof (struct format_data_t)))
+	if (copy_from_user(&fdata, argp, sizeof(struct format_data_t)))
 		return -EFAULT;
 	if (bdev != bdev->bd_contains) {
 		DEV_MESSAGE(KERN_WARNING, device, "%s",
@@ -335,17 +264,8 @@
  * Reset device profile information
  */
 static int
-dasd_ioctl_reset_profile(struct block_device *bdev, int no, long args)
+dasd_ioctl_reset_profile(struct dasd_device *device)
 {
-	struct dasd_device *device;
-
-	if (!capable(CAP_SYS_ADMIN))
-		return -EACCES;
-
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
-
 	memset(&device->profile, 0, sizeof (struct dasd_profile_info_t));
 	return 0;
 }
@@ -354,31 +274,24 @@
  * Return device profile information
  */
 static int
-dasd_ioctl_read_profile(struct block_device *bdev, int no, long args)
+dasd_ioctl_read_profile(struct dasd_device *device, void __user *argp)
 {
-	struct dasd_device *device;
-
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
-
 	if (dasd_profile_level == DASD_PROFILE_OFF)
 		return -EIO;
-
-	if (copy_to_user((long __user *) args, (long *) &device->profile,
+	if (copy_to_user(argp, &device->profile,
 			 sizeof (struct dasd_profile_info_t)))
 		return -EFAULT;
 	return 0;
 }
 #else
 static int
-dasd_ioctl_reset_profile(struct block_device *bdev, int no, long args)
+dasd_ioctl_reset_profile(struct dasd_device *device)
 {
 	return -ENOSYS;
 }
 
 static int
-dasd_ioctl_read_profile(struct block_device *bdev, int no, long args)
+dasd_ioctl_read_profile(struct dasd_device *device, void __user *argp)
 {
 	return -ENOSYS;
 }
@@ -388,18 +301,14 @@
  * Return dasd information. Used for BIODASDINFO and BIODASDINFO2.
  */
 static int
-dasd_ioctl_information(struct block_device *bdev, int no, long args)
+dasd_ioctl_information(struct dasd_device *device,
+		unsigned int cmd, void __user *argp)
 {
-	struct dasd_device *device;
 	struct dasd_information2_t *dasd_info;
 	unsigned long flags;
 	int rc;
 	struct ccw_device *cdev;
 
-	device = bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
-
 	if (!device->discipline->fill_info)
 		return -EINVAL;
 
@@ -467,8 +376,8 @@
 	}
 
 	rc = 0;
-	if (copy_to_user((long __user *) args, (long *) dasd_info,
-			 ((no == (unsigned int) BIODASDINFO2) ?
+	if (copy_to_user(argp, dasd_info,
+			 ((cmd == (unsigned int) BIODASDINFO2) ?
 			  sizeof (struct dasd_information2_t) :
 			  sizeof (struct dasd_information_t))))
 		rc = -EFAULT;
@@ -480,68 +389,90 @@
  * Set read only
  */
 static int
-dasd_ioctl_set_ro(struct block_device *bdev, int no, long args)
+dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
 {
-	struct dasd_device *device;
-	int intval, rc;
+	struct dasd_device *device =  bdev->bd_disk->private_data;
+	int intval;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 	if (bdev != bdev->bd_contains)
 		// ro setting is not allowed for partitions
 		return -EINVAL;
-	if (get_user(intval, (int __user *) args))
+	if (get_user(intval, (int *)argp))
 		return -EFAULT;
-	device =  bdev->bd_disk->private_data;
-	if (device == NULL)
-		return -ENODEV;
 
 	set_disk_ro(bdev->bd_disk, intval);
-	rc = dasd_set_feature(device->cdev, DASD_FEATURE_READONLY, intval);
-
-	return rc;
+	return dasd_set_feature(device->cdev, DASD_FEATURE_READONLY, intval);
 }
 
-/*
- * List of static ioctls.
- */
-static struct { int no; dasd_ioctl_fn_t fn; } dasd_ioctls[] =
-{
-	{ BIODASDDISABLE, dasd_ioctl_disable },
-	{ BIODASDENABLE, dasd_ioctl_enable },
-	{ BIODASDQUIESCE, dasd_ioctl_quiesce },
-	{ BIODASDRESUME, dasd_ioctl_resume },
-	{ BIODASDFMT, dasd_ioctl_format },
-	{ BIODASDINFO, dasd_ioctl_information },
-	{ BIODASDINFO2, dasd_ioctl_information },
-	{ BIODASDPRRD, dasd_ioctl_read_profile },
-	{ BIODASDPRRST, dasd_ioctl_reset_profile },
-	{ BLKROSET, dasd_ioctl_set_ro },
-	{ DASDAPIVER, dasd_ioctl_api_version },
-	{ -1, NULL }
-};
-
 int
-dasd_ioctl_init(void)
+dasd_ioctl(struct inode *inode, struct file *file,
+	   unsigned int cmd, unsigned long arg)
 {
-	int i;
+	struct block_device *bdev = inode->i_bdev;
+	struct dasd_device *device = bdev->bd_disk->private_data;
+	void __user *argp = (void __user *)arg;
+	struct dasd_ioctl *ioctl;
+	int rc;
 
-	for (i = 0; dasd_ioctls[i].no != -1; i++)
-		dasd_ioctl_no_register(NULL, dasd_ioctls[i].no,
-				       dasd_ioctls[i].fn);
-	return 0;
+	if (!device)
+                return -ENODEV;
+
+	if ((_IOC_DIR(cmd) != _IOC_NONE) && !arg) {
+		PRINT_DEBUG("empty data ptr");
+		return -EINVAL;
+	}
 
+	switch (cmd) {
+	case BIODASDDISABLE:
+		return dasd_ioctl_disable(bdev);
+	case BIODASDENABLE:
+		return dasd_ioctl_enable(bdev);
+	case BIODASDQUIESCE:
+		return dasd_ioctl_quiesce(device);
+	case BIODASDRESUME:
+		return dasd_ioctl_resume(device);
+	case BIODASDFMT:
+		return dasd_ioctl_format(bdev, argp);
+	case BIODASDINFO:
+		return dasd_ioctl_information(device, cmd, argp);
+	case BIODASDINFO2:
+		return dasd_ioctl_information(device, cmd, argp);
+	case BIODASDPRRD:
+		return dasd_ioctl_read_profile(device, argp);
+	case BIODASDPRRST:
+		return dasd_ioctl_reset_profile(device);
+	case BLKROSET:
+		return dasd_ioctl_set_ro(bdev, argp);
+	case DASDAPIVER:
+		return dasd_ioctl_api_version(argp);
+	default:
+		/* resort to the deprecated dynamic ioctl list */
+		list_for_each_entry(ioctl, &dasd_ioctl_list, list) {
+			if (ioctl->no == cmd) {
+				/* Found a matching ioctl. Call it. */
+				if (!try_module_get(ioctl->owner))
+					continue;
+				rc = ioctl->handler(bdev, cmd, arg);
+				module_put(ioctl->owner);
+				return rc;
+			}
+		}
+		return -EINVAL;
+	}
 }
 
-void
-dasd_ioctl_exit(void)
+long
+dasd_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	int i;
+	int rval;
 
-	for (i = 0; dasd_ioctls[i].no != -1; i++)
-		dasd_ioctl_no_unregister(NULL, dasd_ioctls[i].no,
-					 dasd_ioctls[i].fn);
+	lock_kernel();
+	rval = dasd_ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
+	unlock_kernel();
 
+	return (rval == -EINVAL) ? -ENOIOCTLCMD : rval;
 }
 
 EXPORT_SYMBOL(dasd_ioctl_no_register);
Index: linux-2.6.16-rc2-mm1/drivers/s390/block/dasd.c
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/s390/block/dasd.c	2006-02-11 17:32:51.000000000 +0100
+++ linux-2.6.16-rc2-mm1/drivers/s390/block/dasd.c	2006-02-11 17:33:47.000000000 +0100
@@ -1803,7 +1803,6 @@
 #ifdef CONFIG_PROC_FS
 	dasd_proc_exit();
 #endif
-	dasd_ioctl_exit();
         if (dasd_page_cache != NULL) {
 		kmem_cache_destroy(dasd_page_cache);
 		dasd_page_cache = NULL;
@@ -2123,9 +2122,6 @@
 	rc = dasd_parse();
 	if (rc)
 		goto failed;
-	rc = dasd_ioctl_init();
-	if (rc)
-		goto failed;
 #ifdef CONFIG_PROC_FS
 	rc = dasd_proc_init();
 	if (rc)

Re: [PATCH 1/5] dasd: cleanup dasd_ioctl

[Bastian Blank]

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

On Sun, Feb 12, 2006 at 06:38:55PM +0100, Christoph Hellwig wrote:
>  static int
> -dasd_ioctl_api_version(struct block_device *bdev, int no, long args)
> +dasd_ioctl_api_version(void __user *argp)
>  {
>  	int ver = DASD_API_VERSION;
> -	return put_user(ver, (int __user *) args);
> +	return put_user(ver, (int *)argp);
>  }

Doesn't this need to be "int __user *"?

> +long
> +dasd_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> -	int i;
> +	int rval;
>  
> -	for (i = 0; dasd_ioctls[i].no != -1; i++)
> -		dasd_ioctl_no_unregister(NULL, dasd_ioctls[i].no,
> -					 dasd_ioctls[i].fn);
> +	lock_kernel();
> +	rval = dasd_ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
> +	unlock_kernel();

The lock_kernel looks spurious.

Bastian

-- 
Conquest is easy. Control is not.
		-- Kirk, "Mirror, Mirror", stardate unknown

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] dasd: cleanup dasd_ioctl

[Christoph Hellwig]

On Sun, Feb 12, 2006 at 07:03:08PM +0100, Bastian Blank wrote:
> On Sun, Feb 12, 2006 at 06:38:55PM +0100, Christoph Hellwig wrote:
> >  static int
> > -dasd_ioctl_api_version(struct block_device *bdev, int no, long args)
> > +dasd_ioctl_api_version(void __user *argp)
> >  {
> >  	int ver = DASD_API_VERSION;
> > -	return put_user(ver, (int __user *) args);
> > +	return put_user(ver, (int *)argp);
> >  }
> 
> Doesn't this need to be "int __user *"?

Yes.

> > +long
> > +dasd_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  {
> > -	int i;
> > +	int rval;
> >  
> > -	for (i = 0; dasd_ioctls[i].no != -1; i++)
> > -		dasd_ioctl_no_unregister(NULL, dasd_ioctls[i].no,
> > -					 dasd_ioctls[i].fn);
> > +	lock_kernel();
> > +	rval = dasd_ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
> > +	unlock_kernel();
> 
> The lock_kernel looks spurious.

dasd_compat_ioctl just moved down unchanged to the end of the file so it
can call dasd_ioctl without a forward-prototype.  When I introduced this
function a while ago I added the lock_kernel because that the BKL is
held when dasd_ioctl is called directly and I wanted to avoid different
locks from different codepathes.  Once we can switch dasd to
->unlocked_ioctl it could probably go away.

Re: [PATCH 1/5] dasd: cleanup dasd_ioctl

[Bastian Blank]

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

On Sun, Feb 12, 2006 at 06:38:55PM +0100, Christoph Hellwig wrote:
> @@ -480,68 +389,90 @@
>   * Set read only
>   */
>  static int
> -dasd_ioctl_set_ro(struct block_device *bdev, int no, long args)
> +dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
>  {
> -	struct dasd_device *device;
> -	int intval, rc;
> +	struct dasd_device *device =  bdev->bd_disk->private_data;
> +	int intval;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
>  	if (bdev != bdev->bd_contains)
>  		// ro setting is not allowed for partitions
>  		return -EINVAL;
> -	if (get_user(intval, (int __user *) args))
> +	if (get_user(intval, (int *)argp))
>  		return -EFAULT;

I think this is another candidate for "int __user *".

Bastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]