* Re: [RFC PATCH 1/3] UART: Add UART subsystem as a bus.
From: Alan Cox @ 2012-12-03 11:46 UTC (permalink / raw)
To: Lv Zheng
Cc: Len Brown, Rafael J Wysocki, Greg Kroah-Hartman, linux-acpi,
linux-serial
In-Reply-To: <6d5fc2e0799c12554aa8acdb2d7782ba8643902b.1354505472.git.lv.zheng@intel.com>
> + * uart_tty_find - find the associated TTY device for the UART target
> + * device
> + * @drv: the low level UART driver
> + * @dev: the parent physical device for the TTY devices
> + * @line: the line number of the UART target device
> + *
Our top level abstraction is struct tty and struct tty_port. There is
nothing requiring a serial tty is using the uart helper layer, nor
should there be, so your code needs to support devices not using the
uart layer. Otherwise it looks quite reasonable.
our 3.7 tty layer has sysfs nodes on the tty which may also help
Alan
^ permalink raw reply
* Re: [PATCHv2 3/5] serial: 8250_dw: Map IO memory
From: Alan Cox @ 2012-12-03 11:47 UTC (permalink / raw)
To: Heikki Krogerus; +Cc: Greg Kroah-Hartman, Jamie Iles, linux-serial, LKML
In-Reply-To: <1354533479-27306-4-git-send-email-heikki.krogerus@linux.intel.com>
On Mon, 3 Dec 2012 13:17:57 +0200
Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote:
> This needs to be done in order to later access the
> Designware specific registers.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Series Acked-by: Alan Cox <alan@linux.intel.com>
^ permalink raw reply
* Re: [PATCHv2 3/5] serial: 8250_dw: Map IO memory
From: Jamie Iles @ 2012-12-03 15:40 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Greg Kroah-Hartman, Alan Cox, Jamie Iles, linux-serial, LKML
In-Reply-To: <1354533479-27306-4-git-send-email-heikki.krogerus@linux.intel.com>
Hi Heikki,
On Mon, Dec 03, 2012 at 01:17:57PM +0200, Heikki Krogerus wrote:
> This needs to be done in order to later access the
> Designware specific registers.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/tty/serial/8250/8250_dw.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index ff83ea5..300bbed 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -111,10 +111,13 @@ static int dw8250_probe(struct platform_device *pdev)
> uart.port.irq = irq->start;
> uart.port.handle_irq = dw8250_handle_irq;
> uart.port.type = PORT_8250;
> - uart.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP |
> - UPF_FIXED_PORT;
> + uart.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT;
> uart.port.dev = &pdev->dev;
>
> + uart.port.membase = ioremap(regs->start, regs->end - regs->start);
Doesn't this have an off-by-one error? Perhaps:
+ uart.port.membase = ioremap(regs->start, resource_size(regs));
instead?
Jamie
^ permalink raw reply
* Re: [PATCHv2 0/5] serial: 8250: 8250_dw changes and dynamic capabilities
From: Jamie Iles @ 2012-12-03 15:43 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Greg Kroah-Hartman, Alan Cox, Jamie Iles, linux-serial, LKML
In-Reply-To: <1354533479-27306-1-git-send-email-heikki.krogerus@linux.intel.com>
Hi Heikki,
On Mon, Dec 03, 2012 at 01:17:54PM +0200, Heikki Krogerus wrote:
>
> Changes since v1:
> - rebased on top of Greg's tty-next
>
> These are mainly small 8250_dw.c changes. The interesting patch is
> probable the first one that changes 8250.c so the drivers are able to
> deliver their UART's capabilities when they are registering ports.
Other than my comment on 3/5 it looks great.
Reviewed-by: Jamie Iles <jamie@jamieiles.com>
Jamie
^ permalink raw reply
* Re: [PATCH] synclink fix ldisc buffer argument
From: Paul Fulghum @ 2012-12-03 16:03 UTC (permalink / raw)
To: Chen Gang; +Cc: Alan Cox, Greg KH, Linux Kernel Mailing List, linux-serial
In-Reply-To: <50BC0C84.4060802@asianux.com>
On 12/2/2012 8:20 PM, Chen Gang wrote:
> pardon (I am just learning)
> does 65535 mean HDLC_MAX_FRAME_SIZE ?
> why do we need info->max_frame_size >= 4096 ?
> in drivers/tty/synclink_gt.c:
> 3550 if (info->max_frame_size < 4096)
> 3551 info->max_frame_size = 4096;
> 3552 else if (info->max_frame_size > 65535)
> 3553 info->max_frame_size = 65535;
> 3554
> ...
> 3603 info->max_frame_size = 4096;
The hardware can send and receive HDLC frames up to
64K in size. The driver defaults to 4K max frame size
to save buffer space for the common case
(line 3603 in alloc_dev()).
The module parameter max_frame_size can override the default
in add_device() (lines 3550-3554 are from add_device()
range checking the module parameter)
> if possible:
> can we move the relative comments (which are inside function) to the
> location just above ldisc_receive_buf ?
The added comment from my first patch described the reuse
of the data buffer as the flag buffer. Alan prefers to use
a zero initialized dummy buffer for the flag buffer argument.
Doing it that way, the comment is not needed.
--
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982 (US Sales)
(512)345-7791 x102 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -6h)
www.microgate.com
^ permalink raw reply
* [PATCH] synclink fix ldisc buffer argument
From: Paul Fulghum @ 2012-12-03 17:13 UTC (permalink / raw)
To: Greg KH; +Cc: Chen Gang, Linux Kernel Mailing List, linux-serial, Alan Cox
In-Reply-To: <50B90D0D.9040401@microgate.com>
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. flag buffer allocation now matches max size of data
buffer. Unused char_buf buffers are removed.
Signed-off-by: Paul Fulghum <paulkf@microgate.com>
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-12-03 10:51:40.000000000 -0600
@@ -210,7 +210,7 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error; /* startup error (DIAGS) */
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -2666,6 +2666,14 @@ static int rx_alloc_buffers(MGSLPC_INFO
if (info->rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->rx_buf);
+ info->rx_buf = NULL;
+ return -ENOMEM;
+ }
+
rx_reset_buffers(info);
return 0;
}
@@ -2674,6 +2682,8 @@ static void rx_free_buffers(MGSLPC_INFO
{
kfree(info->rx_buf);
info->rx_buf = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
}
static int claim_resources(MGSLPC_INFO *info)
--- a/drivers/tty/synclink.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink.c 2012-12-03 10:51:56.000000000 -0600
@@ -291,8 +291,7 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -3891,7 +3890,13 @@ static int mgsl_alloc_intermediate_rxbuf
info->intermediate_rxbuffer = kmalloc(info->max_frame_size, GFP_KERNEL | GFP_DMA);
if ( info->intermediate_rxbuffer == NULL )
return -ENOMEM;
-
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->intermediate_rxbuffer);
+ info->intermediate_rxbuffer = NULL;
+ return -ENOMEM;
+ }
return 0;
} /* end of mgsl_alloc_intermediate_rxbuffer_memory() */
@@ -3910,6 +3915,8 @@ static void mgsl_free_intermediate_rxbuf
{
kfree(info->intermediate_rxbuffer);
info->intermediate_rxbuffer = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
} /* end of mgsl_free_intermediate_rxbuffer_memory() */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclinkmp.c 2012-12-03 10:52:09.000000000 -0600
@@ -262,8 +262,7 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -3544,6 +3543,13 @@ static int alloc_tmp_rx_buf(SLMP_INFO *i
info->tmp_rx_buf = kmalloc(info->max_frame_size, GFP_KERNEL);
if (info->tmp_rx_buf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->tmp_rx_buf);
+ info->tmp_rx_buf = NULL;
+ return -ENOMEM;
+ }
return 0;
}
@@ -3551,6 +3557,8 @@ static void free_tmp_rx_buf(SLMP_INFO *i
{
kfree(info->tmp_rx_buf);
info->tmp_rx_buf = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
}
static int claim_resources(SLMP_INFO *info)
--- a/drivers/tty/synclink_gt.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink_gt.c 2012-12-03 11:00:15.000000000 -0600
@@ -317,8 +317,7 @@ struct slgt_info {
unsigned char *tx_buf;
int tx_count;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
+ char *flag_buf;
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -3355,11 +3354,24 @@ static int block_til_ready(struct tty_st
return retval;
}
+/*
+ * allocate buffers used for calling line discipline receive_buf
+ * directly in synchronous mode
+ * note: add 5 bytes to max frame size to allow appending
+ * 32-bit CRC and status byte when configured to do so
+ */
static int alloc_tmp_rbuf(struct slgt_info *info)
{
info->tmp_rbuf = kmalloc(info->max_frame_size + 5, GFP_KERNEL);
if (info->tmp_rbuf == NULL)
return -ENOMEM;
+ /* unused flag buffer to satisfy receive_buf calling interface */
+ info->flag_buf = kzalloc(info->max_frame_size + 5, GFP_KERNEL);
+ if (!info->flag_buf) {
+ kfree(info->tmp_rbuf);
+ info->tmp_rbuf = NULL;
+ return -ENOMEM;
+ }
return 0;
}
@@ -3367,6 +3379,8 @@ static void free_tmp_rbuf(struct slgt_in
{
kfree(info->tmp_rbuf);
info->tmp_rbuf = NULL;
+ kfree(info->flag_buf);
+ info->flag_buf = NULL;
}
/*
^ permalink raw reply
* [PATCH 1/2] serial: sccnxp: Implement polling mode
From: Alexander Shiyan @ 2012-12-03 18:23 UTC (permalink / raw)
To: linux-serial; +Cc: Alan Cox, Greg Kroah-Hartman, Alexander Shiyan
This patch adds support for polling work mode, i.e. system is perform
periodical check chip status when IRQ-line is not connected to CPU.
Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
drivers/tty/serial/sccnxp.c | 148 +++++++++++++++++++++++----------
include/linux/platform_data/sccnxp.h | 2 +
2 files changed, 105 insertions(+), 45 deletions(-)
diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index 418b495..ae68e8c 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -23,6 +23,7 @@
#include <linux/io.h>
#include <linux/tty.h>
#include <linux/tty_flip.h>
+#include <linux/spinlock.h>
#include <linux/platform_device.h>
#include <linux/platform_data/sccnxp.h>
@@ -106,6 +107,7 @@ enum {
struct sccnxp_port {
struct uart_driver uart;
struct uart_port port[SCCNXP_MAX_UARTS];
+ bool opened[SCCNXP_MAX_UARTS];
const char *name;
int irq;
@@ -122,7 +124,10 @@ struct sccnxp_port {
struct console console;
#endif
- struct mutex sccnxp_mutex;
+ spinlock_t lock;
+
+ bool poll;
+ struct timer_list timer;
struct sccnxp_pdata pdata;
};
@@ -377,31 +382,48 @@ static void sccnxp_handle_tx(struct uart_port *port)
uart_write_wakeup(port);
}
-static irqreturn_t sccnxp_ist(int irq, void *dev_id)
+static void sccnxp_handle_events(struct sccnxp_port *s)
{
int i;
u8 isr;
- struct sccnxp_port *s = (struct sccnxp_port *)dev_id;
-
- mutex_lock(&s->sccnxp_mutex);
- for (;;) {
+ do {
isr = sccnxp_read(&s->port[0], SCCNXP_ISR_REG);
isr &= s->imr;
if (!isr)
break;
- dev_dbg(s->port[0].dev, "IRQ status: 0x%02x\n", isr);
-
for (i = 0; i < s->uart.nr; i++) {
- if (isr & ISR_RXRDY(i))
+ if (s->opened[i] && (isr & ISR_RXRDY(i)))
sccnxp_handle_rx(&s->port[i]);
- if (isr & ISR_TXRDY(i))
+ if (s->opened[i] && (isr & ISR_TXRDY(i)))
sccnxp_handle_tx(&s->port[i]);
}
- }
+ } while (1);
+}
+
+static void sccnxp_timer(unsigned long data)
+{
+ struct sccnxp_port *s = (struct sccnxp_port *)data;
+ unsigned long flags;
- mutex_unlock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
+ sccnxp_handle_events(s);
+ spin_unlock_irqrestore(&s->lock, flags);
+
+ if (!timer_pending(&s->timer))
+ mod_timer(&s->timer, jiffies +
+ usecs_to_jiffies(s->pdata.poll_time_us));
+}
+
+static irqreturn_t sccnxp_ist(int irq, void *dev_id)
+{
+ struct sccnxp_port *s = (struct sccnxp_port *)dev_id;
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
+ sccnxp_handle_events(s);
+ spin_unlock_irqrestore(&s->lock, flags);
return IRQ_HANDLED;
}
@@ -409,8 +431,9 @@ static irqreturn_t sccnxp_ist(int irq, void *dev_id)
static void sccnxp_start_tx(struct uart_port *port)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
/* Set direction to output */
if (s->flags & SCCNXP_HAVE_IO)
@@ -418,7 +441,7 @@ static void sccnxp_start_tx(struct uart_port *port)
sccnxp_enable_irq(port, IMR_TXRDY);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static void sccnxp_stop_tx(struct uart_port *port)
@@ -429,20 +452,22 @@ static void sccnxp_stop_tx(struct uart_port *port)
static void sccnxp_stop_rx(struct uart_port *port)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
sccnxp_port_write(port, SCCNXP_CR_REG, CR_RX_DISABLE);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static unsigned int sccnxp_tx_empty(struct uart_port *port)
{
u8 val;
+ unsigned long flags;
struct sccnxp_port *s = dev_get_drvdata(port->dev);
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
val = sccnxp_port_read(port, SCCNXP_SR_REG);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
return (val & SR_TXEMT) ? TIOCSER_TEMT : 0;
}
@@ -455,28 +480,30 @@ static void sccnxp_enable_ms(struct uart_port *port)
static void sccnxp_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
if (!(s->flags & SCCNXP_HAVE_IO))
return;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
sccnxp_set_bit(port, DTR_OP, mctrl & TIOCM_DTR);
sccnxp_set_bit(port, RTS_OP, mctrl & TIOCM_RTS);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static unsigned int sccnxp_get_mctrl(struct uart_port *port)
{
u8 bitmask, ipr;
+ unsigned long flags;
struct sccnxp_port *s = dev_get_drvdata(port->dev);
unsigned int mctrl = TIOCM_DSR | TIOCM_CTS | TIOCM_CAR;
if (!(s->flags & SCCNXP_HAVE_IO))
return mctrl;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
ipr = ~sccnxp_read(port, SCCNXP_IPCR_REG);
@@ -505,7 +532,7 @@ static unsigned int sccnxp_get_mctrl(struct uart_port *port)
mctrl |= (ipr & bitmask) ? TIOCM_RNG : 0;
}
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
return mctrl;
}
@@ -513,21 +540,23 @@ static unsigned int sccnxp_get_mctrl(struct uart_port *port)
static void sccnxp_break_ctl(struct uart_port *port, int break_state)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
sccnxp_port_write(port, SCCNXP_CR_REG, break_state ?
CR_CMD_START_BREAK : CR_CMD_STOP_BREAK);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static void sccnxp_set_termios(struct uart_port *port,
struct ktermios *termios, struct ktermios *old)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
u8 mr1, mr2;
int baud;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
/* Mask termios capabilities we don't support */
termios->c_cflag &= ~CMSPAR;
@@ -594,20 +623,22 @@ static void sccnxp_set_termios(struct uart_port *port,
/* Update timeout according to new baud rate */
uart_update_timeout(port, termios->c_cflag, baud);
+ /* Report actual baudrate back to core */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);
/* Enable RX & TX */
sccnxp_port_write(port, SCCNXP_CR_REG, CR_RX_ENABLE | CR_TX_ENABLE);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static int sccnxp_startup(struct uart_port *port)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
if (s->flags & SCCNXP_HAVE_IO) {
/* Outputs are controlled manually */
@@ -626,7 +657,9 @@ static int sccnxp_startup(struct uart_port *port)
/* Enable RX interrupt */
sccnxp_enable_irq(port, IMR_RXRDY);
- mutex_unlock(&s->sccnxp_mutex);
+ s->opened[port->line] = 1;
+
+ spin_unlock_irqrestore(&s->lock, flags);
return 0;
}
@@ -634,8 +667,11 @@ static int sccnxp_startup(struct uart_port *port)
static void sccnxp_shutdown(struct uart_port *port)
{
struct sccnxp_port *s = dev_get_drvdata(port->dev);
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
- mutex_lock(&s->sccnxp_mutex);
+ s->opened[port->line] = 0;
/* Disable interrupts */
sccnxp_disable_irq(port, IMR_TXRDY | IMR_RXRDY);
@@ -647,7 +683,7 @@ static void sccnxp_shutdown(struct uart_port *port)
if (s->flags & SCCNXP_HAVE_IO)
sccnxp_set_bit(port, DIR_OP, 0);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static const char *sccnxp_type(struct uart_port *port)
@@ -721,10 +757,11 @@ static void sccnxp_console_write(struct console *co, const char *c, unsigned n)
{
struct sccnxp_port *s = (struct sccnxp_port *)co->data;
struct uart_port *port = &s->port[co->index];
+ unsigned long flags;
- mutex_lock(&s->sccnxp_mutex);
+ spin_lock_irqsave(&s->lock, flags);
uart_console_write(port, c, n, sccnxp_console_putchar);
- mutex_unlock(&s->sccnxp_mutex);
+ spin_unlock_irqrestore(&s->lock, flags);
}
static int sccnxp_console_setup(struct console *co, char *options)
@@ -763,7 +800,7 @@ static int sccnxp_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, s);
- mutex_init(&s->sccnxp_mutex);
+ spin_lock_init(&s->lock);
/* Individual chip settings */
switch (chiptype) {
@@ -860,11 +897,19 @@ static int sccnxp_probe(struct platform_device *pdev)
} else
memcpy(&s->pdata, pdata, sizeof(struct sccnxp_pdata));
- s->irq = platform_get_irq(pdev, 0);
- if (s->irq <= 0) {
- dev_err(&pdev->dev, "Missing irq resource data\n");
- ret = -ENXIO;
- goto err_out;
+ if (pdata->poll_time_us) {
+ dev_info(&pdev->dev, "Using poll mode, resolution %u usecs\n",
+ pdata->poll_time_us);
+ s->poll = 1;
+ }
+
+ if (!s->poll) {
+ s->irq = platform_get_irq(pdev, 0);
+ if (s->irq < 0) {
+ dev_err(&pdev->dev, "Missing irq resource data\n");
+ ret = -ENXIO;
+ goto err_out;
+ }
}
/* Check input frequency */
@@ -929,13 +974,23 @@ static int sccnxp_probe(struct platform_device *pdev)
if (s->pdata.init)
s->pdata.init();
- ret = devm_request_threaded_irq(&pdev->dev, s->irq, NULL, sccnxp_ist,
- IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
- dev_name(&pdev->dev), s);
- if (!ret)
+ if (!s->poll) {
+ ret = devm_request_threaded_irq(&pdev->dev, s->irq, NULL,
+ sccnxp_ist,
+ IRQF_TRIGGER_FALLING |
+ IRQF_ONESHOT,
+ dev_name(&pdev->dev), s);
+ if (!ret)
+ return 0;
+
+ dev_err(&pdev->dev, "Unable to reguest IRQ %i\n", s->irq);
+ } else {
+ init_timer(&s->timer);
+ setup_timer(&s->timer, sccnxp_timer, (unsigned long)s);
+ mod_timer(&s->timer, jiffies +
+ usecs_to_jiffies(s->pdata.poll_time_us));
return 0;
-
- dev_err(&pdev->dev, "Unable to reguest IRQ %i\n", s->irq);
+ }
err_out:
platform_set_drvdata(pdev, NULL);
@@ -948,7 +1003,10 @@ static int sccnxp_remove(struct platform_device *pdev)
int i;
struct sccnxp_port *s = platform_get_drvdata(pdev);
- devm_free_irq(&pdev->dev, s->irq, s);
+ if (!s->poll)
+ devm_free_irq(&pdev->dev, s->irq, s);
+ else
+ del_timer_sync(&s->timer);
for (i = 0; i < s->uart.nr; i++)
uart_remove_one_port(&s->uart, &s->port[i]);
diff --git a/include/linux/platform_data/sccnxp.h b/include/linux/platform_data/sccnxp.h
index 7311ccd..096de90 100644
--- a/include/linux/platform_data/sccnxp.h
+++ b/include/linux/platform_data/sccnxp.h
@@ -84,6 +84,8 @@ struct sccnxp_pdata {
const u8 reg_shift;
/* Modem control lines configuration */
const u32 mctrl_cfg[SCCNXP_MAX_UARTS];
+ /* Timer value for polling mode (usecs) */
+ const unsigned int poll_time_us;
/* Called during startup */
void (*init)(void);
/* Called before finish */
--
1.7.8.6
^ permalink raw reply related
* [PATCH 2/2] serial: sccnxp: Rename header file to match functionality
From: Alexander Shiyan @ 2012-12-03 18:23 UTC (permalink / raw)
To: linux-serial; +Cc: Alan Cox, Greg Kroah-Hartman, Alexander Shiyan
In-Reply-To: <1354559012-21882-1-git-send-email-shc_work@mail.ru>
Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
arch/mips/sni/a20r.c | 2 +-
drivers/tty/serial/sccnxp.c | 2 +-
.../platform_data/{sccnxp.h => serial-sccnxp.h} | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
rename include/linux/platform_data/{sccnxp.h => serial-sccnxp.h} (96%)
diff --git a/arch/mips/sni/a20r.c b/arch/mips/sni/a20r.c
index 9cb9d43..e05ad4d 100644
--- a/arch/mips/sni/a20r.c
+++ b/arch/mips/sni/a20r.c
@@ -118,7 +118,7 @@ static struct resource sc26xx_rsrc[] = {
}
};
-#include <linux/platform_data/sccnxp.h>
+#include <linux/platform_data/serial-sccnxp.h>
static struct sccnxp_pdata sccnxp_data = {
.reg_shift = 2,
diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index ae68e8c..0c86a0b 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -25,7 +25,7 @@
#include <linux/tty_flip.h>
#include <linux/spinlock.h>
#include <linux/platform_device.h>
-#include <linux/platform_data/sccnxp.h>
+#include <linux/platform_data/serial-sccnxp.h>
#define SCCNXP_NAME "uart-sccnxp"
#define SCCNXP_MAJOR 204
diff --git a/include/linux/platform_data/sccnxp.h b/include/linux/platform_data/serial-sccnxp.h
similarity index 96%
rename from include/linux/platform_data/sccnxp.h
rename to include/linux/platform_data/serial-sccnxp.h
index 096de90..215574d 100644
--- a/include/linux/platform_data/sccnxp.h
+++ b/include/linux/platform_data/serial-sccnxp.h
@@ -11,8 +11,8 @@
* (at your option) any later version.
*/
-#ifndef __SCCNXP_H
-#define __SCCNXP_H
+#ifndef _PLATFORM_DATA_SERIAL_SCCNXP_H_
+#define _PLATFORM_DATA_SERIAL_SCCNXP_H_
#define SCCNXP_MAX_UARTS 2
--
1.7.8.6
^ permalink raw reply related
* [PATCH -next 2/9] tty: Add diagnostic for halted line discipline
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Flip buffer work must not be scheduled after the line discipline
has been halted; issue warning.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/n_tty.c | 6 ++++++
drivers/tty/tty_buffer.c | 3 +++
drivers/tty/tty_ldisc.c | 45 +++++++++++++++++++++++++--------------------
include/linux/tty.h | 1 +
4 files changed, 35 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 19083ef..3f704a9 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -152,6 +152,12 @@ static void n_tty_set_room(struct tty_struct *tty)
if (left && !old_left) {
WARN_RATELIMIT(tty->port->itty == NULL,
"scheduling with invalid itty\n");
+ /* see if ldisc has been killed - if so, this means that
+ * even though the ldisc has been halted and ->buf.work
+ * cancelled, ->buf.work is about to be rescheduled
+ */
+ WARN_RATELIMIT(test_bit(TTY_LDISC_HALTED, &tty->flags),
+ "scheduling buffer work for halted ldisc\n");
schedule_work(&tty->port->buf.work);
}
}
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index cc9cbcf..d8d6f77 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -520,6 +520,9 @@ void tty_flip_buffer_push(struct tty_struct *tty)
struct tty_bufhead *buf = &tty->port->buf;
unsigned long flags;
+ WARN_RATELIMIT(test_bit(TTY_LDISC_HALTED, &tty->flags),
+ "scheduling buffer work for halted ldisc\n");
+
spin_lock_irqsave(&buf->lock, flags);
if (buf->tail != NULL)
buf->tail->commit = buf->tail->used;
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index c578229..f986bb0 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -373,6 +373,7 @@ static inline void tty_ldisc_put(struct tty_ldisc *ld)
void tty_ldisc_enable(struct tty_struct *tty)
{
+ clear_bit(TTY_LDISC_HALTED, &tty->flags);
set_bit(TTY_LDISC, &tty->flags);
clear_bit(TTY_LDISC_CHANGING, &tty->flags);
wake_up(&tty_ldisc_wait);
@@ -496,26 +497,6 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
}
/**
- * tty_ldisc_halt - shut down the line discipline
- * @tty: tty device
- *
- * Shut down the line discipline and work queue for this tty device.
- * The TTY_LDISC flag being cleared ensures no further references can
- * be obtained while the delayed work queue halt ensures that no more
- * data is fed to the ldisc.
- *
- * You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
- * in order to make sure any currently executing ldisc work is also
- * flushed.
- */
-
-static int tty_ldisc_halt(struct tty_struct *tty)
-{
- clear_bit(TTY_LDISC, &tty->flags);
- return cancel_work_sync(&tty->port->buf.work);
-}
-
-/**
* tty_ldisc_flush_works - flush all works of a tty
* @tty: tty device to flush works for
*
@@ -545,6 +526,29 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
}
/**
+ * tty_ldisc_halt - shut down the line discipline
+ * @tty: tty device
+ *
+ * Shut down the line discipline and work queue for this tty device.
+ * The TTY_LDISC flag being cleared ensures no further references can
+ * be obtained while the delayed work queue halt ensures that no more
+ * data is fed to the ldisc.
+ *
+ * You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
+ * in order to make sure any currently executing ldisc work is also
+ * flushed.
+ */
+
+static int tty_ldisc_halt(struct tty_struct *tty)
+{
+ int scheduled;
+ clear_bit(TTY_LDISC, &tty->flags);
+ scheduled = cancel_work_sync(&tty->port->buf.work);
+ set_bit(TTY_LDISC_HALTED, &tty->flags);
+ return scheduled;
+}
+
+/**
* tty_set_ldisc - set line discipline
* @tty: the terminal to set
* @ldisc: the line discipline
@@ -818,6 +822,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
clear_bit(TTY_LDISC, &tty->flags);
tty_unlock(tty);
cancel_work_sync(&tty->port->buf.work);
+ set_bit(TTY_LDISC_HALTED, &tty->flags);
mutex_unlock(&tty->ldisc_mutex);
retry:
tty_lock(tty);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 8db1b56..e6b968d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -314,6 +314,7 @@ struct tty_file_private {
#define TTY_NO_WRITE_SPLIT 17 /* Preserve write boundaries to driver */
#define TTY_HUPPED 18 /* Post driver->hangup() */
#define TTY_HUPPING 21 /* ->hangup() in progress */
+#define TTY_LDISC_HALTED 22 /* Line discipline is halted */
#define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
--
1.8.0
^ permalink raw reply related
* [PATCH -next 4/9] tty: Refactor wait for ldisc refs out of tty_ldisc_hangup()
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Refactor tty_ldisc_hangup() to extract standalone function,
tty_ldisc_hangup_wait_idle(), to wait for ldisc references
to be released.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 54 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f986bb0..6c1d8aa 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -549,6 +549,41 @@ static int tty_ldisc_halt(struct tty_struct *tty)
}
/**
+ * tty_ldisc_hangup_wait_idle - wait for the ldisc to become idle
+ * @tty: tty to wait for
+ *
+ * Wait for the line discipline to become idle. The discipline must
+ * have been halted for this to guarantee it remains idle.
+ *
+ * Caller must hold legacy and ->ldisc_mutex.
+ */
+static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
+{
+ while (tty->ldisc) { /* Not yet closed */
+ if (atomic_read(&tty->ldisc->users) != 1) {
+ char cur_n[TASK_COMM_LEN], tty_n[64];
+ long timeout = 3 * HZ;
+ tty_unlock(tty);
+
+ while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ printk_ratelimited(KERN_WARNING
+ "%s: waiting (%s) for %s took too long, but we keep waiting...\n",
+ __func__, get_task_comm(cur_n, current),
+ tty_name(tty, tty_n));
+ }
+ /* must reacquire both locks and preserve lock order */
+ mutex_unlock(&tty->ldisc_mutex);
+ tty_lock(tty);
+ mutex_lock(&tty->ldisc_mutex);
+ continue;
+ }
+ break;
+ }
+ return !!(tty->ldisc);
+}
+
+/**
* tty_set_ldisc - set line discipline
* @tty: the terminal to set
* @ldisc: the line discipline
@@ -824,7 +859,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
cancel_work_sync(&tty->port->buf.work);
set_bit(TTY_LDISC_HALTED, &tty->flags);
mutex_unlock(&tty->ldisc_mutex);
-retry:
tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
@@ -832,23 +866,7 @@ retry:
reopen it. We could defer this to the next open but
it means auditing a lot of other paths so this is
a FIXME */
- if (tty->ldisc) { /* Not yet closed */
- if (atomic_read(&tty->ldisc->users) != 1) {
- char cur_n[TASK_COMM_LEN], tty_n[64];
- long timeout = 3 * HZ;
- tty_unlock(tty);
-
- while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
- timeout = MAX_SCHEDULE_TIMEOUT;
- printk_ratelimited(KERN_WARNING
- "%s: waiting (%s) for %s took too long, but we keep waiting...\n",
- __func__, get_task_comm(cur_n, current),
- tty_name(tty, tty_n));
- }
- mutex_unlock(&tty->ldisc_mutex);
- goto retry;
- }
-
+ if (tty_ldisc_hangup_wait_idle(tty)) {
if (reset == 0) {
if (!tty_ldisc_reinit(tty, tty->termios.c_line))
--
1.8.0
^ permalink raw reply related
* [PATCH -next 5/9] tty: Remove unnecessary re-test of ldisc ref count
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Since the tty->ldisc is prevented from being changed by tty_set_ldisc()
when a tty is being hung up, re-testing the ldisc user count is
unnecessary -- ie, it cannot be a different ldisc and the user count
cannot have increased (assuming the caller meets the precondition that
TTY_LDISC flag is cleared)
Removal of the 'early-out' locking optimization is necessary for
the subsequent patch 'tty: Fix ldisc halt sequence on hangup'.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 6c1d8aa..0d18d1e 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -556,29 +556,29 @@ static int tty_ldisc_halt(struct tty_struct *tty)
* have been halted for this to guarantee it remains idle.
*
* Caller must hold legacy and ->ldisc_mutex.
+ *
+ * NB: tty_set_ldisc() is prevented from changing the ldisc concurrently
+ * with this function when it checks the TTY_HUPPING state.
*/
static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
{
- while (tty->ldisc) { /* Not yet closed */
- if (atomic_read(&tty->ldisc->users) != 1) {
- char cur_n[TASK_COMM_LEN], tty_n[64];
- long timeout = 3 * HZ;
- tty_unlock(tty);
-
- while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
- timeout = MAX_SCHEDULE_TIMEOUT;
- printk_ratelimited(KERN_WARNING
- "%s: waiting (%s) for %s took too long, but we keep waiting...\n",
- __func__, get_task_comm(cur_n, current),
- tty_name(tty, tty_n));
- }
- /* must reacquire both locks and preserve lock order */
- mutex_unlock(&tty->ldisc_mutex);
- tty_lock(tty);
- mutex_lock(&tty->ldisc_mutex);
- continue;
+ char cur_n[TASK_COMM_LEN], tty_n[64];
+ long timeout = 3 * HZ;
+
+ if (tty->ldisc) { /* Not yet closed */
+ tty_unlock(tty);
+
+ while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ printk_ratelimited(KERN_WARNING
+ "%s: waiting (%s) for %s took too long, but we keep waiting...\n",
+ __func__, get_task_comm(cur_n, current),
+ tty_name(tty, tty_n));
}
- break;
+ /* must reacquire both locks and preserve lock order */
+ mutex_unlock(&tty->ldisc_mutex);
+ tty_lock(tty);
+ mutex_lock(&tty->ldisc_mutex);
}
return !!(tty->ldisc);
}
--
1.8.0
^ permalink raw reply related
* [PATCH -next 6/9] tty: Fix ldisc halt sequence on hangup
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Flip buffer work cannot be cancelled until all outstanding ldisc
references have been released. Convert the ldisc ref wait into
a full ldisc halt with buffer work cancellation.
Note that the legacy mutex is not held while cancelling.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 0d18d1e..c3dec37 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -549,22 +549,31 @@ static int tty_ldisc_halt(struct tty_struct *tty)
}
/**
- * tty_ldisc_hangup_wait_idle - wait for the ldisc to become idle
- * @tty: tty to wait for
- *
- * Wait for the line discipline to become idle. The discipline must
- * have been halted for this to guarantee it remains idle.
+ * tty_ldisc_hangup_halt - halt the line discipline for hangup
+ * @tty: tty being hung up
*
+ * Shut down the line discipline and work queue for the tty device
+ * being hungup. Clear the TTY_LDISC flag to ensure no further
+ * references can be obtained, wait for remaining references to be
+ * released, and cancel pending buffer work to ensure no more
+ * data is fed to the ldisc.
* Caller must hold legacy and ->ldisc_mutex.
*
* NB: tty_set_ldisc() is prevented from changing the ldisc concurrently
* with this function when it checks the TTY_HUPPING state.
+ *
+ * NB: if tty->ldisc is NULL then buffer work does not need to be
+ * cancelled because it must already have done as a precondition
+ * of setting tty->ldisc to NULL
+ *
*/
-static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
+static bool tty_ldisc_hangup_halt(struct tty_struct *tty)
{
char cur_n[TASK_COMM_LEN], tty_n[64];
long timeout = 3 * HZ;
+ clear_bit(TTY_LDISC, &tty->flags);
+
if (tty->ldisc) { /* Not yet closed */
tty_unlock(tty);
@@ -575,6 +584,10 @@ static bool tty_ldisc_hangup_wait_idle(struct tty_struct *tty)
__func__, get_task_comm(cur_n, current),
tty_name(tty, tty_n));
}
+
+ cancel_work_sync(&tty->port->buf.work);
+ set_bit(TTY_LDISC_HALTED, &tty->flags);
+
/* must reacquire both locks and preserve lock order */
mutex_unlock(&tty->ldisc_mutex);
tty_lock(tty);
@@ -849,24 +862,11 @@ void tty_ldisc_hangup(struct tty_struct *tty)
*/
mutex_lock(&tty->ldisc_mutex);
- /*
- * this is like tty_ldisc_halt, but we need to give up
- * the BTM before calling cancel_work_sync, which may
- * need to wait for another function taking the BTM
- */
- clear_bit(TTY_LDISC, &tty->flags);
- tty_unlock(tty);
- cancel_work_sync(&tty->port->buf.work);
- set_bit(TTY_LDISC_HALTED, &tty->flags);
- mutex_unlock(&tty->ldisc_mutex);
- tty_lock(tty);
- mutex_lock(&tty->ldisc_mutex);
-
/* At this point we have a closed ldisc and we want to
reopen it. We could defer this to the next open but
it means auditing a lot of other paths so this is
a FIXME */
- if (tty_ldisc_hangup_wait_idle(tty)) {
+ if (tty_ldisc_hangup_halt(tty)) {
if (reset == 0) {
if (!tty_ldisc_reinit(tty, tty->termios.c_line))
--
1.8.0
^ permalink raw reply related
* [PATCH -next 7/9] tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt()
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
In preparation for destructing and freeing the tty, the line discipline
must first be brought to an inactive state before it can be destructed.
This line discipline shutdown must:
- disallow new users of the ldisc
- wait for existing ldisc users to finish
- only then, cancel/flush their pending/running work
Factor tty_ldisc_wait_idle() from tty_set_ldisc() and tty_ldisc_kill()
to ensure this shutdown order.
Failure to provide this guarantee can result in scheduled work
running after the tty has already been freed, as indicated in the
following log message:
[ 88.331234] WARNING: at /home/peter/src/kernels/next/drivers/tty/tty_buffer.c:435 flush_to_ldisc+0x194/0x1d0()
[ 88.334505] Hardware name: Bochs
[ 88.335618] tty is bad=-1
[ 88.335703] Modules linked in: netconsole configfs bnep rfcomm bluetooth ......
[ 88.345272] Pid: 39, comm: kworker/1:1 Tainted: G W 3.7.0-next-20121129+ttydebug-xeon #20121129+ttydebug
[ 88.347736] Call Trace:
[ 88.349024] [<ffffffff81058aff>] warn_slowpath_common+0x7f/0xc0
[ 88.350383] [<ffffffff81058bf6>] warn_slowpath_fmt+0x46/0x50
[ 88.351745] [<ffffffff81432bd4>] flush_to_ldisc+0x194/0x1d0
[ 88.353047] [<ffffffff816f7fe1>] ? _raw_spin_unlock_irq+0x21/0x50
[ 88.354190] [<ffffffff8108a809>] ? finish_task_switch+0x49/0xe0
[ 88.355436] [<ffffffff81077ad1>] process_one_work+0x121/0x490
[ 88.357674] [<ffffffff81432a40>] ? __tty_buffer_flush+0x90/0x90
[ 88.358954] [<ffffffff81078c84>] worker_thread+0x164/0x3e0
[ 88.360247] [<ffffffff81078b20>] ? manage_workers+0x120/0x120
[ 88.361282] [<ffffffff8107e230>] kthread+0xc0/0xd0
[ 88.362284] [<ffffffff816f0000>] ? cmos_do_probe+0x2eb/0x3bf
[ 88.363391] [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[ 88.364797] [<ffffffff816fff6c>] ret_from_fork+0x7c/0xb0
[ 88.366087] [<ffffffff8107e170>] ? flush_kthread_worker+0xb0/0xb0
[ 88.367266] ---[ end trace 453a7c9f38fbfec0 ]---
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index c3dec37..9f4c7b0 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -528,24 +528,38 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
/**
* tty_ldisc_halt - shut down the line discipline
* @tty: tty device
+ * @pending: returns true if work was scheduled when cancelled
+ * (can be set to NULL)
+ * @timeout: # of jiffies to wait for ldisc refs to be released
*
* Shut down the line discipline and work queue for this tty device.
* The TTY_LDISC flag being cleared ensures no further references can
* be obtained while the delayed work queue halt ensures that no more
* data is fed to the ldisc.
*
+ * Furthermore, guarantee that existing ldisc references have been
+ * released, which in turn, guarantees that no future buffer work
+ * can be rescheduled.
+ *
* You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
* in order to make sure any currently executing ldisc work is also
* flushed.
*/
-static int tty_ldisc_halt(struct tty_struct *tty)
+static int tty_ldisc_halt(struct tty_struct *tty, int *pending, long timeout)
{
- int scheduled;
+ int scheduled, retval;
+
clear_bit(TTY_LDISC, &tty->flags);
+ retval = tty_ldisc_wait_idle(tty, timeout);
+ if (retval)
+ return retval;
+
scheduled = cancel_work_sync(&tty->port->buf.work);
set_bit(TTY_LDISC_HALTED, &tty->flags);
- return scheduled;
+ if (pending)
+ *pending = scheduled;
+ return 0;
}
/**
@@ -687,9 +701,9 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
* parallel to the change and re-referencing the tty.
*/
- work = tty_ldisc_halt(tty);
+ retval = tty_ldisc_halt(tty, &work, 5 * HZ);
if (o_tty)
- o_work = tty_ldisc_halt(o_tty);
+ tty_ldisc_halt(o_tty, &o_work, 0);
/*
* Wait for ->hangup_work and ->buf.work handlers to terminate.
@@ -700,8 +714,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
tty_ldisc_flush_works(tty);
- retval = tty_ldisc_wait_idle(tty, 5 * HZ);
-
tty_lock(tty);
mutex_lock(&tty->ldisc_mutex);
@@ -920,11 +932,6 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
static void tty_ldisc_kill(struct tty_struct *tty)
{
- /* There cannot be users from userspace now. But there still might be
- * drivers holding a reference via tty_ldisc_ref. Do not steal them the
- * ldisc until they are done. */
- tty_ldisc_wait_idle(tty, MAX_SCHEDULE_TIMEOUT);
-
mutex_lock(&tty->ldisc_mutex);
/*
* Now kill off the ldisc
@@ -958,10 +965,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
*/
tty_lock_pair(tty, o_tty);
- tty_ldisc_halt(tty);
+ tty_ldisc_halt(tty, NULL, MAX_SCHEDULE_TIMEOUT);
tty_ldisc_flush_works(tty);
if (o_tty) {
- tty_ldisc_halt(o_tty);
+ tty_ldisc_halt(o_tty, NULL, MAX_SCHEDULE_TIMEOUT);
tty_ldisc_flush_works(o_tty);
}
--
1.8.0
^ permalink raw reply related
* [PATCH -next 9/9] tty: Halt both ldiscs concurrently
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
The pty driver does not obtain an ldisc reference to the linked
tty when writing. When the ldiscs are sequentially halted, it
is possible for one ldisc to be halted, and before the second
ldisc can be halted, a concurrent write schedules buffer work on
the first ldisc. This can lead to an access-after-free error when
the scheduled buffer work starts on the closed ldisc.
Prevent subsequent use after halt by performing each stage
of the halt alternately the tty pair.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 19e088a..a6d3078 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -527,37 +527,53 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
/**
* tty_ldisc_halt - shut down the line discipline
* @tty: tty device
+ * @o_tty: paired pty device (can be NULL)
* @pending: returns true if work was scheduled when cancelled
* (can be set to NULL)
+ * @o_pending: returns true if work was scheduled when cancelled
+ * (can be set to NULL)
* @timeout: # of jiffies to wait for ldisc refs to be released
*
- * Shut down the line discipline and work queue for this tty device.
- * The TTY_LDISC flag being cleared ensures no further references can
- * be obtained while the delayed work queue halt ensures that no more
- * data is fed to the ldisc.
+ * Shut down the line discipline and work queue for this tty device and
+ * its paired pty (if exists). Clearing the TTY_LDISC flag ensures
+ * no further references can be obtained while the work queue halt
+ * ensures that no more data is fed to the ldisc.
*
* Furthermore, guarantee that existing ldisc references have been
* released, which in turn, guarantees that no future buffer work
* can be rescheduled.
*
- * You need to do a 'flush_scheduled_work()' (outside the ldisc_mutex)
+ * You need to do a 'tty_ldisc_flush_works()' (outside the ldisc_mutex)
* in order to make sure any currently executing ldisc work is also
* flushed.
*/
-static int tty_ldisc_halt(struct tty_struct *tty, int *pending, long timeout)
+static int tty_ldisc_halt(struct tty_struct *tty, struct tty_struct *o_tty,
+ int *pending, int *o_pending, long timeout)
{
- int scheduled, retval;
+ int scheduled, o_scheduled, retval;
clear_bit(TTY_LDISC, &tty->flags);
+ if (o_tty)
+ clear_bit(TTY_LDISC, &o_tty->flags);
+
retval = tty_ldisc_wait_idle(tty, timeout);
+ if (!retval && o_tty)
+ retval = tty_ldisc_wait_idle(o_tty, timeout);
if (retval)
return retval;
scheduled = cancel_work_sync(&tty->port->buf.work);
set_bit(TTY_LDISC_HALTED, &tty->flags);
+ if (o_tty) {
+ o_scheduled = cancel_work_sync(&o_tty->port->buf.work);
+ set_bit(TTY_LDISC_HALTED, &o_tty->flags);
+ }
+
if (pending)
*pending = scheduled;
+ if (o_tty && o_pending)
+ *o_pending = o_scheduled;
return 0;
}
@@ -700,9 +716,7 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
* parallel to the change and re-referencing the tty.
*/
- retval = tty_ldisc_halt(tty, &work, 5 * HZ);
- if (o_tty)
- tty_ldisc_halt(o_tty, &o_work, 0);
+ retval = tty_ldisc_halt(tty, o_tty, &work, &o_work, 5 * HZ);
/*
* Wait for ->hangup_work and ->buf.work handlers to terminate.
@@ -964,12 +978,10 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
*/
tty_lock_pair(tty, o_tty);
- tty_ldisc_halt(tty, NULL, MAX_SCHEDULE_TIMEOUT);
+ tty_ldisc_halt(tty, o_tty, NULL, NULL, MAX_SCHEDULE_TIMEOUT);
tty_ldisc_flush_works(tty);
- if (o_tty) {
- tty_ldisc_halt(o_tty, NULL, MAX_SCHEDULE_TIMEOUT);
+ if (o_tty)
tty_ldisc_flush_works(o_tty);
- }
/* This will need doing differently if we need to lock */
tty_ldisc_kill(tty);
--
1.8.0
^ permalink raw reply related
* [PATCH -next 0/9] tty: Fix buffer work access-after-free
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
This patch series addresses the causes of flush_to_ldisc accessing
the tty after freeing.
The most common cause stems from the n_tty_close() path spuriously
scheduling buffer work, when the ldisc has already been halted.
This is fixed in 'tty: Don't reschedule buffer work while closing'
The other causes have a central theme: incorrect order-of-operations
when halting a line discipline. In general, to prevent buffer work
from being scheduled requires:
1. Disallowing further ldisc references
2. Waiting for all existing ldisc references to be released
3. Cancelling existing buffer work
If the wait takes place _after_ cancellation, then new work
can be scheduled by existing holder(s) of ldisc reference(s). That's
bad.
Halting the line discipline is performed when,
- hanging up the tty (tty_ldisc_hangup())
- TIOCSETD ioctl (tty_set_ldisc())
- finally closing the tty (pair) (tty_ldisc_release())
Concurrent halts are governed by the following requirements:
1. tty_ldisc_release is not concurrent with the other two and so
does not need lock or state protection during the ldiscs halt.
2. Accesses through tty->ldisc must be protected by the ldisc_mutex.
The wait operation checks the user count (ldisc references)
in tty->ldisc->users.
3. tty_set_ldisc() reschedules buffer work that was pending when
the ldiscs were halted. That must be an atomic operation in
conjunction with re-enabling the ldisc -- which necessitates
locking concurrent halts (tty_ldisc_release is exempt here)
4. The legacy mutex cannot be held while waiting for ldisc
reference(s) release -or- for cancelling buffer work.
5. Because of #4, the legacy mutex must be dropped prior to or
during the halt. Which means reacquiring after the halt. But
to preserve lock order the ldisc_mutex must be dropped and
reacquired after reacquiring the legacy mutex.
6. Because of #5, the ldisc state may have changed while the
ldisc mutex was dropped.
Note: this series does not include the lock correction initially
reported on by Sebastian Andrzej Siewior <bigeasy@linutronix.de> here
https://lkml.org/lkml/2012/11/21/267 . I commented on the latest
version here https://lkml.org/lkml/2012/12/3/362
This series also does not include Jiri's debug patch here
https://lkml.org/lkml/2012/11/2/278 for identifying which itty cleanup
path is used. I think it may be helpful to include this in -next.
Peter Hurley (9):
tty: WARN if buffer work racing with tty free
tty: Add diagnostic for halted line discipline
tty: Don't reschedule buffer work while closing
tty: Refactor wait for ldisc refs out of tty_ldisc_hangup()
tty: Remove unnecessary re-test of ldisc ref count
tty: Fix ldisc halt sequence on hangup
tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt()
tty: Remove unnecessary buffer work flush
tty: Halt both ldiscs concurrently
drivers/tty/n_tty.c | 8 ++-
drivers/tty/tty_buffer.c | 3 +
drivers/tty/tty_io.c | 2 +
drivers/tty/tty_ldisc.c | 171 +++++++++++++++++++++++++++++------------------
include/linux/tty.h | 1 +
5 files changed, 119 insertions(+), 66 deletions(-)
--
1.8.0
^ permalink raw reply
* [PATCH -next 1/9] tty: WARN if buffer work racing with tty free
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_io.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 78c3000..3d2b6d7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1511,6 +1511,8 @@ static void queue_release_one_tty(struct kref *kref)
{
struct tty_struct *tty = container_of(kref, struct tty_struct, kref);
+ WARN_ON(work_pending(&tty->port->buf.work));
+
/* The hangup queue is now free so we can reuse it rather than
waste a chunk of memory for each port */
INIT_WORK(&tty->hangup_work, release_one_tty);
--
1.8.0
^ permalink raw reply related
* [PATCH -next 3/9] tty: Don't reschedule buffer work while closing
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
Prevent buffer work scheduling when called from n_tty_close(). Since
the ldisc has been halted and the tty soon-to-be-destructed, pending
work would be accessing an invalid tty and ldisc state. Fixes this:
[ 38.051111] ------------[ cut here ]------------
[ 38.052113] WARNING: at /home/peter/src/kernels/next/drivers/tty/n_tty.c:160 n_tty_set_room.part.6+0x8b/0xa0()
[ 38.053916] Hardware name: Bochs
[ 38.054819] Modules linked in: netconsole configfs bnep rfcomm bluetooth parport_pc ppdev snd_hda_intel snd_hda_codec
snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq psmouse snd_timer serio_raw mac_hid snd_seq_device
snd microcode lp parport virtio_balloon soundcore i2c_piix4 snd_page_alloc floppy 8139too 8139cp
[ 38.059704] Pid: 1564, comm: pty_kill Tainted: G W 3.7.0-next-20121130+ttydebug-xeon #20121130+ttydebug
[ 38.061578] Call Trace:
[ 38.062491] [<ffffffff81058b4f>] warn_slowpath_common+0x7f/0xc0
[ 38.063448] [<ffffffff81058baa>] warn_slowpath_null+0x1a/0x20
[ 38.064439] [<ffffffff8142dc2b>] n_tty_set_room.part.6+0x8b/0xa0
[ 38.065381] [<ffffffff8142dc82>] n_tty_set_room+0x42/0x80
[ 38.066323] [<ffffffff8142e6f2>] reset_buffer_flags+0x102/0x160
[ 38.077508] [<ffffffff8142e76d>] n_tty_flush_buffer+0x1d/0x90
[ 38.078782] [<ffffffff81046569>] ? default_spin_lock_flags+0x9/0x10
[ 38.079734] [<ffffffff8142e804>] n_tty_close+0x24/0x60
[ 38.080730] [<ffffffff81431b61>] tty_ldisc_close.isra.2+0x41/0x60
[ 38.081680] [<ffffffff81431bbb>] tty_ldisc_kill+0x3b/0x80
[ 38.082618] [<ffffffff81432a07>] tty_ldisc_release+0x77/0xe0
[ 38.083549] [<ffffffff8142b781>] tty_release+0x451/0x4d0
[ 38.084525] [<ffffffff811950be>] __fput+0xae/0x230
[ 38.085472] [<ffffffff8119524e>] ____fput+0xe/0x10
[ 38.086401] [<ffffffff8107aa88>] task_work_run+0xc8/0xf0
[ 38.087334] [<ffffffff8105ea56>] do_exit+0x196/0x4b0
[ 38.088304] [<ffffffff8106c77b>] ? __dequeue_signal+0x6b/0xb0
[ 38.089240] [<ffffffff8105ef34>] do_group_exit+0x44/0xa0
[ 38.090182] [<ffffffff8106f43d>] get_signal_to_deliver+0x20d/0x4e0
[ 38.091125] [<ffffffff81016979>] do_signal+0x29/0x130
[ 38.092096] [<ffffffff81431a9e>] ? tty_ldisc_deref+0xe/0x10
[ 38.093030] [<ffffffff8142a317>] ? tty_write+0xb7/0xf0
[ 38.093976] [<ffffffff81193f53>] ? vfs_write+0xb3/0x180
[ 38.094904] [<ffffffff81016b20>] do_notify_resume+0x80/0xc0
[ 38.095830] [<ffffffff81700492>] int_signal+0x12/0x17
[ 38.096788] ---[ end trace 5f6f7a9651cd999b ]---
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/n_tty.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3f704a9..574d099 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -149,7 +149,7 @@ static void n_tty_set_room(struct tty_struct *tty)
tty->receive_room = left;
/* Did this open up the receive buffer? We may need to flip */
- if (left && !old_left) {
+ if (left && !old_left && !test_bit(TTY_CLOSING, &tty->flags)) {
WARN_RATELIMIT(tty->port->itty == NULL,
"scheduling with invalid itty\n");
/* see if ldisc has been killed - if so, this means that
--
1.8.0
^ permalink raw reply related
* [PATCH -next 8/9] tty: Remove unnecessary buffer work flush
From: Peter Hurley @ 2012-12-04 7:07 UTC (permalink / raw)
To: Alan Cox, Jiri Slaby, Greg Kroah-Hartman
Cc: linux-serial, linux-kernel, Peter Hurley
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
In order to safely call tty_ldisc_flush_works(), the ldisc must
already have been halted, so any pending buffer work has already
been cancelled or flushed as part of the halt.
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
drivers/tty/tty_ldisc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 9f4c7b0..19e088a 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -506,7 +506,6 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
{
flush_work(&tty->hangup_work);
flush_work(&tty->SAK_work);
- flush_work(&tty->port->buf.work);
}
/**
--
1.8.0
^ permalink raw reply related
* Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free
From: Ilya Zykov @ 2012-12-04 7:40 UTC (permalink / raw)
To: Peter Hurley
Cc: Alan Cox, Jiri Slaby, Greg Kroah-Hartman, linux-serial,
linux-kernel
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
On 04.12.2012 11:07, Peter Hurley wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.
>
I think, it is have sense only if you can take effect,
with this patch or something like. I can't. :)
Signed-off-by: Ilya Zykov <ilya@ilyx.ru>
---
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 2ea176b..f24751d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -170,6 +170,10 @@ struct tty_struct *alloc_tty_struct(void)
return kzalloc(sizeof(struct tty_struct), GFP_KERNEL);
}
+static void flush_to_ldisc2(struct work_struct *work)
+{
+ printk(KERN_WARNING "Possible intrusion detected.\n");
+}
/**
* free_tty_struct - free a disused tty
* @tty: tty struct to free
@@ -188,6 +192,8 @@ void free_tty_struct(struct tty_struct *tty)
kfree(tty->write_buf);
tty_buffer_free_all(tty);
tty->magic = 0xDEADDEAD;
+ PREPARE_WORK(&tty->buf.work,flush_to_ldisc2);
+ //memset(tty, 0, sizeof(struct tty_struct));
kfree(tty);
}
^ permalink raw reply related
* Re: [PATCHv2 3/5] serial: 8250_dw: Map IO memory
From: Heikki Krogerus @ 2012-12-04 8:17 UTC (permalink / raw)
To: Jamie Iles; +Cc: Greg Kroah-Hartman, Alan Cox, linux-serial, LKML
In-Reply-To: <20121203154014.GA20140@page>
Hi Jamie,
On Mon, Dec 03, 2012 at 03:40:14PM +0000, Jamie Iles wrote:
> > + uart.port.membase = ioremap(regs->start, regs->end - regs->start);
>
> Doesn't this have an off-by-one error?
True.
> + uart.port.membase = ioremap(regs->start, resource_size(regs));
>
> instead?
Yes, I'll fix this. Thanks!
--
heikki
^ permalink raw reply
* Re: [PATCH] tty: Correct tty buffer flushing.
From: Alan Cox @ 2012-12-04 8:53 UTC (permalink / raw)
To: Ilya Zykov
Cc: Alan Cox, Xiaobing Tu, Jiri Slaby, Alek Du, Greg Kroah-Hartman,
linux-kernel, linux-serial
In-Reply-To: <50B76D96.20809@ilyx.ru>
> Main idea here - we never flash last (struct tty_buffer) in the
> active buffer. Only data for ldisc. (tty->buf.head->read =
> tty->buf.head->commit). At that moment driver can collect(write) data
> in buffer without conflict.
This one I agree with (sorry it took a while to get to, I wanted to sit
and think carefully about it as it is subtle and clever)
Can you generate a single patch which reverts the unneeded locks and
adds this instead. Add a Signed-off-by and send it to Greg to get it
into 3.8/3.9 somewhere.
Thanks
Alan
^ permalink raw reply
* Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free
From: Alan Cox @ 2012-12-04 8:54 UTC (permalink / raw)
To: Peter Hurley; +Cc: Jiri Slaby, Greg Kroah-Hartman, linux-serial, linux-kernel
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
On Tue, 4 Dec 2012 02:07:36 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.
Looks good to me. Would be nice to keep a copy of the test that shows
it up in the comments of the patches somewhere.
^ permalink raw reply
* Re: [PATCH -next 0/9] tty: Fix buffer work access-after-free
From: Jiri Slaby @ 2012-12-04 9:38 UTC (permalink / raw)
To: Peter Hurley; +Cc: Alan Cox, Greg Kroah-Hartman, linux-serial, linux-kernel
In-Reply-To: <1354604865-10278-1-git-send-email-peter@hurleysoftware.com>
On 12/04/2012 08:07 AM, Peter Hurley wrote:
> This patch series addresses the causes of flush_to_ldisc accessing
> the tty after freeing.
Hi, thanks for doing the work. The series looks good to me.
> This series also does not include Jiri's debug patch here
> https://lkml.org/lkml/2012/11/2/278 for identifying which itty cleanup
> path is used. I think it may be helpful to include this in -next.
Feel free to add your SOB and repost. Even though I have some patches in
my queue, I'm not going to send anything for the next merge window as I
will be on longish vacation and don't want to spend it by fixing tty
bugs I could introduce 8).
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH] tty: Correct tty buffer flushing.
From: Ilya Zykov @ 2012-12-04 10:11 UTC (permalink / raw)
To: Alan Cox
Cc: Alan Cox, Xiaobing Tu, Jiri Slaby, Alek Du, Greg Kroah-Hartman,
linux-kernel, linux-serial
In-Reply-To: <20121204085359.03894026@bob.linux.org.uk>
On 04.12.2012 12:53, Alan Cox wrote:
>> Main idea here - we never flash last (struct tty_buffer) in the
>> active buffer. Only data for ldisc. (tty->buf.head->read =
>> tty->buf.head->commit). At that moment driver can collect(write) data
>> in buffer without conflict.
>
> This one I agree with (sorry it took a while to get to, I wanted to sit
> and think carefully about it as it is subtle and clever)
>
> Can you generate a single patch which reverts the unneeded locks and
> adds this instead. Add a Signed-off-by and send it to Greg to get it
> into 3.8/3.9 somewhere.
>
> Thanks
>
> Alan
>
OK.
Thank you.
^ permalink raw reply
* [PATCH] tty: Correct tty buffer flush.
From: Ilya Zykov @ 2012-12-04 13:10 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Alan Cox, linux-kernel, linux-serial
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()),
when another thread can use it. It can be cause of "NULL pointer dereference".
Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer.
Only flush the data for ldisc(tty->buf.head->read = tty->buf.head->commit).
At that moment driver can collect(write) data in buffer without conflict.
It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc.
Also revert:
commit c56a00a165712fd73081f40044b1e64407bb1875
tty: hold lock across tty buffer finding and buffer filling
In order to delete the unneeded locks any more.
Signed-off-by: Ilya Zykov <ilya@ilyx.ru>
---
drivers/tty/tty_buffer.c | 96 +++++++++++++---------------------------------
1 files changed, 27 insertions(+), 69 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 91e326f..4f02f9c 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty)
{
struct tty_buffer *thead;
- while ((thead = tty->buf.head) != NULL) {
- tty->buf.head = thead->next;
- tty_buffer_free(tty, thead);
+ if (tty->buf.head == NULL)
+ return;
+ while ((thead = tty->buf.head->next) != NULL) {
+ tty_buffer_free(tty, tty->buf.head);
+ tty->buf.head = thead;
}
- tty->buf.tail = NULL;
+ WARN_ON(tty->buf.head != tty->buf.tail);
+ tty->buf.head->read = tty->buf.head->commit;
}
/**
@@ -185,19 +188,25 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
/* Should possibly check if this fails for the largest buffer we
have queued and recycle that ? */
}
+
/**
- * __tty_buffer_request_room - grow tty buffer if needed
+ * tty_buffer_request_room - grow tty buffer if needed
* @tty: tty structure
* @size: size desired
*
* Make at least size bytes of linear space available for the tty
* buffer. If we fail return the size we managed to find.
- * Locking: Caller must hold tty->buf.lock
+ *
+ * Locking: Takes tty->buf.lock
*/
-static int __tty_buffer_request_room(struct tty_struct *tty, size_t size)
+int tty_buffer_request_room(struct tty_struct *tty, size_t size)
{
struct tty_buffer *b, *n;
int left;
+ unsigned long flags;
+
+ spin_lock_irqsave(&tty->buf.lock, flags);
+
/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
remove this conditional if its worth it. This would be invisible
to the callers */
@@ -219,29 +228,8 @@ static int __tty_buffer_request_room(struct tty_struct *tty, size_t size)
size = left;
}
- return size;
-}
-
-
-/**
- * tty_buffer_request_room - grow tty buffer if needed
- * @tty: tty structure
- * @size: size desired
- *
- * Make at least size bytes of linear space available for the tty
- * buffer. If we fail return the size we managed to find.
- *
- * Locking: Takes tty->buf.lock
- */
-int tty_buffer_request_room(struct tty_struct *tty, size_t size)
-{
- unsigned long flags;
- int length;
-
- spin_lock_irqsave(&tty->buf.lock, flags);
- length = __tty_buffer_request_room(tty, size);
spin_unlock_irqrestore(&tty->buf.lock, flags);
- return length;
+ return size;
}
EXPORT_SYMBOL_GPL(tty_buffer_request_room);
@@ -264,22 +252,14 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty,
int copied = 0;
do {
int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
- int space;
- unsigned long flags;
- struct tty_buffer *tb;
-
- spin_lock_irqsave(&tty->buf.lock, flags);
- space = __tty_buffer_request_room(tty, goal);
- tb = tty->buf.tail;
+ int space = tty_buffer_request_room(tty, goal);
+ struct tty_buffer *tb = tty->buf.tail;
/* If there is no space then tb may be NULL */
- if (unlikely(space == 0)) {
- spin_unlock_irqrestore(&tty->buf.lock, flags);
+ if (unlikely(space == 0))
break;
- }
memcpy(tb->char_buf_ptr + tb->used, chars, space);
memset(tb->flag_buf_ptr + tb->used, flag, space);
tb->used += space;
- spin_unlock_irqrestore(&tty->buf.lock, flags);
copied += space;
chars += space;
/* There is a small chance that we need to split the data over
@@ -309,22 +289,14 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
int copied = 0;
do {
int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
- int space;
- unsigned long __flags;
- struct tty_buffer *tb;
-
^ 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