[PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[Christoph Hellwig]

all ioctls are 32bit compat clean, so the driver can use ->compat_ioctl
and ->unlocked_ioctl easily.


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

Index: linux-2.6/arch/sparc64/kernel/ioctl32.c
=================================--- linux-2.6.orig/arch/sparc64/kernel/ioctl32.c	2005-11-07 11:55:40.000000000 +0100
+++ linux-2.6/arch/sparc64/kernel/ioctl32.c	2005-11-07 11:56:00.000000000 +0100
@@ -121,9 +121,6 @@
 /* Little v, the video4linux ioctls */
 COMPATIBLE_IOCTL(_IOR('p', 20, int[7])) /* RTCGET */
 COMPATIBLE_IOCTL(_IOW('p', 21, int[7])) /* RTCSET */
-/* COMPATIBLE_IOCTL(D7SIOCRD) same value as ENVCTRL_RD_VOLTAGE_STATUS */
-COMPATIBLE_IOCTL(D7SIOCWR)
-COMPATIBLE_IOCTL(D7SIOCTM)
 COMPATIBLE_IOCTL(WIOCSTART)
 COMPATIBLE_IOCTL(WIOCSTOP)
 COMPATIBLE_IOCTL(WIOCGSTAT)
Index: linux-2.6/drivers/sbus/char/display7seg.c
=================================--- linux-2.6.orig/drivers/sbus/char/display7seg.c	2005-11-07 11:53:44.000000000 +0100
+++ linux-2.6/drivers/sbus/char/display7seg.c	2005-11-07 11:55:48.000000000 +0100
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/miscdevice.h>
 #include <linux/ioport.h>		/* request_region */
+#include <linux/smp_lock.h>
 #include <asm/atomic.h>
 #include <asm/ebus.h>			/* EBus device					*/
 #include <asm/oplib.h>			/* OpenProm Library 			*/
@@ -114,22 +115,25 @@
 	return 0;
 }
 
-static int d7s_ioctl(struct inode *inode, struct file *f, 
-		     unsigned int cmd, unsigned long arg)
+static long d7s_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	__u8 regs = readb(d7s_regs);
 	__u8 ireg = 0;
+	int error = 0
 
-	if (D7S_MINOR != iminor(inode))
+	if (D7S_MINOR != iminor(file->f_dentry->d_inode))
 		return -ENODEV;
 
+	lock_kernel();
 	switch (cmd) {
 	case D7SIOCWR:
 		/* assign device register values
 		 * we mask-out D7S_FLIP if in sol_compat mode
 		 */
-		if (get_user(ireg, (int __user *) arg))
-			return -EFAULT;
+		if (get_user(ireg, (int __user *) arg)) {
+			error = -EFAULT;
+			break;
+		}
 		if (0 != sol_compat) {
 			(regs & D7S_FLIP) ? 
 				(ireg |= D7S_FLIP) : (ireg &= ~D7S_FLIP);
@@ -144,8 +148,10 @@
 		 * This driver will not misinform you about the state
 		 * of your hardware while in sol_compat mode
 		 */
-		if (put_user(regs, (int __user *) arg))
-			return -EFAULT;
+		if (put_user(regs, (int __user *) arg)) {
+			error = -EFAULT;
+			break;
+		}
 		break;
 
 	case D7SIOCTM:
@@ -155,15 +161,17 @@
 		writeb(regs, d7s_regs);
 		break;
 	};
+	lock_kernel();
 
-	return 0;
+	return error;
 }
 
 static struct file_operations d7s_fops = {
-	.owner =	THIS_MODULE,
-	.ioctl =	d7s_ioctl,
-	.open =		d7s_open,
-	.release =	d7s_release,
+	.owner =		THIS_MODULE,
+	.unlocked_ioctl =	d7s_ioctl,
+	.compat_ioctl =		d7s_ioctl,
+	.open =			d7s_open,
+	.release =		d7s_release,
 };
 
 static struct miscdevice d7s_miscdev = { D7S_MINOR, D7S_DEVNAME, &d7s_fops };

Re: [PATCH] display7seg: implement ->unlocked_ioctl and

[David S. Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Mon, 7 Nov 2005 20:42:32 +0100

> all ioctls are 32bit compat clean, so the driver can use ->compat_ioctl
> and ->unlocked_ioctl easily.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied, thanks.

Re: [PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[Eric Brower]

Are we intentionally locking the kernel twice in the ioctl handler?

I'd suggest we do away with the ioctl lock_kernel() altogether here--
unless the interrupt handler becomes reentrant we are not going to
experience concurrency issues.

E

On 11/7/05, David S. Miller <davem@davemloft.net> wrote:
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 7 Nov 2005 20:42:32 +0100
>
> > all ioctls are 32bit compat clean, so the driver can use ->compat_ioctl
> > and ->unlocked_ioctl easily.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Applied, thanks.
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
E

Re: [PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[Christoph Hellwig]

On Tue, Nov 08, 2005 at 12:48:57PM -0800, Eric Brower wrote:
> Are we intentionally locking the kernel twice in the ioctl handler?

No.  Otoh the BKL is recursive so it doesn't matter.

> I'd suggest we do away with the ioctl lock_kernel() altogether here--
> unless the interrupt handler becomes reentrant we are not going to
> experience concurrency issues.

Yes, that sounds like a good plan.  I can't test the driver so I
wouldn't want to do it myself.

Re: [PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[Eric Brower]

On 11/8/05, Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Nov 08, 2005 at 12:48:57PM -0800, Eric Brower wrote:
> > Are we intentionally locking the kernel twice in the ioctl handler?
>
> No.  Otoh the BKL is recursive so it doesn't matter.

I'm speaking specifically of the patch doing the following within the
driver's ioctl handler:

  lock_kernel()
    ... handle ioctl
  lock_kernel()

We ought to be calling unlock_kernel() at the end, correct?
--
E

Re: [PATCH] display7seg: implement ->unlocked_ioctl and ->compat_ioctl

[Christoph Hellwig]

On Tue, Nov 08, 2005 at 01:19:21PM -0800, Eric Brower wrote:
> On 11/8/05, Christoph Hellwig <hch@lst.de> wrote:
> > On Tue, Nov 08, 2005 at 12:48:57PM -0800, Eric Brower wrote:
> > > Are we intentionally locking the kernel twice in the ioctl handler?
> >
> > No.  Otoh the BKL is recursive so it doesn't matter.
> 
> I'm speaking specifically of the patch doing the following within the
> driver's ioctl handler:
> 
>   lock_kernel()
>     ... handle ioctl
>   lock_kernel()
> 
> We ought to be calling unlock_kernel() at the end, correct?

Umm, yes.  Sorry, this is a typo and the second one should be a
unlock_kernel().

Re: [PATCH] display7seg: implement ->unlocked_ioctl and

[David S. Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Tue, 8 Nov 2005 22:21:09 +0100

> On Tue, Nov 08, 2005 at 01:19:21PM -0800, Eric Brower wrote:
> > On 11/8/05, Christoph Hellwig <hch@lst.de> wrote:
> > > On Tue, Nov 08, 2005 at 12:48:57PM -0800, Eric Brower wrote:
> > > > Are we intentionally locking the kernel twice in the ioctl handler?
> > >
> > > No.  Otoh the BKL is recursive so it doesn't matter.
> > 
> > I'm speaking specifically of the patch doing the following within the
> > driver's ioctl handler:
> > 
> >   lock_kernel()
> >     ... handle ioctl
> >   lock_kernel()
> > 
> > We ought to be calling unlock_kernel() at the end, correct?
> 
> Umm, yes.  Sorry, this is a typo and the second one should be a
> unlock_kernel().

I've fixed both the display7seg and cpwatchdog driver instances
of this bug in my tree, thanks.