[PATCH] usb/serial: Add compat_ioctl pass-through

[PATCH] usb/serial: Add compat_ioctl pass-through

[Keith Packard]

USB serial devices with extended IOCTLs cannot be used in a 64-bit kernel
from 32-bit user space as the compat_ioctl path is missing. This adds a
pass-through so that drivers may offer this functionality. This requires
that all drivers actually implement a compat_ioctl function if they want to
support this operation.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/usb/serial/usb-serial.c |   22 ++++++++++++++++++++++
 include/linux/usb/serial.h      |    2 ++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 794b5ff..8fb4e13 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -381,6 +381,27 @@ static int serial_ioctl(struct tty_struct *tty, struct file *file,
 	return retval;
 }
 
+static long serial_compat_ioctl(struct tty_struct *tty, struct file *file,
+			       unsigned int cmd, unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	long retval = -ENODEV;
+
+	dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);
+
+	WARN_ON(!port->port.count);
+
+	/* pass on to the driver specific version of this function
+	   if it is available */
+	if (port->serial->type->compat_ioctl) {
+		lock_kernel();
+		retval = port->serial->type->compat_ioctl(tty, file, cmd, arg);
+		unlock_kernel();
+	} else
+		retval = -ENOIOCTLCMD;
+	return retval;
+}
+
 static void serial_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -1095,6 +1116,7 @@ static const struct tty_operations serial_ops = {
 	.write =		serial_write,
 	.write_room =		serial_write_room,
 	.ioctl =		serial_ioctl,
+	.compat_ioctl =		serial_compat_ioctl,
 	.set_termios =		serial_set_termios,
 	.throttle =		serial_throttle,
 	.unthrottle =		serial_unthrottle,
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 0b8617a..c15766a 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -228,6 +228,8 @@ struct usb_serial_driver {
 	int  (*write_room)(struct tty_struct *tty);
 	int  (*ioctl)(struct tty_struct *tty, struct file *file,
 		      unsigned int cmd, unsigned long arg);
+	long  (*compat_ioctl)(struct tty_struct *tty, struct file *file,
+			      unsigned int cmd, unsigned long arg);
 	void (*set_termios)(struct tty_struct *tty,
 			struct usb_serial_port *port, struct ktermios *old);
 	void (*break_ctl)(struct tty_struct *tty, int break_state);
-- 
1.5.6.5

[PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Keith Packard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6503 bytes --]

The cp2103 is programmed the same as the cp2101/cp2102 except for the
addition of a set of four GPIO pins which can be directly controlled by the
host. Access to this is done through a custom ioctl.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/usb/serial/cp2101.c |   97 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/serial/cp2101.h |   39 +++++++++++++++++
 2 files changed, 135 insertions(+), 1 deletions(-)
 create mode 100644 drivers/usb/serial/cp2101.h

diff --git a/drivers/usb/serial/cp2101.c b/drivers/usb/serial/cp2101.c
index 8008d0b..d0b96aa 100644
--- a/drivers/usb/serial/cp2101.c
+++ b/drivers/usb/serial/cp2101.c
@@ -28,11 +28,13 @@
 #include <linux/uaccess.h>
 #include <linux/usb/serial.h>
 
+#include "cp2101.h"
+
 /*
  * Version Information
  */
 #define DRIVER_VERSION "v0.07"
-#define DRIVER_DESC "Silicon Labs CP2101/CP2102 RS232 serial adaptor driver"
+#define DRIVER_DESC "Silicon Labs CP2101/CP2102/CP2103 RS232 serial adaptor driver"
 
 /*
  * Function Prototypes
@@ -42,6 +44,10 @@ static int cp2101_open(struct tty_struct *, struct usb_serial_port *,
 static void cp2101_cleanup(struct usb_serial_port *);
 static void cp2101_close(struct tty_struct *, struct usb_serial_port *,
 							struct file*);
+static int cp2101_ioctl(struct tty_struct *, struct file *,
+			unsigned int cmd, unsigned long arg);
+static long cp2101_compat_ioctl32(struct tty_struct *, struct file *,
+				 unsigned int cmd, unsigned long arg);
 static void cp2101_get_termios(struct tty_struct *);
 static void cp2101_set_termios(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios*);
@@ -52,6 +58,8 @@ static void cp2101_break_ctl(struct tty_struct *, int);
 static int cp2101_startup(struct usb_serial *);
 static void cp2101_shutdown(struct usb_serial *);
 
+static int cp210x_gpioget(struct usb_serial_port *port, u8* gpio);
+static int cp210x_gpioset(struct usb_serial_port *port, uint16_t arg);
 
 static int debug;
 
@@ -120,6 +128,8 @@ static struct usb_serial_driver cp2101_device = {
 	.open			= cp2101_open,
 	.close			= cp2101_close,
 	.break_ctl		= cp2101_break_ctl,
+	.ioctl			= cp2101_ioctl,
+	.compat_ioctl		= cp2101_compat_ioctl32,
 	.set_termios		= cp2101_set_termios,
 	.tiocmget 		= cp2101_tiocmget,
 	.tiocmset		= cp2101_tiocmset,
@@ -364,6 +374,45 @@ static void cp2101_close(struct tty_struct *tty, struct usb_serial_port *port,
 	mutex_unlock(&port->serial->disc_mutex);
 }
 
+static int cp2101_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	int ret;
+	u8 gpio_get;
+	u16 gpio_set;
+
+	switch(cmd) {
+	case CP2101_IOCTL_GPIOGET:
+		ret = cp210x_gpioget(port, &gpio_get);
+		if (ret < 0)
+			return ret;
+		if (copy_to_user((void __user*)arg,
+				 &gpio_get,
+				 sizeof(__u8)))
+			return -EFAULT;
+		return 0;
+
+	case CP2101_IOCTL_GPIOSET:
+		if (copy_from_user(&gpio_set,
+				   (void __user *)arg,
+				   sizeof(__u16)))
+			return -EFAULT;
+		ret = cp210x_gpioset(port, gpio_set);
+		if (ret < 0)
+			return ret;
+		return 0;
+
+	}
+	return -ENOIOCTLCMD;
+}
+
+static long cp2101_compat_ioctl32(struct tty_struct *tty, struct file *file,
+				  unsigned int cmd, unsigned long arg)
+{
+	return (long) cp2101_ioctl(tty, file, cmd, arg);
+}
+
 /*
  * cp2101_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
@@ -719,6 +768,52 @@ static void cp2101_break_ctl (struct tty_struct *tty, int break_state)
 	cp2101_set_config(port, CP2101_BREAK, &state, 2);
 }
 
+/*
+ * cp2101_ctlmsg
+ * A generic usb control message interface.
+ * Returns the actual size of the data read or written within the message, 0
+ * if no data were read or written, or a negative value to indicate an error.
+ */
+static int cp2101_ctlmsg(struct usb_serial_port* port, u8 request,
+		u8 requestype, u16 value, u16 index, void* data, u16 size)
+{
+	struct usb_device *dev = port->serial->dev;
+	u8 *tbuf;
+	int ret;
+
+	if (!(tbuf = kmalloc(size, GFP_KERNEL))) {
+		return -ENOMEM;
+	}
+
+	if (requestype & 0x80) {
+		ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
+				requestype, value, index, tbuf, size, 300);
+
+		if (ret > 0 && size)
+			memcpy(data, tbuf, size);
+	} else {
+		if (size)
+			memcpy(tbuf, data, size);
+
+		ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
+				requestype, value, index, tbuf, size, 300);
+	}
+	kfree(tbuf);
+	return ret;
+}
+
+/* Get current GPIO status */
+static int cp210x_gpioget(struct usb_serial_port *port, u8* gpio)
+{
+	return cp2101_ctlmsg(port, 0xff, 0xc0, 0x00c2, 0, gpio, 1);
+}
+
+/* Set all gpio simultaneously */
+static int cp210x_gpioset(struct usb_serial_port *port, uint16_t arg)
+{
+	return cp2101_ctlmsg(port, 0xff, 0x40, 0x37e1, arg, 0, 0);
+}
+
 static int cp2101_startup(struct usb_serial *serial)
 {
 	/* CP2101 buffers behave strangely unless device is reset */
diff --git a/drivers/usb/serial/cp2101.h b/drivers/usb/serial/cp2101.h
new file mode 100644
index 0000000..713c332
--- /dev/null
+++ b/drivers/usb/serial/cp2101.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright © 2008 Keith Packard <keithp@keithp.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef _CP2101_H_
+#define _CP2101_H_
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Write GPIO register */
+#define CP2101_REQTYPE_HOST_TO_DEVICE	0x40
+
+/* Read GPIO register */
+#define CP2101_REQTYPE_DEVICE_TO_HOST	0xc0
+
+#define CP2101_IOCTL_GPIOGET	_IOR('C', 1, unsigned char)
+#define CP2101_IOCTL_GPIOSET	_IOW('C', 2, unsigned short)
+
+#define CP2101_GPIO_MASK(bit)			(1 << (bit))
+#define CP2101_GPIO_VALUE(bit)			(0x100 << (bit))
+
+#define CP2101_GPIO_SET(bit,value)	((1 << (bit)) | ((value & 1) << ((bit) + 8)))
+
+#endif /* _CP2101_H_ */
-- 
1.5.6.5

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Alan Cox]

On Thu, 27 Nov 2008 00:29:36 -0800
Keith Packard <keithp@keithp.com> wrote:

> The cp2103 is programmed the same as the cp2101/cp2102 except for the
> addition of a set of four GPIO pins which can be directly controlled by the
> host. Access to this is done through a custom ioctl.

Only concern I have is that a custom ioctl means every time a new serial
chip grows GPIO pins we end up with more ioctls.

We have an LED class driver which might perhaps do the job but isn't
really oriented that way but would keep the tty and gpio pins separate
and available to different applications at the same time.

Failing that a rename to make it a generic tty gpio ioctl might be more
futurerpoof ?

Alan

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Keith Packard]

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

On Thu, 2008-11-27 at 10:58 +0000, Alan Cox wrote:

> Only concern I have is that a custom ioctl means every time a new serial
> chip grows GPIO pins we end up with more ioctls.

I'm not sure we'll find a lot of serial chips with GPIO pins attached...

> We have an LED class driver which might perhaps do the job but isn't
> really oriented that way but would keep the tty and gpio pins separate
> and available to different applications at the same time.

Yeah, I wondered about doing it that way, although not using the LED
driver as GPIO is bi-directional. There is an existing GPIO subsystem
which makes gpios available within the kernel, but does not expose them
up to user land except through the /sys/class interface.

> Failing that a rename to make it a generic tty gpio ioctl might be more
> futurerpoof ?

Perhaps exposing the existing gpio kernel infrastructure into /dev in
some fashion would make sense? We'd have to create some kind of
'protocol' for the write operation that would mark which bits to change,
and perhaps some way of explicitly setting pins to tri-state.

That would have the benefit of making gpio-based drivers possible from
user-mode, which is exactly what I'm doing here (building an interface
to the debug port on a cc1111 chip).

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Greg KH]

On Thu, Nov 27, 2008 at 10:20:26AM -0800, Keith Packard wrote:
> On Thu, 2008-11-27 at 10:58 +0000, Alan Cox wrote:
> 
> > Only concern I have is that a custom ioctl means every time a new serial
> > chip grows GPIO pins we end up with more ioctls.
> 
> I'm not sure we'll find a lot of serial chips with GPIO pins attached...

Yes, there are a number of other ones already in use that have them.
What we have done in the past for them is just use a simple userspace
program/library that uses usbfs/libusb to access the device directly.

Can you do the same thing here as well?  Or do you also need to
send/recieve serial data through the device at the same time?

thanks,

greg k-h

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Keith Packard]

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

On Thu, 2008-11-27 at 10:41 -0800, Greg KH wrote:

> Can you do the same thing here as well?  Or do you also need to
> send/recieve serial data through the device at the same time?

I was hoping to use both at the same time; the target device has a
serial port that I'll need to get working eventually.

Besides, libusb scares me, but if that's the officially supported
mechanism, I can manage; I don't need it to go fast for this
application.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Greg KH]

On Thu, Nov 27, 2008 at 05:31:43PM -0800, Keith Packard wrote:
> On Thu, 2008-11-27 at 10:41 -0800, Greg KH wrote:
> 
> > Can you do the same thing here as well?  Or do you also need to
> > send/recieve serial data through the device at the same time?
> 
> I was hoping to use both at the same time; the target device has a
> serial port that I'll need to get working eventually.

Ick.  Ok, care to come up with some kind of hook into the gpio subsystem
of the kernel so we don't create a new user/kernel interface for
something that we already support?

> Besides, libusb scares me, but if that's the officially supported
> mechanism, I can manage; I don't need it to go fast for this
> application.

libusb can be very fast.  The new version handles multiple urbs in
flight, and threaded applications very easily.  You might want to take a
look at it again if it has been a while.

thanks,

greg k-h

Re: [PATCH] usb/serial/cp2101: Add support for cp2103 GPIO pins

[Keith Packard]

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

On Tue, 2008-12-02 at 23:12 -0800, Greg KH wrote:

> Ick.  Ok, care to come up with some kind of hook into the gpio subsystem
> of the kernel so we don't create a new user/kernel interface for
> something that we already support?

Yeah, it does seem like exposing GPIOs to user space would be really
useful. Coming up with credible semantics seems a bit tricky though, and
may not fit the read/write model that we know and love.

> libusb can be very fast.  The new version handles multiple urbs in
> flight, and threaded applications very easily.  You might want to take a
> look at it again if it has been a while.

Sigh. Yes, I can probably use libusb for this application. Not as easily
as I can hack up the kernel, but it should do the trick.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Arnd Bergmann]

On Thursday 27 November 2008, Keith Packard wrote:
> +static long serial_compat_ioctl(struct tty_struct *tty, struct file *file,
> +                              unsigned int cmd, unsigned long arg)
> +{
> +       struct usb_serial_port *port = tty->driver_data;
> +       long retval = -ENODEV;
> +
> +       dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);
> +
> +       WARN_ON(!port->port.count);
> +
> +       /* pass on to the driver specific version of this function
> +          if it is available */
> +       if (port->serial->type->compat_ioctl) {
> +               lock_kernel();
> +               retval = port->serial->type->compat_ioctl(tty, file, cmd, arg);
> +               unlock_kernel(); 

By convention, compat_ioctl functions should be called without the big kernel
lock held.

The usb-serial driver all still need to be converted to have their
ioctl function called without bkl.

> --- a/include/linux/usb/serial.h
> +++ b/include/linux/usb/serial.h
> @@ -228,6 +228,8 @@ struct usb_serial_driver {
>         int  (*write_room)(struct tty_struct *tty);
>         int  (*ioctl)(struct tty_struct *tty, struct file *file,
>                       unsigned int cmd, unsigned long arg);
> +       long  (*compat_ioctl)(struct tty_struct *tty, struct file *file,
> +                             unsigned int cmd, unsigned long arg);
>         void (*set_termios)(struct tty_struct *tty,
>                         struct usb_serial_port *port, struct ktermios *old);
>         void (*break_ctl)(struct tty_struct *tty, int break_state);

You should probably define compat_ioctl to return an int as well
so that it becomes possible to use the same function for both eventually.
Then again, we can also drop the file argument, which is entirely unused
in all the usb-serial ioctls.
Maybe the best way for now is to make the new compat_ioctl be
int compat_ioctl(struct tty_struct, unsigned int cmd, unsigned long arg);
and leave the old ioctl as it is. When we get around to pushing the
BKL down into ->ioctl, we can change the prototype at the same time
to warn potential out-of-tree drivers.

	Arnd <><

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Keith Packard]

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

On Thu, 2008-11-27 at 15:31 +0100, Arnd Bergmann wrote:
> On Thursday 27 November 2008, Keith Packard wrote:
> > +static long serial_compat_ioctl(struct tty_struct *tty, struct file *file,
> > +                              unsigned int cmd, unsigned long arg)
> > +{
> > +       struct usb_serial_port *port = tty->driver_data;
> > +       long retval = -ENODEV;
> > +
> > +       dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);
> > +
> > +       WARN_ON(!port->port.count);
> > +
> > +       /* pass on to the driver specific version of this function
> > +          if it is available */
> > +       if (port->serial->type->compat_ioctl) {
> > +               lock_kernel();
> > +               retval = port->serial->type->compat_ioctl(tty, file, cmd, arg);
> > +               unlock_kernel(); 
> 
> By convention, compat_ioctl functions should be called without the big kernel
> lock held.

Thanks - I didn't realize these were supposed to be different.

> You should probably define compat_ioctl to return an int as well
> so that it becomes possible to use the same function for both eventually.

The existing tty layer compat_ioctl is defined to return long; is that
also wrong?

> Then again, we can also drop the file argument, which is entirely unused
> in all the usb-serial ioctls.

I'm not sure it's worth the effort; if some future usb serial ioctl
needs the argument, we'd end up changing every existing driver back. I
assume there are some serial devices for which the file is relevant
during ioctl.

> Maybe the best way for now is to make the new compat_ioctl be
> int compat_ioctl(struct tty_struct, unsigned int cmd, unsigned long arg);
> and leave the old ioctl as it is. When we get around to pushing the
> BKL down into ->ioctl, we can change the prototype at the same time
> to warn potential out-of-tree drivers.

Let's figure out what the right compat_ioctl interface is first and
worry about fixing the existing ioctl interface later.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Arnd Bergmann]

On Thursday 27 November 2008, Keith Packard wrote:
> On Thu, 2008-11-27 at 15:31 +0100, Arnd Bergmann wrote:
>
> > You should probably define compat_ioctl to return an int as well
> > so that it becomes possible to use the same function for both eventually.
> 
> The existing tty layer compat_ioctl is defined to return long; is that
> also wrong?

Yes, this was a slight mistake that got introduced when file_operations->
compat_ioctl first appeared with a long return value. It doesn't really
hurt, but new compat_ioctl functions should just return the 'int' that
gets sent back to user space.

> > Then again, we can also drop the file argument, which is entirely unused
> > in all the usb-serial ioctls.
> 
> I'm not sure it's worth the effort; if some future usb serial ioctl
> needs the argument, we'd end up changing every existing driver back. I
> assume there are some serial devices for which the file is relevant
> during ioctl.

I've just checked all tty device drivers. The only one using the file
pointer in ->ioctl, ->tiocmget and ->tiocmset is serial_core, which
passes it to tty_hung_up_p(). That could easily be changed to check
(tty->flags & TTY_HUPPED) AFAICT.

Maybe Alan can comment on this. I think the code would become cleaner
if we dropped the file argument to these three functions throughout
the tty layer, but it's probably not worth the pain to change themm
call.

> > Maybe the best way for now is to make the new compat_ioctl be
> > int compat_ioctl(struct tty_struct, unsigned int cmd, unsigned long arg);
> > and leave the old ioctl as it is. When we get around to pushing the
> > BKL down into ->ioctl, we can change the prototype at the same time
> > to warn potential out-of-tree drivers.
> 
> Let's figure out what the right compat_ioctl interface is first and
> worry about fixing the existing ioctl interface later.

ok.

	Arnd <><

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Alan Cox]

On Fri, Nov 28, 2008 at 12:43:02PM +0100, Arnd Bergmann wrote:
> I've just checked all tty device drivers. The only one using the file
> pointer in ->ioctl, ->tiocmget and ->tiocmset is serial_core, which
> passes it to tty_hung_up_p(). That could easily be changed to check
> (tty->flags & TTY_HUPPED) AFAICT.
> 
> Maybe Alan can comment on this. I think the code would become cleaner
> if we dropped the file argument to these three functions throughout
> the tty layer, but it's probably not worth the pain to change themm
> call.

Removing it seems a bit pointless as someone will one day no doubt need
to put it back again

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Arnd Bergmann]

On Friday 28 November 2008, Alan Cox wrote:

> Removing it seems a bit pointless as someone will one day no doubt need
> to put it back again

Ok, so we should probably also have it in the new ->compat_ioctl call.

	Arnd <><

[PATCH] usb/serial: Add compat_ioctl pass-through

[Keith Packard]

USB serial devices with extended IOCTLs cannot be used in a 64-bit kernel
from 32-bit user space as the compat_ioctl path is missing. This adds a
pass-through so that drivers may offer this functionality. This requires
that all drivers actually implement a compat_ioctl function if they want to
support this operation.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/usb/serial/usb-serial.c |   20 ++++++++++++++++++++
 include/linux/usb/serial.h      |    4 ++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 794b5ff..c83a9f0 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -381,6 +381,25 @@ static int serial_ioctl(struct tty_struct *tty, struct file *file,
 	return retval;
 }
 
+static int serial_compat_ioctl(struct tty_struct *tty, struct file *file,
+			       unsigned int cmd, unsigned long arg)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	int retval = -ENODEV;
+
+	dbg("%s - port %d, cmd 0x%.4x", __func__, port->number, cmd);
+
+	WARN_ON(!port->port.count);
+
+	/* pass on to the driver specific version of this function
+	   if it is available */
+	if (port->serial->type->compat_ioctl) {
+		retval = port->serial->type->compat_ioctl(tty, file, cmd, arg);
+	} else
+		retval = -ENOIOCTLCMD;
+	return retval;
+}
+
 static void serial_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -1095,6 +1114,7 @@ static const struct tty_operations serial_ops = {
 	.write =		serial_write,
 	.write_room =		serial_write_room,
 	.ioctl =		serial_ioctl,
+	.compat_ioctl =		serial_compat_ioctl,
 	.set_termios =		serial_set_termios,
 	.throttle =		serial_throttle,
 	.unthrottle =		serial_unthrottle,
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index 0b8617a..02c4080 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -226,8 +226,12 @@ struct usb_serial_driver {
 			const unsigned char *buf, int count);
 	/* Called only by the tty layer */
 	int  (*write_room)(struct tty_struct *tty);
+	/* Called with bkl held */
 	int  (*ioctl)(struct tty_struct *tty, struct file *file,
 		      unsigned int cmd, unsigned long arg);
+	/* Called without bkl held */
+	int  (*compat_ioctl)(struct tty_struct *tty, struct file *file,
+			     unsigned int cmd, unsigned long arg);
 	void (*set_termios)(struct tty_struct *tty,
 			struct usb_serial_port *port, struct ktermios *old);
 	void (*break_ctl)(struct tty_struct *tty, int break_state);
-- 
1.5.6.5

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Alan Cox]

On Fri, 28 Nov 2008 14:28:02 -0800
Keith Packard <keithp@keithp.com> wrote:

> USB serial devices with extended IOCTLs cannot be used in a 64-bit kernel
> from 32-bit user space as the compat_ioctl path is missing. This adds a
> pass-through so that drivers may offer this functionality. This requires
> that all drivers actually implement a compat_ioctl function if they want to
> support this operation.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

Would be far better to keep compatible ioctls anyway however that aside
clearly it should be passed on.

> +static int serial_compat_ioctl(struct tty_struct *tty, struct file *file,
> +			       unsigned int cmd, unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	int retval = -ENODEV;

Can never be used only overwritten so why not replace with -ENOIOCTLCMD
and remove the else of the if

Also I get a CodingStyle whine about the port->serial-> side of the if
not needing { }

> +	/* Called with bkl held */
>  	int  (*ioctl)(struct tty_struct *tty, struct file *file,
>  		      unsigned int cmd, unsigned long arg);

No... the ioctl path for tty comes from unlocked_ioctl so the BKL is not
held and should not be held.

Alan

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Keith Packard]

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

On Fri, 2008-11-28 at 22:33 +0000, Alan Cox wrote:

> Can never be used only overwritten so why not replace with -ENOIOCTLCMD
> and remove the else of the if

Yeah, of course.

> Also I get a CodingStyle whine about the port->serial-> side of the if
> not needing { }

Oops. Changed when I removed the bkl calls.

> No... the ioctl path for tty comes from unlocked_ioctl so the BKL is not
> held and should not be held.

Oh yes, it is -- serial_ioctl does:

        if (port->serial->type->ioctl) {
                lock_kernel();
                retval = port->serial->type->ioctl(tty, file, cmd, arg);
                unlock_kernel();
        } else

I added the comment to make the difference in calling convention
documented in the header file at least, I didn't change the code.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Alan Cox]

> Oh yes, it is -- serial_ioctl does:
> 
>         if (port->serial->type->ioctl) {
>                 lock_kernel();
>                 retval = port->serial->type->ioctl(tty, file, cmd, arg);
>                 unlock_kernel();
>         } else
> 
> I added the comment to make the difference in calling convention
> documented in the header file at least, I didn't change the code.

Gak I thought I'd killed all of those.

Let me rephase it "It won't be next week" ;)

Alan

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Arjan van de Ven]

On Sat, 29 Nov 2008 01:10:57 +0000
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Oh yes, it is -- serial_ioctl does:
> > 
> >         if (port->serial->type->ioctl) {
> >                 lock_kernel();
> >                 retval = port->serial->type->ioctl(tty, file, cmd,
> > arg); unlock_kernel();
> >         } else
> > 
> > I added the comment to make the difference in calling convention
> > documented in the header file at least, I didn't change the code.
> 
> Gak I thought I'd killed all of those.
> 
> Let me rephase it "It won't be next week" ;)

it's a double BKL since it's also using .ioctl not .unlocked_ioctl
(but then again we should just over time merge the two into a .ioctl
which is unlocked ;-)


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] usb/serial: Add compat_ioctl pass-through

[Alan Cox]

> it's a double BKL since it's also using .ioctl not .unlocked_ioctl
> (but then again we should just over time merge the two into a .ioctl
> which is unlocked ;-)

tty_ioctl is an unlocked_ioctl method and that is what calls the ioctl
method of tty drivers.