* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Greg KH @ 2012-05-02 20:03 UTC (permalink / raw)
To: Preston Fick; +Cc: linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <1335845210-5147-3-git-send-email-preston.fick@silabs.com>
On Mon, Apr 30, 2012 at 11:06:50PM -0500, Preston Fick wrote:
> This patch adds support for GPIO for CP210x devices that support it through two
> IOCTLs to get or set the GPIO latch on a CP210x device. The specification for
> this can be found in Silicon Labs AN571 document on section 5.27.1-4.
>
> Signed-off-by: Preston Fick <preston.fick@silabs.com>
> ---
> drivers/usb/serial/cp210x.c | 98 +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 98 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index b3646b8..9d1e542 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -35,6 +35,8 @@
> */
> static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *);
> static void cp210x_close(struct usb_serial_port *);
> +static int cp210x_ioctl(struct tty_struct *tty,
> + unsigned int cmd, unsigned long arg);
> static void cp210x_get_termios(struct tty_struct *,
> struct usb_serial_port *port);
> static void cp210x_get_termios_port(struct usb_serial_port *port,
> @@ -175,6 +177,7 @@ static struct usb_serial_driver cp210x_device = {
> .bulk_out_size = 256,
> .open = cp210x_open,
> .close = cp210x_close,
> + .ioctl = cp210x_ioctl,
> .break_ctl = cp210x_break_ctl,
> .set_termios = cp210x_set_termios,
> .tiocmget = cp210x_tiocmget,
> @@ -195,6 +198,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
> #define CP2104_PARTNUM 0x04
> #define CP2105_PARTNUM 0x05
>
> +/* IOCTLs */
> +#define IOCTL_GPIOGET 0x8000
> +#define IOCTL_GPIOSET 0x8001
As Alan pointed out, we can't just add random ioctls for individual
drivers for this type of thing. We need to standardize on this.
Actually, why can't you use the GPIO subsystem for something like this?
Can't you export your device as both a usb-serial device and a gpio
device and have things work properly that way?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/3] usb: cp210x: Added in support to get and store part number
From: Greg KH @ 2012-05-02 20:04 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Preston Fick, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
preston.fick-S6d6foEdJf7QT0dZR+AlfA
In-Reply-To: <4FA00C49.7000105-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
On Tue, May 01, 2012 at 08:16:09PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 01-05-2012 8:06, Preston Fick wrote:
>
> >This change gets the part number of the device when the driver is loaded and
> >stores it in the private portion of the port structure. This addition will
> >allow for part specific functionality to be added to the driver if needed.
>
> >Signed-off-by: Preston Fick<preston.fick-S6d6foEdJf7QT0dZR+AlfA@public.gmane.org>
> >---
> > drivers/usb/serial/cp210x.c | 24 ++++++++++++++++++++++++
> > 1 files changed, 24 insertions(+), 0 deletions(-)
>
> >diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> >index e67ccf3..b3646b8 100644
> >--- a/drivers/usb/serial/cp210x.c
> >+++ b/drivers/usb/serial/cp210x.c
> [...]
> >@@ -862,6 +874,7 @@ static int cp210x_startup(struct usb_serial *serial)
> > {
> > struct cp210x_port_private *port_priv;
> > int i;
> >+ unsigned int partNum;
> >
> > /* cp210x buffers behave strangely unless device is reset */
> > usb_reset_device(serial->dev);
> >@@ -876,6 +889,17 @@ static int cp210x_startup(struct usb_serial *serial)
> > serial->interface->cur_altsetting->desc.bInterfaceNumber;
> >
> > usb_set_serial_port_data(serial->port[i], port_priv);
> >+
> >+ /* Get the 1-byte part number of the cp210x device */
> >+ usb_control_msg(serial->dev,
>
> This may involve DMA...
>
> >+ usb_rcvctrlpipe(serial->dev, 0),
> >+ CP210X_VENDOR_SPECIFIC,
> >+ REQTYPE_DEVICE_TO_HOST,
> >+ CP210X_GET_PARTNUM,
> >+ port_priv->bInterfaceNumber,
> >+ &partNum, 1,
>
> You can't do DMA to a buffer situated on stack. You should
> kmalloc() the buffer.
You "must" kmalloc() the buffer, otherwise the driver will break on some
systems.
Sorry, I can't accept this patch because of this. Please redo it.
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [GIT PATCH] TTY VT bugfix for 3.4-rc5
From: Greg KH @ 2012-05-02 20:28 UTC (permalink / raw)
To: Linus Torvalds, Andrew Morton; +Cc: linux-kernel, linux-serial
The following changes since commit 69964ea4c7b68c9399f7977aa5b9aa6539a6a98a:
Linux 3.4-rc5 (2012-04-29 15:19:10 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/ tags/tty-3.4-rc6
for you to fetch changes up to 84f904ecd3aa2ccb5779b815b69c1cb592f07bb5:
vt: Fix deadlock on scroll-lock (2012-05-01 14:01:28 -0400)
----------------------------------------------------------------
TTY fix for 3.4-rc5
This is a deadlock bugfix that was easy to hit, and that the vt layer lock
rework got wrong, so it reverts the logic back to the way it was in 3.3 and
earlier kernels to prevent problems.
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
----------------------------------------------------------------
Alan Cox (1):
vt: Fix deadlock on scroll-lock
drivers/tty/vt/keyboard.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
^ permalink raw reply
* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Alan Cox @ 2012-05-02 20:49 UTC (permalink / raw)
To: Greg KH; +Cc: Preston Fick, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <20120502200356.GA25915@kroah.com>
> Actually, why can't you use the GPIO subsystem for something like this?
> Can't you export your device as both a usb-serial device and a gpio
> device and have things work properly that way?
You still need the ioctls even then in order to discover the gpio
numbers, and having done that youi've got potential races with unload
when you try and open them. You've also got permissions considerations
and synchronization between gpio and data problems.
It's not a good way to go. It might make sense in some platforms to
expose them as both but its not a good general model.
I'm currently favouring adding some 'additional control line' bits to
termiox.
^ permalink raw reply
* Re: [PATCH 06/06] serial8250-em: Add Emma Mobile UART driver
From: Greg KH @ 2012-05-02 21:22 UTC (permalink / raw)
To: Paul Gortmaker
Cc: Magnus Damm, linux-serial, horms, arnd, linux-sh, swarren,
linux-kernel, rjw, lethal, olof, dan.j.williams, alan
In-Reply-To: <4FA14788.6090000@windriver.com>
On Wed, May 02, 2012 at 10:41:12AM -0400, Paul Gortmaker wrote:
> On 12-05-02 08:47 AM, Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> >
> > Add a platform driver for Emma Mobile 8250 hardware.
>
>
> [...]
>
> > +
> > +config SERIAL_8250_EM
> > + tristate "Support for Emma Mobile intergrated serial port"
> > + depends on SERIAL_8250
>
> Should we depend on arch=ARM or something similar? Even if the
> driver is written to be arch independent, if the Emma itself is
> physically tied to ARM platforms, we probably don't need to be
> building it for sparc allyesconfig etc.
Yeah, it doesn't build on x86-64, I get the following build problems:
ERROR: "clk_get_rate" [drivers/tty/serial/8250/8250_em.ko] undefined!
ERROR: "clk_enable" [drivers/tty/serial/8250/8250_em.ko] undefined!
ERROR: "clk_get" [drivers/tty/serial/8250/8250_em.ko] undefined!
ERROR: "clk_put" [drivers/tty/serial/8250/8250_em.ko] undefined!
ERROR: "clk_disable" [drivers/tty/serial/8250/8250_em.ko] undefined!
So it should depend on some arch that has clk at the least.
Magnus, care to redo just this one patch that fixes this up? I've
applied the first 5 already.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Greg KH @ 2012-05-02 21:52 UTC (permalink / raw)
To: Alan Cox
Cc: Preston Fick, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <20120502214901.6b6e031c@pyramind.ukuu.org.uk>
On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > Actually, why can't you use the GPIO subsystem for something like this?
> > Can't you export your device as both a usb-serial device and a gpio
> > device and have things work properly that way?
>
> You still need the ioctls even then in order to discover the gpio
> numbers
What discovery? The device knows what gpio values it has in it, and
should be able to register those with the gpio subsystem.
> and having done that youi've got potential races with unload
> when you try and open them. You've also got permissions considerations
> and synchronization between gpio and data problems.
That can be handled in the driver itself, if it really is a problem (the
existing patch sure didn't handle any of that at all, so I'm guessing
either it wasn't considered, or it isn't a problem.)
> It's not a good way to go. It might make sense in some platforms to
> expose them as both but its not a good general model.
Why isn't the gpio subsystem a good general model? I thought that is
what it was created to solve?
> I'm currently favouring adding some 'additional control line' bits to
> termiox.
Yes, but that's only good for usb-serial devices that also have gpio
pins on the controller side. Which seems pretty limited to me.
If I have a userspace program, and I want to use GPIO, I shouldn't have
to care/know that the pins are really on the end of a USB->serial
bridge, or somewhere hanging off of a SOC, the same userspace api should
"just work", right?
Or am I missing something really obvious here?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Alan Cox @ 2012-05-02 22:10 UTC (permalink / raw)
To: Greg KH; +Cc: Preston Fick, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <20120502215226.GD27281@kroah.com>
On Wed, 2 May 2012 14:52:26 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > > Actually, why can't you use the GPIO subsystem for something like this?
> > > Can't you export your device as both a usb-serial device and a gpio
> > > device and have things work properly that way?
> >
> > You still need the ioctls even then in order to discover the gpio
> > numbers
>
> What discovery? The device knows what gpio values it has in it, and
> should be able to register those with the gpio subsystem.
You open /dev/ttyUSB0
Ok now in your user application how are you going to find which gpio
numbers to use that are associated with this specific port, and how is
udev going to do that to manage permissions ?
So you need an ioctl to give you the range that is mapped to this (or a
sysfs node, but the sysfs node makes the security problem pretty
much insoluble)
> > and having done that youi've got potential races with unload
> > when you try and open them. You've also got permissions considerations
> > and synchronization between gpio and data problems.
>
> That can be handled in the driver itself, if it really is a problem (the
> existing patch sure didn't handle any of that at all, so I'm guessing
> either it wasn't considered, or it isn't a problem.)
Not reliably
open /dev/ttyUSB0 [or sysfs node]
read gpio numbers
close
open gpio foo
Oh dear... so random shell scripting user is going to screw up horribly.
> > It's not a good way to go. It might make sense in some platforms to
> > expose them as both but its not a good general model.
>
> Why isn't the gpio subsystem a good general model? I thought that is
> what it was created to solve?
gpio provides a flat abstraction for arbitary pins on a platform. It's
really oriented to fairly fixed system stuff. It doesn't provide a useful
abstraction for extra carrier lines.
Imagine if RTS and DTR were driven by GPIO pins instead of the tty
layer. Many devices use them as extra magic GPIO lines not as tty control
lines, so its an equivalent argument.
> > I'm currently favouring adding some 'additional control line' bits to
> > termiox.
>
> Yes, but that's only good for usb-serial devices that also have gpio
> pins on the controller side. Which seems pretty limited to me.
>
> If I have a userspace program, and I want to use GPIO, I shouldn't have
> to care/know that the pins are really on the end of a USB->serial
> bridge, or somewhere hanging off of a SOC, the same userspace api should
> "just work", right?
For certain applications that makes more sense, which is why I said you
may well want to expose it as both. However you still need both, and
ideally we need a standardised pattern of assignments for line
disciplines that use the extra control lines (which has come up recently
for another device with extra lines)
I think it basically boils down to this
If you have a serial port with some gpio lines that drive arbitary
unrelated electronics then the gpio interface is handy because you can
use the same code logic as if it was wired to other pins elsewhere
If you are using them as part of the tty interface as extra control lines
(eg for smartcard protocols) then you want them driven via the tty
interface and doubly so once we add some of the smartcard/sim ldisc
support.
Hence we really need to expose them both ways because end users are doing
both things with the gpio pins on these ports.
So I'd suggest we expose them via termiox bits and also via the tty
providing gpio range info in a standardised way.
Alan
^ permalink raw reply
* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Greg KH @ 2012-05-02 22:27 UTC (permalink / raw)
To: Alan Cox
Cc: Preston Fick, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <20120502231027.1c45105d@pyramind.ukuu.org.uk>
On Wed, May 02, 2012 at 11:10:27PM +0100, Alan Cox wrote:
> On Wed, 2 May 2012 14:52:26 -0700
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Wed, May 02, 2012 at 09:49:01PM +0100, Alan Cox wrote:
> > > > Actually, why can't you use the GPIO subsystem for something like this?
> > > > Can't you export your device as both a usb-serial device and a gpio
> > > > device and have things work properly that way?
> > >
> > > You still need the ioctls even then in order to discover the gpio
> > > numbers
> >
> > What discovery? The device knows what gpio values it has in it, and
> > should be able to register those with the gpio subsystem.
>
> You open /dev/ttyUSB0
>
> Ok now in your user application how are you going to find which gpio
> numbers to use that are associated with this specific port,
Just look at the gpio device that has ttyUSB0 as its parent.
> and how is udev going to do that to manage permissions ?
How does udev handle permissions for gpio devices today?
> So you need an ioctl to give you the range that is mapped to this (or a
> sysfs node, but the sysfs node makes the security problem pretty
> much insoluble)
range for what? Doesn't the gpio interfac provide the size of the gpio
registers to userspace?
As for security, how is that handled today with gpio devices?
> > > and having done that youi've got potential races with unload
> > > when you try and open them. You've also got permissions considerations
> > > and synchronization between gpio and data problems.
> >
> > That can be handled in the driver itself, if it really is a problem (the
> > existing patch sure didn't handle any of that at all, so I'm guessing
> > either it wasn't considered, or it isn't a problem.)
>
> Not reliably
>
> open /dev/ttyUSB0 [or sysfs node]
> read gpio numbers
> close
>
> open gpio foo
>
> Oh dear... so random shell scripting user is going to screw up horribly.
What's the odds that the data going across the tty link corrisponds with
the gpio control?
And shell scripting the gpio interface is used today, I've seen it on
the beaglebone machine.
> > > It's not a good way to go. It might make sense in some platforms to
> > > expose them as both but its not a good general model.
> >
> > Why isn't the gpio subsystem a good general model? I thought that is
> > what it was created to solve?
>
> gpio provides a flat abstraction for arbitary pins on a platform. It's
> really oriented to fairly fixed system stuff. It doesn't provide a useful
> abstraction for extra carrier lines.
>
> Imagine if RTS and DTR were driven by GPIO pins instead of the tty
> layer. Many devices use them as extra magic GPIO lines not as tty control
> lines, so its an equivalent argument.
Ok, fair enough, but RTS and DTR are "well defined" as part of the RS232
specs, and handle flow of the data itself. These gpio lines are not tty
control, but as you point out, used for other things, so keeping them
separate from the tty node seems to make sense to me.
> > > I'm currently favouring adding some 'additional control line' bits to
> > > termiox.
> >
> > Yes, but that's only good for usb-serial devices that also have gpio
> > pins on the controller side. Which seems pretty limited to me.
> >
> > If I have a userspace program, and I want to use GPIO, I shouldn't have
> > to care/know that the pins are really on the end of a USB->serial
> > bridge, or somewhere hanging off of a SOC, the same userspace api should
> > "just work", right?
>
> For certain applications that makes more sense, which is why I said you
> may well want to expose it as both. However you still need both, and
> ideally we need a standardised pattern of assignments for line
> disciplines that use the extra control lines (which has come up recently
> for another device with extra lines)
>
> I think it basically boils down to this
>
> If you have a serial port with some gpio lines that drive arbitary
> unrelated electronics then the gpio interface is handy because you can
> use the same code logic as if it was wired to other pins elsewhere
>
> If you are using them as part of the tty interface as extra control lines
> (eg for smartcard protocols) then you want them driven via the tty
> interface and doubly so once we add some of the smartcard/sim ldisc
> support.
>
> Hence we really need to expose them both ways because end users are doing
> both things with the gpio pins on these ports.
>
> So I'd suggest we expose them via termiox bits and also via the tty
> providing gpio range info in a standardised way.
Ok, I'll wait for your proposed standardised way before complaining any
more :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] usb: cp210x: Add ioctl for GPIO support
From: Alan Cox @ 2012-05-02 22:59 UTC (permalink / raw)
To: Greg KH; +Cc: Preston Fick, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <20120502222728.GA2871@kroah.com>
> > Ok now in your user application how are you going to find which gpio
> > numbers to use that are associated with this specific port,
>
> Just look at the gpio device that has ttyUSB0 as its parent.
Taking care of course that you keep the ttyUSB file handle open while you
do so, ugly from user space, hideous from kernel space.
>
> > and how is udev going to do that to manage permissions ?
>
> How does udev handle permissions for gpio devices today?
It sticks its fingers in its ears and goes "la la la"
> > So you need an ioctl to give you the range that is mapped to this (or a
> > sysfs node, but the sysfs node makes the security problem pretty
> > much insoluble)
>
> range for what? Doesn't the gpio interfac provide the size of the gpio
> registers to userspace?
If you fish them out via sysfs trees yes.
>> > open /dev/ttyUSB0 [or sysfs node]
> > read gpio numbers
> > close
> >
> > open gpio foo
> >
> > Oh dear... so random shell scripting user is going to screw up horribly.
>
> What's the odds that the data going across the tty link corrisponds with
> the gpio control?
I think you missed the problem - which speaks volumes for the interface
issue
open sysfs node
read gpio numbers
close
unplug
new device
assigned the same gpio numbers
open gpio
whoops - that was the milling machine not the smartcard
> And shell scripting the gpio interface is used today, I've seen it on
> the beaglebone machine.
Yes it works very well but that isn't the issue.
> > I think it basically boils down to this
> >
> > If you have a serial port with some gpio lines that drive arbitary
> > unrelated electronics then the gpio interface is handy because you can
> > use the same code logic as if it was wired to other pins elsewhere
> >
> > If you are using them as part of the tty interface as extra control lines
> > (eg for smartcard protocols) then you want them driven via the tty
> > interface and doubly so once we add some of the smartcard/sim ldisc
> > support.
> >
> > Hence we really need to expose them both ways because end users are doing
> > both things with the gpio pins on these ports.
> >
> > So I'd suggest we expose them via termiox bits and also via the tty
> > providing gpio range info in a standardised way.
>
> Ok, I'll wait for your proposed standardised way before complaining any
> more :)
I think I'd suggest we support the following
open /dev/ttyUSB0
get gpio info somehow
run via gpio interface
close /dev/ttyUSB0
and
open /dev/ttyUSB0
via termiox( what gpios do you have )
via termiox( set/get gpio values)
close /dev/ttyUSB0
that would support the ldisc use of them. I need to go read the specs on
that and look at some hardware. I think we may need a mapping ioctl too
because the pin allocations may need to be described in terms of "pin 0
is this reader signal", "pin 1 is that"
Alan
^ permalink raw reply
* Re: 3.4-rc5: usbserial pl2303 attached via Texas Instruments USB3.0 chip Oopses
From: Sarah Sharp @ 2012-05-02 23:46 UTC (permalink / raw)
To: Oliver Neukum
Cc: Martin Mokrejs, linux-usb@vger.kernel.org, gregkh, linux-serial
In-Reply-To: <201205021444.07823.oneukum@suse.de>
On Wed, May 02, 2012 at 02:44:07PM +0200, Oliver Neukum wrote:
> Am Mittwoch, 2. Mai 2012, 12:48:51 schrieb Martin Mokrejs:
> > > Does it work if you boot with nomsi?
> >
> > Yes, dmesg attached.
>
> Sorry, I was referring to pci=nomsi
>
> So it works if you simply add "nomsi" to the kernel command line?
> Can you check whether this is repeatable?
If it's repeatable, we can just add the XHCI_BROKEN_MSI quirk to that TI
host.
Sarah Sharp
^ permalink raw reply
* Re: [PATCH 06/06] serial8250-em: Add Emma Mobile UART driver
From: Arnd Bergmann @ 2012-05-03 8:46 UTC (permalink / raw)
To: Greg KH
Cc: Paul Gortmaker, Magnus Damm, linux-serial, horms, linux-sh,
swarren, linux-kernel, rjw, lethal, olof, dan.j.williams, alan
In-Reply-To: <20120502212205.GA12212@kroah.com>
On Wednesday 02 May 2012, Greg KH wrote:
> > > +
> > > +config SERIAL_8250_EM
> > > + tristate "Support for Emma Mobile intergrated serial port"
> > > + depends on SERIAL_8250
> >
> > Should we depend on arch=ARM or something similar? Even if the
> > driver is written to be arch independent, if the Emma itself is
> > physically tied to ARM platforms, we probably don't need to be
> > building it for sparc allyesconfig etc.
>
> Yeah, it doesn't build on x86-64, I get the following build problems:
> ERROR: "clk_get_rate" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_enable" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_get" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_put" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_disable" [drivers/tty/serial/8250/8250_em.ko] undefined!
>
> So it should depend on some arch that has clk at the least.
>
> Magnus, care to redo just this one patch that fixes this up? I've
> applied the first 5 already.
I think it's best to make it depend on HAVE_CLK, but depending on
ARM && HAVE_CLK is also fine with me. I would not like to have it depend
on EMMA specifically though because I want to increase built coverage.
Arnd
^ permalink raw reply
* Re: [PATCH 1/3] usb: cp210x: Corrected USB request type definitions
From: Bjørn Mork @ 2012-05-03 8:58 UTC (permalink / raw)
To: Preston Fick; +Cc: gregkh, linux-usb, linux-kernel, linux-serial, preston.fick
In-Reply-To: <1335845210-5147-1-git-send-email-preston.fick@silabs.com>
Preston Fick <pffick@gmail.com> writes:
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index ec30f95..e67ccf3 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -188,8 +188,10 @@ static struct usb_serial_driver * const serial_drivers[] = {
> };
>
> /* Config request types */
> -#define REQTYPE_HOST_TO_DEVICE 0x41
> -#define REQTYPE_DEVICE_TO_HOST 0xc1
> +#define REQTYPE_HOST_TO_INTERFACE 0x41
> +#define REQTYPE_INTERFACE_TO_HOST 0xc1
> +#define REQTYPE_HOST_TO_DEVICE 0x40
> +#define REQTYPE_DEVICE_TO_HOST 0xc0
Any particular reason you need to define these instead of just using the
standard flags from linux/usb/ch9.h directly in the requests?:
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT)
(USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN)
If nothing else, using those from the beginning would have avoided the
mis-labelling you are fixing up.
Bjørn
^ permalink raw reply
* Re: 3.4-rc5: usbserial pl2303 attached via Texas Instruments USB3.0 chip Oopses
From: Martin Mokrejs @ 2012-05-03 9:14 UTC (permalink / raw)
To: Sarah Sharp
Cc: Oliver Neukum, Martin Mokrejs, linux-usb@vger.kernel.org, gregkh,
linux-serial
In-Reply-To: <20120502234639.GH22341@xanatos>
Sarah Sharp wrote:
> On Wed, May 02, 2012 at 02:44:07PM +0200, Oliver Neukum wrote:
>> Am Mittwoch, 2. Mai 2012, 12:48:51 schrieb Martin Mokrejs:
>>>> Does it work if you boot with nomsi?
>>>
>>> Yes, dmesg attached.
>>
>> Sorry, I was referring to pci=nomsi
>>
>> So it works if you simply add "nomsi" to the kernel command line?
>> Can you check whether this is repeatable?
>
> If it's repeatable, we can just add the XHCI_BROKEN_MSI quirk to that TI
> host.
I have to figure out what I did to my .config that it is not repeatable anymore,
at all. :(( I will come back to you later when I repeat that. Thanks.
Martin
^ permalink raw reply
* [PATCH] serial8250-em: Emma Mobile UART driver V2
From: Magnus Damm @ 2012-05-03 12:13 UTC (permalink / raw)
To: linux-serial
Cc: horms, arnd, linux-sh, gregkh, swarren, linux-kernel, rjw,
paul.gortmaker, lethal, olof, Magnus Damm, dan.j.williams, alan
From: Magnus Damm <damm@opensource.se>
This is V2 of the Emma Mobile 8250 platform driver.
The hardware itself has according to the data sheet
up to 64 byte FIFOs but at this point we only make
use of the 16550 compatible mode.
To support this piece of hardware the common UART
registers need to be remapped, and the access size
differences need to be handled.
The DLL and DLM registers can due to offset collision
not be remapped easily, and because of that this
driver makes use of ->dl_read() and ->dl_write()
callbacks. This in turn requires a registration
function that takes 8250-specific paramenters.
Future potential enhancements include DT support,
early platform driver console and fine grained PM.
Signed-off-by: Magnus Damm <damm@opensource.se>
---
Developed for the UARTs in Emma Mobile EV2, but should
in theory also support other SoCs in the Emma Mobile line.
Changes since V1:
- Depend on ARM && HAVE_CLK - thanks Paul, Greg and Arnd!
- Always use UPF_IOREMAP to support a wider range of hardware.
V1 got feedback from the following people, thanks!
- Acked-by: Alan Cox <alan@linux.intel.com>
- Acked-by: Arnd Bergmann <arnd@arndb.de>
Depends on the first 5 patches in the following series:
"[PATCH 00/06] serial8250: DLL/DLM rework, Emma Mobile UART driver"
drivers/tty/serial/8250/8250_em.c | 178 +++++++++++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 8 +
drivers/tty/serial/8250/Makefile | 1
3 files changed, 187 insertions(+)
--- /dev/null
+++ work/drivers/tty/serial/8250/8250_em.c 2012-05-03 18:12:18.000000000 +0900
@@ -0,0 +1,178 @@
+/*
+ * Renesas Emma Mobile 8250 driver
+ *
+ * Copyright (C) 2012 Magnus Damm
+ *
+ * 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
+ *
+ * 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
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/serial_8250.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+
+#include "8250.h"
+
+#define UART_DLL_EM 9
+#define UART_DLM_EM 10
+
+struct serial8250_em_priv {
+ struct clk *sclk;
+ int line;
+};
+
+static void serial8250_em_serial_out(struct uart_port *p, int offset, int value)
+{
+ switch (offset) {
+ case UART_TX: /* TX @ 0x00 */
+ writeb(value, p->membase);
+ break;
+ case UART_FCR: /* FCR @ 0x0c (+1) */
+ case UART_LCR: /* LCR @ 0x10 (+1) */
+ case UART_MCR: /* MCR @ 0x14 (+1) */
+ case UART_SCR: /* SCR @ 0x20 (+1) */
+ writel(value, p->membase + ((offset + 1) << 2));
+ break;
+ case UART_IER: /* IER @ 0x04 */
+ value &= 0x0f; /* only 4 valid bits - not Xscale */
+ /* fall-through */
+ case UART_DLL_EM: /* DLL @ 0x24 (+9) */
+ case UART_DLM_EM: /* DLM @ 0x28 (+9) */
+ writel(value, p->membase + (offset << 2));
+ }
+}
+
+static unsigned int serial8250_em_serial_in(struct uart_port *p, int offset)
+{
+ switch (offset) {
+ case UART_RX: /* RX @ 0x00 */
+ return readb(p->membase);
+ case UART_MCR: /* MCR @ 0x14 (+1) */
+ case UART_LSR: /* LSR @ 0x18 (+1) */
+ case UART_MSR: /* MSR @ 0x1c (+1) */
+ case UART_SCR: /* SCR @ 0x20 (+1) */
+ return readl(p->membase + ((offset + 1) << 2));
+ case UART_IER: /* IER @ 0x04 */
+ case UART_IIR: /* IIR @ 0x08 */
+ case UART_DLL_EM: /* DLL @ 0x24 (+9) */
+ case UART_DLM_EM: /* DLM @ 0x28 (+9) */
+ return readl(p->membase + (offset << 2));
+ }
+ return 0;
+}
+
+static int serial8250_em_serial_dl_read(struct uart_8250_port *up)
+{
+ return serial_in(up, UART_DLL_EM) | serial_in(up, UART_DLM_EM) << 8;
+}
+
+static void serial8250_em_serial_dl_write(struct uart_8250_port *up, int value)
+{
+ serial_out(up, UART_DLL_EM, value & 0xff);
+ serial_out(up, UART_DLM_EM, value >> 8 & 0xff);
+}
+
+static int __devinit serial8250_em_probe(struct platform_device *pdev)
+{
+ struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ struct serial8250_em_priv *priv;
+ struct uart_8250_port up;
+ int ret = -EINVAL;
+
+ if (!regs || !irq) {
+ dev_err(&pdev->dev, "missing registers or irq\n");
+ goto err0;
+ }
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ dev_err(&pdev->dev, "unable to allocate private data\n");
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ priv->sclk = clk_get(&pdev->dev, "sclk");
+ if (!priv->sclk) {
+ dev_err(&pdev->dev, "unable to get clock\n");
+ goto err1;
+ }
+
+ memset(&up, 0, sizeof(up));
+ up.port.mapbase = regs->start;
+ up.port.irq = irq->start;
+ up.port.type = PORT_UNKNOWN;
+ up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_IOREMAP;
+ up.port.dev = &pdev->dev;
+ up.port.private_data = priv;
+
+ clk_enable(priv->sclk);
+ up.port.uartclk = clk_get_rate(priv->sclk);
+
+ up.port.iotype = UPIO_MEM32;
+ up.port.serial_in = serial8250_em_serial_in;
+ up.port.serial_out = serial8250_em_serial_out;
+ up.dl_read = serial8250_em_serial_dl_read;
+ up.dl_write = serial8250_em_serial_dl_write;
+
+ ret = serial8250_register_8250_port(&up);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "unable to register 8250 port\n");
+ goto err2;
+ }
+
+ priv->line = ret;
+ platform_set_drvdata(pdev, priv);
+ return 0;
+
+ err2:
+ clk_disable(priv->sclk);
+ clk_put(priv->sclk);
+ err1:
+ kfree(priv);
+ err0:
+ return ret;
+}
+
+static int __devexit serial8250_em_remove(struct platform_device *pdev)
+{
+ struct serial8250_em_priv *priv = platform_get_drvdata(pdev);
+
+ serial8250_unregister_port(priv->line);
+ clk_disable(priv->sclk);
+ clk_put(priv->sclk);
+ kfree(priv);
+ return 0;
+}
+
+static struct platform_driver serial8250_em_platform_driver = {
+ .driver = {
+ .name = "serial8250-em",
+ .owner = THIS_MODULE,
+ },
+ .probe = serial8250_em_probe,
+ .remove = __devexit_p(serial8250_em_remove),
+};
+
+module_platform_driver(serial8250_em_platform_driver);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas Emma Mobile 8250 Driver");
+MODULE_LICENSE("GPL v2");
--- 0001/drivers/tty/serial/8250/Kconfig
+++ work/drivers/tty/serial/8250/Kconfig 2012-05-03 15:19:23.000000000 +0900
@@ -278,3 +278,11 @@ config SERIAL_8250_DW
help
Selecting this option will enable handling of the extra features
present in the Synopsys DesignWare APB UART.
+
+config SERIAL_8250_EM
+ tristate "Support for Emma Mobile intergrated serial port"
+ depends on SERIAL_8250 && ARM && HAVE_CLK
+ help
+ Selecting this option will add support for the integrated serial
+ port hardware found on the Emma Mobile line of processors.
+ If unsure, say N.
--- 0001/drivers/tty/serial/8250/Makefile
+++ work/drivers/tty/serial/8250/Makefile 2012-05-03 15:19:23.000000000 +0900
@@ -18,3 +18,4 @@ obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_
obj-$(CONFIG_SERIAL_8250_MCA) += 8250_mca.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
+obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
^ permalink raw reply
* Re: [PATCH 06/06] serial8250-em: Add Emma Mobile UART driver
From: Magnus Damm @ 2012-05-03 12:19 UTC (permalink / raw)
To: Greg KH
Cc: Paul Gortmaker, linux-serial, horms, arnd, linux-sh, swarren,
linux-kernel, rjw, lethal, olof, dan.j.williams, alan
In-Reply-To: <20120502212205.GA12212@kroah.com>
On Thu, May 3, 2012 at 6:22 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, May 02, 2012 at 10:41:12AM -0400, Paul Gortmaker wrote:
>> On 12-05-02 08:47 AM, Magnus Damm wrote:
>> > From: Magnus Damm <damm@opensource.se>
>> >
>> > Add a platform driver for Emma Mobile 8250 hardware.
>>
>>
>> [...]
>>
>> > +
>> > +config SERIAL_8250_EM
>> > + tristate "Support for Emma Mobile intergrated serial port"
>> > + depends on SERIAL_8250
>>
>> Should we depend on arch=ARM or something similar? Even if the
>> driver is written to be arch independent, if the Emma itself is
>> physically tied to ARM platforms, we probably don't need to be
>> building it for sparc allyesconfig etc.
>
> Yeah, it doesn't build on x86-64, I get the following build problems:
> ERROR: "clk_get_rate" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_enable" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_get" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_put" [drivers/tty/serial/8250/8250_em.ko] undefined!
> ERROR: "clk_disable" [drivers/tty/serial/8250/8250_em.ko] undefined!
>
> So it should depend on some arch that has clk at the least.
>
> Magnus, care to redo just this one patch that fixes this up? I've
> applied the first 5 already.
Done!
I decided to go with SERIAL_8250 && ARM && HAVE_CLK to limit the
exposure a bit but not hinder any single ARM kernel image work. Hope
this balance is making everyone happy.
It is not impossible that this driver can be used on older MIPS based
SoCs from NEC, but that can easily be dealt with later if needed.
Thanks for your help!
/ magnus
^ permalink raw reply
* [PATCH 1/3] RFC: Solved unnecessary schedule latency in the TTY layer (1/3)
From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben
Solved unnecessary schedule latency in the TTY layer when used in
realtime context:
In case of PREEMPT_RT or when low_latency flag is set by the serial driver
the TTY receive flip buffer is copied to the line discipline directly
instead of using a workqueue in the background. Therefor only in case a
workqueue is actually used for copying data to the line discipline
we'll have to wait for the workqueue to finish. For a PREEMPT system this
prevents us from unnecessary blocking by the workqueue spin lock.
Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and
no interrupts (and therefor no scheduling) is disabled.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
drivers/tty/tty_buffer.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 6c9b7cd..ed86359 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -317,12 +317,7 @@ EXPORT_SYMBOL(tty_insert_flip_string_flags);
void tty_schedule_flip(struct tty_struct *tty)
{
- unsigned long flags;
- spin_lock_irqsave(&tty->buf.lock, flags);
- if (tty->buf.tail != NULL)
- tty->buf.tail->commit = tty->buf.tail->used;
- spin_unlock_irqrestore(&tty->buf.lock, flags);
- schedule_work(&tty->buf.work);
+ tty_flip_buffer_push(tty);
}
EXPORT_SYMBOL(tty_schedule_flip);
@@ -469,7 +464,16 @@ static void flush_to_ldisc(struct work_struct *work)
*/
void tty_flush_to_ldisc(struct tty_struct *tty)
{
- flush_work(&tty->buf.work);
+ /*
+ * Only in case a workqueue is actually used for copying data to the
+ * line discipline, we'll have to wait for the workqueue to finish. In
+ * other cases this prevents us from unnecessary blocking by the workqueue
+ * spin lock.
+ */
+#ifndef CONFIG_PREEMPT_RT_FULL
+ if (!tty->low_latency)
+ flush_work(&tty->buf.work);
+#endif
}
/**
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3)
From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben
In-Reply-To: <1336048663-21882-1-git-send-email-meltedpianoman@gmail.com>
Solved unnecessary schedule latency in the TTY layer when used in
realtime context:
The global "normal" spin lock that guards the line discipline
administration is replaced by a raw spin lock.
Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and
no interrupts (and therefor no scheduling) is disabled.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
Conflicts:
drivers/tty/tty_ldisc.c
---
drivers/tty/tty_ldisc.c | 30 +++++++++++++++---------------
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..30836cc 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -26,7 +26,7 @@
* callers who will do ldisc lookups and cannot sleep.
*/
-static DEFINE_SPINLOCK(tty_ldisc_lock);
+static DEFINE_RAW_SPINLOCK(tty_ldisc_lock);
static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle);
/* Line disc dispatch table */
@@ -53,18 +53,18 @@ static void put_ldisc(struct tty_ldisc *ld)
* We really want an "atomic_dec_and_lock_irqsave()",
* but we don't have it, so this does it by hand.
*/
- local_irq_save(flags);
- if (atomic_dec_and_lock(&ld->users, &tty_ldisc_lock)) {
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
+ if (atomic_dec_and_test(&ld->users)) {
struct tty_ldisc_ops *ldo = ld->ops;
ldo->refcount--;
module_put(ldo->owner);
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
kfree(ld);
return;
}
- local_irq_restore(flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
wake_up(&tty_ldisc_idle);
}
@@ -89,11 +89,11 @@ int tty_register_ldisc(int disc, struct tty_ldisc_ops *new_ldisc)
if (disc < N_TTY || disc >= NR_LDISCS)
return -EINVAL;
- spin_lock_irqsave(&tty_ldisc_lock, flags);
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
tty_ldiscs[disc] = new_ldisc;
new_ldisc->num = disc;
new_ldisc->refcount = 0;
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
return ret;
}
@@ -119,12 +119,12 @@ int tty_unregister_ldisc(int disc)
if (disc < N_TTY || disc >= NR_LDISCS)
return -EINVAL;
- spin_lock_irqsave(&tty_ldisc_lock, flags);
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
if (tty_ldiscs[disc]->refcount)
ret = -EBUSY;
else
tty_ldiscs[disc] = NULL;
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
return ret;
}
@@ -135,7 +135,7 @@ static struct tty_ldisc_ops *get_ldops(int disc)
unsigned long flags;
struct tty_ldisc_ops *ldops, *ret;
- spin_lock_irqsave(&tty_ldisc_lock, flags);
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
ret = ERR_PTR(-EINVAL);
ldops = tty_ldiscs[disc];
if (ldops) {
@@ -145,7 +145,7 @@ static struct tty_ldisc_ops *get_ldops(int disc)
ret = ldops;
}
}
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
return ret;
}
@@ -153,10 +153,10 @@ static void put_ldops(struct tty_ldisc_ops *ldops)
{
unsigned long flags;
- spin_lock_irqsave(&tty_ldisc_lock, flags);
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
ldops->refcount--;
module_put(ldops->owner);
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
}
/**
@@ -286,11 +286,11 @@ static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty)
unsigned long flags;
struct tty_ldisc *ld;
- spin_lock_irqsave(&tty_ldisc_lock, flags);
+ raw_spin_lock_irqsave(&tty_ldisc_lock, flags);
ld = NULL;
if (test_bit(TTY_LDISC, &tty->flags))
ld = get_ldisc(tty->ldisc);
- spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+ raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
return ld;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3)
From: Ivo Sieben @ 2012-05-03 12:37 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT; +Cc: Ivo Sieben
In-Reply-To: <1336048663-21882-1-git-send-email-meltedpianoman@gmail.com>
Solved unnecessary schedule latency in the TTY layer when used in
realtime context:
The global wait_queue that is used for line discipline idle handling is
moved to a separate wait_queue for each line instance. This prevents
unnecessary blocking on one line, because of idle handling on another
line.
Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and
no interrupts (and therefor no scheduling) is disabled.
Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
---
drivers/tty/tty_ldisc.c | 7 ++++---
include/linux/tty_ldisc.h | 2 ++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 30836cc..77eb299 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -28,7 +28,6 @@
static DEFINE_RAW_SPINLOCK(tty_ldisc_lock);
static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
-static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle);
/* Line disc dispatch table */
static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS];
@@ -65,7 +64,7 @@ static void put_ldisc(struct tty_ldisc *ld)
return;
}
raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
- wake_up(&tty_ldisc_idle);
+ wake_up(&ld->wq_idle);
}
/**
@@ -200,6 +199,8 @@ static struct tty_ldisc *tty_ldisc_get(int disc)
ld->ops = ldops;
atomic_set(&ld->users, 1);
+ init_waitqueue_head(&ld->wq_idle);
+
return ld;
}
@@ -538,7 +539,7 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
{
long ret;
- ret = wait_event_timeout(tty_ldisc_idle,
+ ret = wait_event_timeout(tty->ldisc->wq_idle,
atomic_read(&tty->ldisc->users) == 1, timeout);
return ret > 0 ? 0 : -EBUSY;
}
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index ff7dc08..fb79dd8 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -110,6 +110,7 @@
#include <linux/fs.h>
#include <linux/wait.h>
#include <linux/pps_kernel.h>
+#include <linux/wait.h>
struct tty_ldisc_ops {
int magic;
@@ -154,6 +155,7 @@ struct tty_ldisc_ops {
struct tty_ldisc {
struct tty_ldisc_ops *ops;
atomic_t users;
+ wait_queue_head_t wq_idle;
};
#define TTY_LDISC_MAGIC 0x5403
--
1.7.0.4
^ permalink raw reply related
* Re: Deterministic behavior for TTY serial
From: Ivo Sieben @ 2012-05-03 15:28 UTC (permalink / raw)
To: Greg KH, linux-serial, Alan Cox, RT
In-Reply-To: <CAMSQXEG+O86t10AF5gN875RoEHkMcGFQE_-G6TdeYVc4NJoBLw@mail.gmail.com>
Hi,
>
> The PREEMPT_RT uses mutexes for "normal" spin locks that do not
> disable interrupts...
> I'll try to use raw spinlocks in this code section and for the tty flip buffer
> See if that can solve my problem.
>
> If you have other ideas... let me know!
>
> Regards,
> Ivo
I've changed some small things to the tty layer (see my other 3 RFC
patches I've send).
Performance increased with my loopback stress test:
- Old situation: average read call last for 50us, with peaks up to 230 us
- New situation: average read call still 50us, peak up to 60 us
- Write was stable in both situations: average of 90 us, peak up to 100 us
Only the very first read & write took extra time (128 us for read, 143
for write)
I'm still investigating that...
Feedback is very appreciated.
Regards,
Ivo Sieben
^ permalink raw reply
* Re: [PATCH 3/3] RFC: Solved unnecessary schedule latency in the TTY layer (3/3)
From: Greg KH @ 2012-05-03 16:24 UTC (permalink / raw)
To: Ivo Sieben; +Cc: linux-serial, Alan Cox, RT
In-Reply-To: <1336048663-21882-3-git-send-email-meltedpianoman@gmail.com>
On Thu, May 03, 2012 at 02:37:43PM +0200, Ivo Sieben wrote:
> Solved unnecessary schedule latency in the TTY layer when used in
> realtime context:
> The global wait_queue that is used for line discipline idle handling is
> moved to a separate wait_queue for each line instance. This prevents
> unnecessary blocking on one line, because of idle handling on another
> line.
This patch makes sense even in a non-RT case, as it localizes the lock
to the specific ldisc.
Alan, any objection to me applying this one to the tree now?
thanks,
greg k-h
> Note: In a PREEMPT_RT system "normal" spin locks behave like mutexes and
> no interrupts (and therefor no scheduling) is disabled.
>
> Signed-off-by: Ivo Sieben <meltedpianoman@gmail.com>
> ---
> drivers/tty/tty_ldisc.c | 7 ++++---
> include/linux/tty_ldisc.h | 2 ++
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 30836cc..77eb299 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -28,7 +28,6 @@
>
> static DEFINE_RAW_SPINLOCK(tty_ldisc_lock);
> static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
> -static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_idle);
> /* Line disc dispatch table */
> static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS];
>
> @@ -65,7 +64,7 @@ static void put_ldisc(struct tty_ldisc *ld)
> return;
> }
> raw_spin_unlock_irqrestore(&tty_ldisc_lock, flags);
> - wake_up(&tty_ldisc_idle);
> + wake_up(&ld->wq_idle);
> }
>
> /**
> @@ -200,6 +199,8 @@ static struct tty_ldisc *tty_ldisc_get(int disc)
>
> ld->ops = ldops;
> atomic_set(&ld->users, 1);
> + init_waitqueue_head(&ld->wq_idle);
> +
> return ld;
> }
>
> @@ -538,7 +539,7 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
> static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
> {
> long ret;
> - ret = wait_event_timeout(tty_ldisc_idle,
> + ret = wait_event_timeout(tty->ldisc->wq_idle,
> atomic_read(&tty->ldisc->users) == 1, timeout);
> return ret > 0 ? 0 : -EBUSY;
> }
> diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
> index ff7dc08..fb79dd8 100644
> --- a/include/linux/tty_ldisc.h
> +++ b/include/linux/tty_ldisc.h
> @@ -110,6 +110,7 @@
> #include <linux/fs.h>
> #include <linux/wait.h>
> #include <linux/pps_kernel.h>
> +#include <linux/wait.h>
>
> struct tty_ldisc_ops {
> int magic;
> @@ -154,6 +155,7 @@ struct tty_ldisc_ops {
> struct tty_ldisc {
> struct tty_ldisc_ops *ops;
> atomic_t users;
> + wait_queue_head_t wq_idle;
> };
>
> #define TTY_LDISC_MAGIC 0x5403
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3] RFC: Solved unnecessary schedule latency in the TTY layer (2/3)
From: Greg KH @ 2012-05-03 16:25 UTC (permalink / raw)
To: Ivo Sieben; +Cc: linux-serial, Alan Cox, RT
In-Reply-To: <1336048663-21882-2-git-send-email-meltedpianoman@gmail.com>
On Thu, May 03, 2012 at 02:37:42PM +0200, Ivo Sieben wrote:
> Solved unnecessary schedule latency in the TTY layer when used in
> realtime context:
> The global "normal" spin lock that guards the line discipline
> administration is replaced by a raw spin lock.
Why, what does this cause to have happen? What's the difference here
that causes any speedups or latency fixes?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] usb: cp210x: Added support for GPIO (CP2103/4/5)
From: Alan Cox @ 2012-05-03 20:34 UTC (permalink / raw)
To: Preston Fick
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
preston.fick-S6d6foEdJf7QT0dZR+AlfA,
linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org
In-Reply-To: <1335817637-2862-1-git-send-email-preston.fick-S6d6foEdJf7QT0dZR+AlfA@public.gmane.org>
Ok this is my suggestion based on GregKH comments and a couple of others
plus some other driver and ldisc stuff that is pending
- register the gpio lines with the gpio layer dynamically
- put them in the right place in the device tree (I'll let Greg advise on
the best way to do that bit), plus make them visible via the ioctls for
convenience and as they will be needed anyway in kernel
That provides the user space API
After that I'll add the hooks to the core tty layer code which allow an
ldisc to adjust the gpio lines.
For that we'll need
struct tty_gpio {
u32 base;
u16 num;
u16 reserved;
#define NR_TTY_GPIOMAP 8
u16 map[NR_TTY_GPIOMAP];
u32 reserved2[4];
};
and
tty->gpiomap
which will be NULL for most users.
Plus
struct tty_gpio d;
ioctl(tty, TIOCGPIO, &d)
and
ioctl(tty, TIOCSGPIO, &d)
where the only bits that can be updated will be the map.
So the normal use case from user space would be
struct tty_gpio d;
int fd = open("/dev/ttyUSB0", O_RDWR);
ioctl(tty, TIOCSGPIO, &d);
stuff using the gpio driver interfaces
close(fd);
And setting up for a kernel ldisc something like
/* Set a GPIO to LDISC signal mapping for ISO7816 */
ioctl(tty, TIOCGPIO, &d);
d.map[TTY_GPIO_ISO7816_RESET] = d.base;
d.map]TTY_GPIO_ISO7816_VCC] = d.base + 1;
ioctl(tty, TIOCSGPIO, &d);
/* Switch to the ldisc */
ld = N_ISO7816;
ioctl(tty, TCSETD, &ld);
and we can then abstract all the wiring details away to keep the ldisc
portable.
Thoughts ?
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/3] tty_lock: undo the old tty_lock use on the ctty
From: Alan Cox @ 2012-05-03 21:21 UTC (permalink / raw)
To: linux-kernel, linux-serial
From: Alan Cox <alan@linux.intel.com>
get_current_tty has its own consistent locking. That means a pile of the
tty lock cases are not needed. As get_current_tty also keeps a reference the
tty object lifetime means we can propogate the lock removal out.
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/tty_io.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index d939bd7..b425c79 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -855,10 +855,11 @@ void disassociate_ctty(int on_exit)
*/
void no_tty(void)
{
+ /* FIXME: Review locking here. The tty_lock never covered any race
+ between a new association and proc_clear_tty but possible we need
+ to protect against this anyway */
struct task_struct *tsk = current;
- tty_lock();
disassociate_ctty(0);
- tty_unlock();
proc_clear_tty(tsk);
}
@@ -1800,6 +1801,9 @@ int tty_release(struct inode *inode, struct file *filp)
*
* We cannot return driver and index like for the other nodes because
* devpts will not work then. It expects inodes to be from devpts FS.
+ *
+ * We need to move to returning a refcounted object from all the lookup
+ * paths including this one.
*/
static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
{
@@ -1816,6 +1820,7 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
/* noctty = 1; */
tty_kref_put(tty);
/* FIXME: we put a reference and return a TTY! */
+ /* This is only safe because the caller holds tty_mutex */
return tty;
}
^ permalink raw reply related
* [PATCH 2/3] pty: Lock the devpts bits privately
From: Alan Cox @ 2012-05-03 21:22 UTC (permalink / raw)
To: linux-kernel, linux-serial
In-Reply-To: <20120503212151.568.91854.stgit@bob.linux.org.uk>
From: Alan Cox <alan@linux.intel.com>
This is a private pty affair, we don't want to tangle it with the tty_lock
any more as we know all the other non tty locking is now handled by the vfs
so we too can move.
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/pty.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index eeae7fa..5505ffc 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -26,11 +26,13 @@
#include <linux/bitops.h>
#include <linux/devpts_fs.h>
#include <linux/slab.h>
+#include <linux/mutex.h>
#ifdef CONFIG_UNIX98_PTYS
static struct tty_driver *ptm_driver;
static struct tty_driver *pts_driver;
+static DEFINE_MUTEX(devpts_mutex);
#endif
static void pty_close(struct tty_struct *tty, struct file *filp)
@@ -54,8 +56,11 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
if (tty->driver->subtype == PTY_TYPE_MASTER) {
set_bit(TTY_OTHER_CLOSED, &tty->flags);
#ifdef CONFIG_UNIX98_PTYS
- if (tty->driver == ptm_driver)
+ if (tty->driver == ptm_driver) {
+ mutex_lock(&devpts_mutex);
devpts_pty_kill(tty->link);
+ mutex_unlock(&devpts_mutex);
+ }
#endif
tty_unlock();
tty_vhangup(tty->link);
@@ -475,13 +480,17 @@ static struct tty_struct *ptm_unix98_lookup(struct tty_driver *driver,
* @idx: tty index
*
* Look up a pty master device. Called under the tty_mutex for now.
- * This provides our locking.
+ * This provides our locking for the tty pointer.
*/
static struct tty_struct *pts_unix98_lookup(struct tty_driver *driver,
struct inode *pts_inode, int idx)
{
- struct tty_struct *tty = devpts_get_tty(pts_inode, idx);
+ struct tty_struct *tty;
+
+ mutex_lock(&devpts_mutex);
+ tty = devpts_get_tty(pts_inode, idx);
+ mutex_unlock(&devpts_mutex);
/* Master must be open before slave */
if (!tty)
return ERR_PTR(-EIO);
@@ -622,8 +631,10 @@ static int ptmx_open(struct inode *inode, struct file *filp)
}
mutex_lock(&tty_mutex);
- tty_lock();
+ mutex_lock(&devpts_mutex);
tty = tty_init_dev(ptm_driver, index);
+ mutex_unlock(&devpts_mutex);
+ tty_lock();
mutex_unlock(&tty_mutex);
if (IS_ERR(tty)) {
^ permalink raw reply related
* [PATCH 3/3] tty_lock: Localise the lock
From: Alan Cox @ 2012-05-03 21:24 UTC (permalink / raw)
To: linux-kernel, linux-serial
In-Reply-To: <20120503212151.568.91854.stgit@bob.linux.org.uk>
From: Alan Cox <alan@linux.intel.com>
In each remaining case the tty_lock is associated with a specific tty. This
means we can now lock on a per tty basis. We do need tty_lock_pair() for
the pty case. Uglier but still a step in the right direction.
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
drivers/tty/amiserial.c | 12 ++++----
drivers/tty/cyclades.c | 2 +
drivers/tty/n_r3964.c | 11 ++++---
drivers/tty/pty.c | 23 +++++++++------
drivers/tty/synclink.c | 4 +--
drivers/tty/synclink_gt.c | 4 +--
drivers/tty/synclinkmp.c | 4 +--
drivers/tty/tty_io.c | 67 +++++++++++++++++++++++++++------------------
drivers/tty/tty_ldisc.c | 30 ++++++++++----------
drivers/tty/tty_mutex.c | 60 ++++++++++++++++++++++++++++++----------
drivers/tty/tty_port.c | 6 ++--
include/linux/tty.h | 23 +++++++++------
12 files changed, 149 insertions(+), 97 deletions(-)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 6cc4358..b88a65c 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -1033,7 +1033,7 @@ static int get_serial_info(struct tty_struct *tty, struct serial_state *state,
if (!retinfo)
return -EFAULT;
memset(&tmp, 0, sizeof(tmp));
- tty_lock();
+ tty_lock(tty);
tmp.line = tty->index;
tmp.port = state->port;
tmp.flags = state->tport.flags;
@@ -1042,7 +1042,7 @@ static int get_serial_info(struct tty_struct *tty, struct serial_state *state,
tmp.close_delay = state->tport.close_delay;
tmp.closing_wait = state->tport.closing_wait;
tmp.custom_divisor = state->custom_divisor;
- tty_unlock();
+ tty_unlock(tty);
if (copy_to_user(retinfo,&tmp,sizeof(*retinfo)))
return -EFAULT;
return 0;
@@ -1059,12 +1059,12 @@ static int set_serial_info(struct tty_struct *tty, struct serial_state *state,
if (copy_from_user(&new_serial,new_info,sizeof(new_serial)))
return -EFAULT;
- tty_lock();
+ tty_lock(tty);
change_spd = ((new_serial.flags ^ port->flags) & ASYNC_SPD_MASK) ||
new_serial.custom_divisor != state->custom_divisor;
if (new_serial.irq || new_serial.port != state->port ||
new_serial.xmit_fifo_size != state->xmit_fifo_size) {
- tty_unlock();
+ tty_unlock(tty);
return -EINVAL;
}
@@ -1084,7 +1084,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_state *state,
}
if (new_serial.baud_base < 9600) {
- tty_unlock();
+ tty_unlock(tty);
return -EINVAL;
}
@@ -1116,7 +1116,7 @@ check_and_exit:
}
} else
retval = startup(tty, state);
- tty_unlock();
+ tty_unlock(tty);
return retval;
}
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index e61cabd..6984e1a 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -1599,7 +1599,7 @@ static int cy_open(struct tty_struct *tty, struct file *filp)
* If the port is the middle of closing, bail out now
*/
if (tty_hung_up_p(filp) || (info->port.flags & ASYNC_CLOSING)) {
- wait_event_interruptible_tty(info->port.close_wait,
+ wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
return (info->port.flags & ASYNC_HUP_NOTIFY) ? -EAGAIN: -ERESTARTSYS;
}
diff --git a/drivers/tty/n_r3964.c b/drivers/tty/n_r3964.c
index 5c6c314..656ad93 100644
--- a/drivers/tty/n_r3964.c
+++ b/drivers/tty/n_r3964.c
@@ -1065,7 +1065,8 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
TRACE_L("read()");
- tty_lock();
+ /* FIXME: should use a private lock */
+ tty_lock(tty);
pClient = findClient(pInfo, task_pid(current));
if (pClient) {
@@ -1077,7 +1078,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
goto unlock;
}
/* block until there is a message: */
- wait_event_interruptible_tty(pInfo->read_wait,
+ wait_event_interruptible_tty(tty, pInfo->read_wait,
(pMsg = remove_msg(pInfo, pClient)));
}
@@ -1107,7 +1108,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
}
ret = -EPERM;
unlock:
- tty_unlock();
+ tty_unlock(tty);
return ret;
}
@@ -1156,7 +1157,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file,
pHeader->locks = 0;
pHeader->owner = NULL;
- tty_lock();
+ tty_lock(tty);
pClient = findClient(pInfo, task_pid(current));
if (pClient) {
@@ -1175,7 +1176,7 @@ static ssize_t r3964_write(struct tty_struct *tty, struct file *file,
add_tx_queue(pInfo, pHeader);
trigger_transmit(pInfo);
- tty_unlock();
+ tty_unlock(tty);
return 0;
}
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 5505ffc..d6fa842 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -47,6 +47,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
wake_up_interruptible(&tty->read_wait);
wake_up_interruptible(&tty->write_wait);
tty->packet = 0;
+ /* Review - krefs on tty_link ?? */
if (!tty->link)
return;
tty->link->packet = 0;
@@ -62,9 +63,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
mutex_unlock(&devpts_mutex);
}
#endif
- tty_unlock();
tty_vhangup(tty->link);
- tty_lock();
}
}
@@ -622,26 +621,29 @@ static int ptmx_open(struct inode *inode, struct file *filp)
return retval;
/* find a device that is not in use. */
- tty_lock();
+ mutex_lock(&devpts_mutex);
index = devpts_new_index(inode);
- tty_unlock();
if (index < 0) {
retval = index;
goto err_file;
}
+ mutex_unlock(&devpts_mutex);
+
mutex_lock(&tty_mutex);
mutex_lock(&devpts_mutex);
tty = tty_init_dev(ptm_driver, index);
- mutex_unlock(&devpts_mutex);
- tty_lock();
- mutex_unlock(&tty_mutex);
if (IS_ERR(tty)) {
retval = PTR_ERR(tty);
goto out;
}
+ /* The tty returned here is locked so we can safely
+ drop the mutex */
+ mutex_unlock(&devpts_mutex);
+ mutex_unlock(&tty_mutex);
+
set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
tty_add_file(tty, filp);
@@ -654,16 +656,17 @@ static int ptmx_open(struct inode *inode, struct file *filp)
if (retval)
goto err_release;
- tty_unlock();
+ tty_unlock(tty);
return 0;
err_release:
- tty_unlock();
+ tty_unlock(tty);
tty_release(inode, filp);
return retval;
out:
+ mutex_unlock(&tty_mutex);
devpts_kill_index(inode, index);
- tty_unlock();
err_file:
+ mutex_unlock(&devpts_mutex);
tty_free_file(filp);
return retval;
}
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index d063b7d..bc1ae4a 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -3338,9 +3338,9 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp,
printk("%s(%d):block_til_ready blocking on %s count=%d\n",
__FILE__,__LINE__, tty->driver->name, port->count );
- tty_unlock();
+ tty_unlock(tty);
schedule();
- tty_lock();
+ tty_lock(tty);
}
set_current_state(TASK_RUNNING);
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index fcde827..df4f0dc 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -3337,9 +3337,9 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
}
DBGINFO(("%s block_til_ready wait\n", tty->driver->name));
- tty_unlock();
+ tty_unlock(tty);
schedule();
- tty_lock();
+ tty_lock(tty);
}
set_current_state(TASK_RUNNING);
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index a040a46..845502f 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -3358,9 +3358,9 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
printk("%s(%d):%s block_til_ready() count=%d\n",
__FILE__,__LINE__, tty->driver->name, port->count );
- tty_unlock();
+ tty_unlock(tty);
schedule();
- tty_lock();
+ tty_lock(tty);
}
set_current_state(TASK_RUNNING);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..9e930c0 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -185,6 +185,7 @@ void free_tty_struct(struct tty_struct *tty)
put_device(tty->dev);
kfree(tty->write_buf);
tty_buffer_free_all(tty);
+ tty->magic = 0xDEADDEAD;
kfree(tty);
}
@@ -573,7 +574,7 @@ void __tty_hangup(struct tty_struct *tty)
}
spin_unlock(&redirect_lock);
- tty_lock();
+ tty_lock(tty);
/* some functions below drop BTM, so we need this bit */
set_bit(TTY_HUPPING, &tty->flags);
@@ -666,7 +667,7 @@ void __tty_hangup(struct tty_struct *tty)
clear_bit(TTY_HUPPING, &tty->flags);
tty_ldisc_enable(tty);
- tty_unlock();
+ tty_unlock(tty);
if (f)
fput(f);
@@ -1103,12 +1104,12 @@ void tty_write_message(struct tty_struct *tty, char *msg)
{
if (tty) {
mutex_lock(&tty->atomic_write_lock);
- tty_lock();
+ tty_lock(tty);
if (tty->ops->write && !test_bit(TTY_CLOSING, &tty->flags)) {
- tty_unlock();
+ tty_unlock(tty);
tty->ops->write(tty, msg, strlen(msg));
} else
- tty_unlock();
+ tty_unlock(tty);
tty_write_unlock(tty);
}
return;
@@ -1403,6 +1404,7 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
}
initialize_tty_struct(tty, driver, idx);
+ tty_lock(tty);
retval = tty_driver_install_tty(driver, tty);
if (retval < 0)
goto err_deinit_tty;
@@ -1415,9 +1417,11 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
retval = tty_ldisc_setup(tty, tty->link);
if (retval)
goto err_release_tty;
+ /* Return the tty locked so that it cannot vanish under the caller */
return tty;
err_deinit_tty:
+ tty_unlock(tty);
deinitialize_tty_struct(tty);
free_tty_struct(tty);
err_module_put:
@@ -1426,6 +1430,7 @@ err_module_put:
/* call the tty release_tty routine to clean out this slot */
err_release_tty:
+ tty_unlock(tty);
printk_ratelimited(KERN_INFO "tty_init_dev: ldisc open failed, "
"clearing slot %d\n", idx);
release_tty(tty, idx);
@@ -1628,7 +1633,7 @@ int tty_release(struct inode *inode, struct file *filp)
if (tty_paranoia_check(tty, inode, __func__))
return 0;
- tty_lock();
+ tty_lock(tty);
check_tty_count(tty, __func__);
__tty_fasync(-1, filp, 0);
@@ -1637,10 +1642,11 @@ int tty_release(struct inode *inode, struct file *filp)
pty_master = (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
tty->driver->subtype == PTY_TYPE_MASTER);
devpts = (tty->driver->flags & TTY_DRIVER_DEVPTS_MEM) != 0;
+ /* Review: parallel close */
o_tty = tty->link;
if (tty_release_checks(tty, o_tty, idx)) {
- tty_unlock();
+ tty_unlock(tty);
return 0;
}
@@ -1652,7 +1658,7 @@ int tty_release(struct inode *inode, struct file *filp)
if (tty->ops->close)
tty->ops->close(tty, filp);
- tty_unlock();
+ tty_unlock(tty);
/*
* Sanity check: if tty->count is going to zero, there shouldn't be
* any waiters on tty->read_wait or tty->write_wait. We test the
@@ -1675,7 +1681,7 @@ int tty_release(struct inode *inode, struct file *filp)
opens on /dev/tty */
mutex_lock(&tty_mutex);
- tty_lock();
+ tty_lock_pair(tty, o_tty);
tty_closing = tty->count <= 1;
o_tty_closing = o_tty &&
(o_tty->count <= (pty_master ? 1 : 0));
@@ -1706,7 +1712,7 @@ int tty_release(struct inode *inode, struct file *filp)
printk(KERN_WARNING "%s: %s: read/write wait queue active!\n",
__func__, tty_name(tty, buf));
- tty_unlock();
+ tty_unlock_pair(tty, o_tty);
mutex_unlock(&tty_mutex);
schedule();
}
@@ -1769,7 +1775,7 @@ int tty_release(struct inode *inode, struct file *filp)
/* check whether both sides are closing ... */
if (!tty_closing || (o_tty && !o_tty_closing)) {
- tty_unlock();
+ tty_unlock_pair(tty, o_tty);
return 0;
}
@@ -1782,14 +1788,16 @@ int tty_release(struct inode *inode, struct file *filp)
tty_ldisc_release(tty, o_tty);
/*
* The release_tty function takes care of the details of clearing
- * the slots and preserving the termios structure.
+ * the slots and preserving the termios structure. The tty_unlock_pair
+ * should be safe as we keep a kref while the tty is locked (so the
+ * unlock never unlocks a freed tty).
*/
release_tty(tty, idx);
+ tty_unlock_pair(tty, o_tty);
/* Make this pty number available for reallocation */
if (devpts)
devpts_kill_index(inode, idx);
- tty_unlock();
return 0;
}
@@ -1893,6 +1901,9 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
* Locking: tty_mutex protects tty, tty_lookup_driver and tty_init_dev.
* tty->count should protect the rest.
* ->siglock protects ->signal/->sighand
+ *
+ * Note: the tty_unlock/lock cases without a ref are only safe due to
+ * tty_mutex
*/
static int tty_open(struct inode *inode, struct file *filp)
@@ -1916,8 +1927,7 @@ retry_open:
retval = 0;
mutex_lock(&tty_mutex);
- tty_lock();
-
+ /* This is protected by the tty_mutex */
tty = tty_open_current_tty(device, filp);
if (IS_ERR(tty)) {
retval = PTR_ERR(tty);
@@ -1938,17 +1948,19 @@ retry_open:
}
if (tty) {
+ tty_lock(tty);
retval = tty_reopen(tty);
- if (retval)
+ if (retval < 0) {
+ tty_unlock(tty);
tty = ERR_PTR(retval);
- } else
+ }
+ } else /* Returns with the tty_lock held for now */
tty = tty_init_dev(driver, index);
mutex_unlock(&tty_mutex);
if (driver)
tty_driver_kref_put(driver);
if (IS_ERR(tty)) {
- tty_unlock();
retval = PTR_ERR(tty);
goto err_file;
}
@@ -1977,7 +1989,7 @@ retry_open:
printk(KERN_DEBUG "%s: error %d in opening %s...\n", __func__,
retval, tty->name);
#endif
- tty_unlock(); /* need to call tty_release without BTM */
+ tty_unlock(tty); /* need to call tty_release without BTM */
tty_release(inode, filp);
if (retval != -ERESTARTSYS)
return retval;
@@ -1989,17 +2001,15 @@ retry_open:
/*
* Need to reset f_op in case a hangup happened.
*/
- tty_lock();
if (filp->f_op == &hung_up_tty_fops)
filp->f_op = &tty_fops;
- tty_unlock();
goto retry_open;
}
- tty_unlock();
+ tty_unlock(tty);
mutex_lock(&tty_mutex);
- tty_lock();
+ tty_lock(tty);
spin_lock_irq(¤t->sighand->siglock);
if (!noctty &&
current->signal->leader &&
@@ -2007,11 +2017,10 @@ retry_open:
tty->session == NULL)
__proc_set_tty(current, tty);
spin_unlock_irq(¤t->sighand->siglock);
- tty_unlock();
+ tty_unlock(tty);
mutex_unlock(&tty_mutex);
return 0;
err_unlock:
- tty_unlock();
mutex_unlock(&tty_mutex);
/* after locks to avoid deadlock */
if (!IS_ERR_OR_NULL(driver))
@@ -2094,10 +2103,13 @@ out:
static int tty_fasync(int fd, struct file *filp, int on)
{
+ struct tty_struct *tty = file_tty(filp);
int retval;
- tty_lock();
+
+ tty_lock(tty);
retval = __tty_fasync(fd, filp, on);
- tty_unlock();
+ tty_unlock(tty);
+
return retval;
}
@@ -2934,6 +2946,7 @@ void initialize_tty_struct(struct tty_struct *tty,
tty->pgrp = NULL;
tty->overrun_time = jiffies;
tty_buffer_init(tty);
+ mutex_init(&tty->legacy_mutex);
mutex_init(&tty->termios_mutex);
mutex_init(&tty->ldisc_mutex);
init_waitqueue_head(&tty->write_wait);
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..fa65cde 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -567,7 +567,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
if (IS_ERR(new_ldisc))
return PTR_ERR(new_ldisc);
- tty_lock();
+ tty_lock(tty);
/*
* We need to look at the tty locking here for pty/tty pairs
* when both sides try to change in parallel.
@@ -581,12 +581,12 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
*/
if (tty->ldisc->ops->num == ldisc) {
- tty_unlock();
+ tty_unlock(tty);
tty_ldisc_put(new_ldisc);
return 0;
}
- tty_unlock();
+ tty_unlock(tty);
/*
* Problem: What do we do if this blocks ?
* We could deadlock here
@@ -594,7 +594,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
tty_wait_until_sent(tty, 0);
- tty_lock();
+ tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
/*
@@ -604,10 +604,10 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
while (test_bit(TTY_LDISC_CHANGING, &tty->flags)) {
mutex_unlock(&tty->ldisc_mutex);
- tty_unlock();
+ tty_unlock(tty);
wait_event(tty_ldisc_wait,
test_bit(TTY_LDISC_CHANGING, &tty->flags) == 0);
- tty_lock();
+ tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
}
@@ -622,7 +622,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
o_ldisc = tty->ldisc;
- tty_unlock();
+ tty_unlock(tty);
/*
* Make sure we don't change while someone holds a
* reference to the line discipline. The TTY_LDISC bit
@@ -649,7 +649,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
retval = tty_ldisc_wait_idle(tty, 5 * HZ);
- tty_lock();
+ tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
/* handle wait idle failure locked */
@@ -664,7 +664,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
clear_bit(TTY_LDISC_CHANGING, &tty->flags);
mutex_unlock(&tty->ldisc_mutex);
tty_ldisc_put(new_ldisc);
- tty_unlock();
+ tty_unlock(tty);
return -EIO;
}
@@ -707,7 +707,7 @@ enable:
if (o_work)
schedule_work(&o_tty->buf.work);
mutex_unlock(&tty->ldisc_mutex);
- tty_unlock();
+ tty_unlock(tty);
return retval;
}
@@ -815,11 +815,11 @@ void tty_ldisc_hangup(struct tty_struct *tty)
* need to wait for another function taking the BTM
*/
clear_bit(TTY_LDISC, &tty->flags);
- tty_unlock();
+ tty_unlock(tty);
cancel_work_sync(&tty->buf.work);
mutex_unlock(&tty->ldisc_mutex);
retry:
- tty_lock();
+ tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
/* At this point we have a closed ldisc and we want to
@@ -830,7 +830,7 @@ retry:
if (atomic_read(&tty->ldisc->users) != 1) {
char cur_n[TASK_COMM_LEN], tty_n[64];
long timeout = 3 * HZ;
- tty_unlock();
+ tty_unlock(tty);
while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
timeout = MAX_SCHEDULE_TIMEOUT;
@@ -911,10 +911,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
* race with the set_ldisc code path.
*/
- tty_unlock();
+ tty_unlock(tty);
tty_ldisc_halt(tty);
tty_ldisc_flush_works(tty);
- tty_lock();
+ tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
/*
diff --git a/drivers/tty/tty_mutex.c b/drivers/tty/tty_mutex.c
index 9ff986c..69adc80 100644
--- a/drivers/tty/tty_mutex.c
+++ b/drivers/tty/tty_mutex.c
@@ -4,29 +4,59 @@
#include <linux/semaphore.h>
#include <linux/sched.h>
-/*
- * The 'big tty mutex'
- *
- * This mutex is taken and released by tty_lock() and tty_unlock(),
- * replacing the older big kernel lock.
- * It can no longer be taken recursively, and does not get
- * released implicitly while sleeping.
- *
- * Don't use in new code.
- */
-static DEFINE_MUTEX(big_tty_mutex);
+/* Legacy tty mutex glue */
/*
* Getting the big tty mutex.
*/
-void __lockfunc tty_lock(void)
+
+void __lockfunc tty_lock(struct tty_struct *tty)
{
- mutex_lock(&big_tty_mutex);
+ if (tty->magic != TTY_MAGIC) {
+ printk(KERN_ERR "L Bad %p\n", tty);
+ WARN_ON(1);
+ return;
+ }
+ tty_kref_get(tty);
+ mutex_lock(&tty->legacy_mutex);
}
EXPORT_SYMBOL(tty_lock);
-void __lockfunc tty_unlock(void)
+void __lockfunc tty_unlock(struct tty_struct *tty)
{
- mutex_unlock(&big_tty_mutex);
+ if (tty->magic != TTY_MAGIC) {
+ printk(KERN_ERR "U Bad %p\n", tty);
+ WARN_ON(1);
+ return;
+ }
+ mutex_unlock(&tty->legacy_mutex);
+ tty_kref_put(tty);
}
EXPORT_SYMBOL(tty_unlock);
+
+/*
+ * Getting the big tty mutex for a pair of ttys with lock ordering
+ * On a non pty/tty pair tty2 can be NULL which is just fine.
+ */
+void __lockfunc tty_lock_pair(struct tty_struct *tty,
+ struct tty_struct *tty2)
+{
+ if (tty < tty2) {
+ tty_lock(tty);
+ tty_lock(tty2);
+ } else {
+ if (tty2 && tty2 != tty)
+ tty_lock(tty2);
+ tty_lock(tty);
+ }
+}
+EXPORT_SYMBOL(tty_lock_pair);
+
+void __lockfunc tty_unlock_pair(struct tty_struct *tty,
+ struct tty_struct *tty2)
+{
+ tty_unlock(tty);
+ if (tty2 && tty2 != tty)
+ tty_unlock(tty2);
+}
+EXPORT_SYMBOL(tty_unlock_pair);
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index d24807a..64f0975 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -237,7 +237,7 @@ int tty_port_block_til_ready(struct tty_port *port,
/* block if port is in the process of being closed */
if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING) {
- wait_event_interruptible_tty(port->close_wait,
+ wait_event_interruptible_tty(tty, port->close_wait,
!(port->flags & ASYNC_CLOSING));
if (port->flags & ASYNC_HUP_NOTIFY)
return -EAGAIN;
@@ -303,9 +303,9 @@ int tty_port_block_til_ready(struct tty_port *port,
retval = -ERESTARTSYS;
break;
}
- tty_unlock();
+ tty_unlock(tty);
schedule();
- tty_lock();
+ tty_lock(tty);
}
finish_wait(&port->open_wait, &wait);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..4990ef2 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -268,6 +268,7 @@ struct tty_struct {
struct mutex ldisc_mutex;
struct tty_ldisc *ldisc;
+ struct mutex legacy_mutex;
struct mutex termios_mutex;
spinlock_t ctrl_lock;
/* Termios values are protected by the termios mutex */
@@ -605,8 +606,12 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
/* tty_mutex.c */
/* functions for preparation of BKL removal */
-extern void __lockfunc tty_lock(void) __acquires(tty_lock);
-extern void __lockfunc tty_unlock(void) __releases(tty_lock);
+extern void __lockfunc tty_lock(struct tty_struct *tty);
+extern void __lockfunc tty_unlock(struct tty_struct *tty);
+extern void __lockfunc tty_lock_pair(struct tty_struct *tty,
+ struct tty_struct *tty2);
+extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,
+ struct tty_struct *tty2);
/*
* this shall be called only from where BTM is held (like close)
@@ -621,9 +626,9 @@ extern void __lockfunc tty_unlock(void) __releases(tty_lock);
static inline void tty_wait_until_sent_from_close(struct tty_struct *tty,
long timeout)
{
- tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
+ tty_unlock(tty); /* tty->ops->close holds the BTM, drop it while waiting */
tty_wait_until_sent(tty, timeout);
- tty_lock();
+ tty_lock(tty);
}
/*
@@ -638,16 +643,16 @@ static inline void tty_wait_until_sent_from_close(struct tty_struct *tty,
*
* Do not use in new code.
*/
-#define wait_event_interruptible_tty(wq, condition) \
+#define wait_event_interruptible_tty(tty, wq, condition) \
({ \
int __ret = 0; \
if (!(condition)) { \
- __wait_event_interruptible_tty(wq, condition, __ret); \
+ __wait_event_interruptible_tty(tty, wq, condition, __ret); \
} \
__ret; \
})
-#define __wait_event_interruptible_tty(wq, condition, ret) \
+#define __wait_event_interruptible_tty(tty, wq, condition, ret) \
do { \
DEFINE_WAIT(__wait); \
\
@@ -656,9 +661,9 @@ do { \
if (condition) \
break; \
if (!signal_pending(current)) { \
- tty_unlock(); \
+ tty_unlock(tty); \
schedule(); \
- tty_lock(); \
+ tty_lock(tty); \
continue; \
} \
ret = -ERESTARTSYS; \
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox