2009-11-18 14:30:58

by Alan

[permalink] [raw]
Subject: [PATCH 00/12] Series short description

Further serial whacking (Resend). Mostly attack the moxa/mxser/isicom drivers
and try to beat them into shape.

---

Alan Cox (12):
moxa: split open lock
moxa: Kill the use of lock_kernel
moxa: Fix modem op locking
moxa: Kill off the throttle method
Locking clean up
moxa: rework the locking a bit
moxa: Use more tty_port ops
isicom: fix deadlock on shutdown
mxser: Use the new locking rules to fix setserial properly
mxser: use the tty_port_open method
isicom: sort out the board init logic
isicom: switch to the new tty_port_open helper


drivers/char/isicom.c | 115 ++++---------------
drivers/char/moxa.c | 291 ++++++++++++++----------------------------------
drivers/char/mxser.c | 248 ++++++++++++++++++-----------------------
include/linux/isicom.h | 1
4 files changed, 219 insertions(+), 436 deletions(-)


2009-11-18 14:31:12

by Alan

[permalink] [raw]
Subject: [PATCH 01/12] isicom: switch to the new tty_port_open helper

Trivial conversion in this case so might as well do it while testing the
port_open design is right

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/isicom.c | 88 ++++++++++++-------------------------------------
1 files changed, 21 insertions(+), 67 deletions(-)


diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index 426bfdd..e7be3ec 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -804,24 +804,21 @@ static inline void isicom_setup_board(struct isi_board *bp)
bp->status |= BOARD_ACTIVE;
for (channel = 0; channel < bp->port_count; channel++, port++)
drop_dtr_rts(port);
+ bp->count++;
spin_unlock_irqrestore(&bp->card_lock, flags);
}

-static int isicom_setup_port(struct tty_struct *tty)
+static int isicom_activate(struct tty_port *tport, struct tty_struct *tty)
{
- struct isi_port *port = tty->driver_data;
+ struct isi_port *port = container_of(tport, struct isi_port, port);
struct isi_board *card = port->card;
unsigned long flags;

- if (port->port.flags & ASYNC_INITIALIZED)
- return 0;
- if (tty_port_alloc_xmit_buf(&port->port) < 0)
+ if (tty_port_alloc_xmit_buf(tport) < 0)
return -ENOMEM;

spin_lock_irqsave(&card->card_lock, flags);
- clear_bit(TTY_IO_ERROR, &tty->flags);
- if (port->port.count == 1)
- card->count++;
+ isicom_setup_board(card);

port->xmit_cnt = port->xmit_head = port->xmit_tail = 0;

@@ -832,9 +829,7 @@ static int isicom_setup_port(struct tty_struct *tty)
outw(((ISICOM_KILLTX | ISICOM_KILLRX) << 8) | 0x06, card->base);
InterruptTheCard(card->base);
}
-
isicom_config_port(tty);
- port->port.flags |= ASYNC_INITIALIZED;
spin_unlock_irqrestore(&card->card_lock, flags);

return 0;
@@ -871,31 +866,20 @@ static struct tty_port *isicom_find_port(struct tty_struct *tty)

return &port->port;
}
-
+
static int isicom_open(struct tty_struct *tty, struct file *filp)
{
struct isi_port *port;
struct isi_board *card;
struct tty_port *tport;
- int error = 0;

tport = isicom_find_port(tty);
if (tport == NULL)
return -ENODEV;
port = container_of(tport, struct isi_port, port);
card = &isi_card[BOARD(tty->index)];
- isicom_setup_board(card);

- /* FIXME: locking on port.count etc */
- port->port.count++;
- tty->driver_data = port;
- tty_port_tty_set(&port->port, tty);
- /* FIXME: Locking on Initialized flag */
- if (!test_bit(ASYNCB_INITIALIZED, &tport->flags))
- error = isicom_setup_port(tty);
- if (error == 0)
- error = tty_port_block_til_ready(&port->port, tty, filp);
- return error;
+ return tty_port_open(tport, tty, filp);
}

/* close et all */
@@ -914,40 +898,21 @@ static void isicom_shutdown_port(struct isi_port *port)

tty = tty_port_tty_get(&port->port);

- if (!(port->port.flags & ASYNC_INITIALIZED)) {
- tty_kref_put(tty);
- return;
- }
-
tty_port_free_xmit_buf(&port->port);
- port->port.flags &= ~ASYNC_INITIALIZED;
- /* 3rd October 2000 : Vinayak P Risbud */
- tty_port_tty_set(&port->port, NULL);
-
- /*Fix done by Anil .S on 30-04-2001
- remote login through isi port has dtr toggle problem
- due to which the carrier drops before the password prompt
- appears on the remote end. Now we drop the dtr only if the
- HUPCL(Hangup on close) flag is set for the tty*/
-
- if (C_HUPCL(tty))
- /* drop dtr on this port */
- drop_dtr(port);
-
- /* any other port uninits */
- if (tty)
- set_bit(TTY_IO_ERROR, &tty->flags);
-
if (--card->count < 0) {
pr_dbg("isicom_shutdown_port: bad board(0x%lx) count %d.\n",
card->base, card->count);
card->count = 0;
}

- /* last port was closed, shutdown that boad too */
- if (C_HUPCL(tty)) {
- if (!card->count)
- isicom_shutdown_board(card);
+ /* last port was closed, shutdown that board too */
+ if (tty && C_HUPCL(tty)) {
+ /* FIXME: this logic is bogus - it's the old logic that was
+ bogus before but it still wants fixing */
+ if (!card->count) {
+ if (card->status & BOARD_ACTIVE)
+ card->status &= ~BOARD_ACTIVE;
+ }
}
tty_kref_put(tty);
}
@@ -968,7 +933,7 @@ static void isicom_flush_buffer(struct tty_struct *tty)
tty_wakeup(tty);
}

-static void isicom_close_port(struct tty_port *port)
+static void isicom_shutdown(struct tty_port *port)
{
struct isi_port *ip = container_of(port, struct isi_port, port);
struct isi_board *card = ip->card;
@@ -977,10 +942,8 @@ static void isicom_close_port(struct tty_port *port)
/* indicate to the card that no more data can be received
on this port */
spin_lock_irqsave(&card->card_lock, flags);
- if (port->flags & ASYNC_INITIALIZED) {
- card->port_status &= ~(1 << ip->channel);
- outw(card->port_status, card->base + 0x02);
- }
+ card->port_status &= ~(1 << ip->channel);
+ outw(card->port_status, card->base + 0x02);
isicom_shutdown_port(ip);
spin_unlock_irqrestore(&card->card_lock, flags);
}
@@ -991,12 +954,7 @@ static void isicom_close(struct tty_struct *tty, struct file *filp)
struct tty_port *port = &ip->port;
if (isicom_paranoia_check(ip, tty->name, "isicom_close"))
return;
-
- if (tty_port_close_start(port, tty, filp) == 0)
- return;
- isicom_close_port(port);
- isicom_flush_buffer(tty);
- tty_port_close_end(port, tty);
+ tty_port_close(port, tty, filp);
}

/* write et all */
@@ -1326,15 +1284,9 @@ static void isicom_start(struct tty_struct *tty)
static void isicom_hangup(struct tty_struct *tty)
{
struct isi_port *port = tty->driver_data;
- unsigned long flags;

if (isicom_paranoia_check(port, tty->name, "isicom_hangup"))
return;
-
- spin_lock_irqsave(&port->card->card_lock, flags);
- isicom_shutdown_port(port);
- spin_unlock_irqrestore(&port->card->card_lock, flags);
-
tty_port_hangup(&port->port);
}

@@ -1367,6 +1319,8 @@ static const struct tty_operations isicom_ops = {
static const struct tty_port_operations isicom_port_ops = {
.carrier_raised = isicom_carrier_raised,
.dtr_rts = isicom_dtr_rts,
+ .activate = isicom_activate,
+ .shutdown = isicom_shutdown,
};

static int __devinit reset_card(struct pci_dev *pdev,

2009-11-18 14:31:25

by Alan

[permalink] [raw]
Subject: [PATCH 02/12] isicom: sort out the board init logic

Split this into two flags - INIT meaning the board is set up and ACTIVE
meaning the board has ports open. Remove the broken HUPCL casing and push
the counts somewhere sensible.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/isicom.c | 41 +++++++++++------------------------------
include/linux/isicom.h | 1 +
2 files changed, 12 insertions(+), 30 deletions(-)


diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index e7be3ec..1e91c30 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -793,21 +793,19 @@ static inline void isicom_setup_board(struct isi_board *bp)
{
int channel;
struct isi_port *port;
- unsigned long flags;

- spin_lock_irqsave(&bp->card_lock, flags);
- if (bp->status & BOARD_ACTIVE) {
- spin_unlock_irqrestore(&bp->card_lock, flags);
- return;
- }
- port = bp->ports;
- bp->status |= BOARD_ACTIVE;
- for (channel = 0; channel < bp->port_count; channel++, port++)
- drop_dtr_rts(port);
bp->count++;
- spin_unlock_irqrestore(&bp->card_lock, flags);
+ if (!(bp->status & BOARD_INIT)) {
+ port = bp->ports;
+ for (channel = 0; channel < bp->port_count; channel++, port++)
+ drop_dtr_rts(port);
+ }
+ bp->status |= BOARD_ACTIVE | BOARD_INIT;
}

+/* Activate and thus setup board are protected from races against shutdown
+ by the tty_port mutex */
+
static int isicom_activate(struct tty_port *tport, struct tty_struct *tty)
{
struct isi_port *port = container_of(tport, struct isi_port, port);
@@ -884,19 +882,10 @@ static int isicom_open(struct tty_struct *tty, struct file *filp)

/* close et all */

-static inline void isicom_shutdown_board(struct isi_board *bp)
-{
- if (bp->status & BOARD_ACTIVE)
- bp->status &= ~BOARD_ACTIVE;
-}
-
/* card->lock HAS to be held */
static void isicom_shutdown_port(struct isi_port *port)
{
struct isi_board *card = port->card;
- struct tty_struct *tty;
-
- tty = tty_port_tty_get(&port->port);

tty_port_free_xmit_buf(&port->port);
if (--card->count < 0) {
@@ -904,17 +893,9 @@ static void isicom_shutdown_port(struct isi_port *port)
card->base, card->count);
card->count = 0;
}
-
/* last port was closed, shutdown that board too */
- if (tty && C_HUPCL(tty)) {
- /* FIXME: this logic is bogus - it's the old logic that was
- bogus before but it still wants fixing */
- if (!card->count) {
- if (card->status & BOARD_ACTIVE)
- card->status &= ~BOARD_ACTIVE;
- }
- }
- tty_kref_put(tty);
+ if (!card->count)
+ card->status &= BOARD_ACTIVE;
}

static void isicom_flush_buffer(struct tty_struct *tty)
diff --git a/include/linux/isicom.h b/include/linux/isicom.h
index bbd4219..b92e056 100644
--- a/include/linux/isicom.h
+++ b/include/linux/isicom.h
@@ -67,6 +67,7 @@

#define FIRMWARE_LOADED 0x0001
#define BOARD_ACTIVE 0x0002
+#define BOARD_INIT 0x0004

/* isi_port status bitmap */

2009-11-18 14:31:37

by Alan

[permalink] [raw]
Subject: [PATCH 03/12] mxser: use the tty_port_open method

At first this looks a fairly trivial conversion but we can't quite push
everything into the right format yet. The open side is easy but care is needed
over the setserial methods. Fix up the locking now that we've adopted the
port->mutex locking rule for the initialization.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/mxser.c | 111 ++++++++++++++++----------------------------------
1 files changed, 35 insertions(+), 76 deletions(-)


diff --git a/drivers/char/mxser.c b/drivers/char/mxser.c
index 5e28d39..9dac516 100644
--- a/drivers/char/mxser.c
+++ b/drivers/char/mxser.c
@@ -856,9 +856,9 @@ static void mxser_check_modem_status(struct tty_struct *tty,
}
}

-static int mxser_startup(struct tty_struct *tty)
+static int mxser_activate(struct tty_port *port, struct tty_struct *tty)
{
- struct mxser_port *info = tty->driver_data;
+ struct mxser_port *info = container_of(port, struct mxser_port, port);
unsigned long page;
unsigned long flags;

@@ -868,22 +868,13 @@ static int mxser_startup(struct tty_struct *tty)

spin_lock_irqsave(&info->slock, flags);

- if (info->port.flags & ASYNC_INITIALIZED) {
- free_page(page);
- spin_unlock_irqrestore(&info->slock, flags);
- return 0;
- }
-
if (!info->ioaddr || !info->type) {
set_bit(TTY_IO_ERROR, &tty->flags);
free_page(page);
spin_unlock_irqrestore(&info->slock, flags);
return 0;
}
- if (info->port.xmit_buf)
- free_page(page);
- else
- info->port.xmit_buf = (unsigned char *) page;
+ info->port.xmit_buf = (unsigned char *) page;

/*
* Clear the FIFO buffers and disable them
@@ -951,24 +942,19 @@ static int mxser_startup(struct tty_struct *tty)
* and set the speed of the serial port
*/
mxser_change_speed(tty, NULL);
- info->port.flags |= ASYNC_INITIALIZED;
spin_unlock_irqrestore(&info->slock, flags);

return 0;
}

/*
- * This routine will shutdown a serial port; interrupts maybe disabled, and
- * DTR is dropped if the hangup on close termio flag is on.
+ * This routine will shutdown a serial port
*/
-static void mxser_shutdown(struct tty_struct *tty)
+static void mxser_shutdown_port(struct tty_port *port)
{
- struct mxser_port *info = tty->driver_data;
+ struct mxser_port *info = container_of(port, struct mxser_port, port);
unsigned long flags;

- if (!(info->port.flags & ASYNC_INITIALIZED))
- return;
-
spin_lock_irqsave(&info->slock, flags);

/*
@@ -978,7 +964,7 @@ static void mxser_shutdown(struct tty_struct *tty)
wake_up_interruptible(&info->port.delta_msr_wait);

/*
- * Free the IRQ, if necessary
+ * Free the xmit buffer, if necessary
*/
if (info->port.xmit_buf) {
free_page((unsigned long) info->port.xmit_buf);
@@ -988,10 +974,6 @@ static void mxser_shutdown(struct tty_struct *tty)
info->IER = 0;
outb(0x00, info->ioaddr + UART_IER);

- if (tty->termios->c_cflag & HUPCL)
- info->MCR &= ~(UART_MCR_DTR | UART_MCR_RTS);
- outb(info->MCR, info->ioaddr + UART_MCR);
-
/* clear Rx/Tx FIFO's */
if (info->board->chip_flag)
outb(UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT |
@@ -1004,9 +986,6 @@ static void mxser_shutdown(struct tty_struct *tty)
/* read data port to reset things */
(void) inb(info->ioaddr + UART_RX);

- set_bit(TTY_IO_ERROR, &tty->flags);
-
- info->port.flags &= ~ASYNC_INITIALIZED;

if (info->board->chip_flag)
SET_MOXA_MUST_NO_SOFTWARE_FLOW_CONTROL(info->ioaddr);
@@ -1023,8 +1002,7 @@ static void mxser_shutdown(struct tty_struct *tty)
static int mxser_open(struct tty_struct *tty, struct file *filp)
{
struct mxser_port *info;
- unsigned long flags;
- int retval, line;
+ int line;

line = tty->index;
if (line == MXSER_PORTS)
@@ -1035,23 +1013,7 @@ static int mxser_open(struct tty_struct *tty, struct file *filp)
if (!info->ioaddr)
return -ENODEV;

- tty->driver_data = info;
- tty_port_tty_set(&info->port, tty);
- /*
- * Start up serial port
- */
- spin_lock_irqsave(&info->port.lock, flags);
- info->port.count++;
- spin_unlock_irqrestore(&info->port.lock, flags);
- retval = mxser_startup(tty);
- if (retval)
- return retval;
-
- retval = tty_port_block_til_ready(&info->port, tty, filp);
- if (retval)
- return retval;
-
- return 0;
+ return tty_port_open(&info->port, tty, filp);
}

static void mxser_flush_buffer(struct tty_struct *tty)
@@ -1075,19 +1037,11 @@ static void mxser_flush_buffer(struct tty_struct *tty)
}


-static void mxser_close_port(struct tty_struct *tty, struct tty_port *port)
+static void mxser_close_port(struct tty_port *port)
{
struct mxser_port *info = container_of(port, struct mxser_port, port);
unsigned long timeout;
/*
- * Save the termios structure, since this port may have
- * separate termios for callout and dialin.
- *
- * FIXME: Can this go ?
- */
- if (port->flags & ASYNC_NORMAL_ACTIVE)
- info->normal_termios = *tty->termios;
- /*
* At this point we stop accepting input. To do this, we
* disable the receive line status interrupts, and tell the
* interrupt driver to stop checking the data ready bit in the
@@ -1097,22 +1051,18 @@ static void mxser_close_port(struct tty_struct *tty, struct tty_port *port)
if (info->board->chip_flag)
info->IER &= ~MOXA_MUST_RECV_ISR;

- if (port->flags & ASYNC_INITIALIZED) {
- outb(info->IER, info->ioaddr + UART_IER);
- /*
- * Before we drop DTR, make sure the UART transmitter
- * has completely drained; this is especially
- * important if there is a transmit FIFO!
- */
- timeout = jiffies + HZ;
- while (!(inb(info->ioaddr + UART_LSR) & UART_LSR_TEMT)) {
- schedule_timeout_interruptible(5);
- if (time_after(jiffies, timeout))
- break;
- }
+ outb(info->IER, info->ioaddr + UART_IER);
+ /*
+ * Before we drop DTR, make sure the UART transmitter
+ * has completely drained; this is especially
+ * important if there is a transmit FIFO!
+ */
+ timeout = jiffies + HZ;
+ while (!(inb(info->ioaddr + UART_LSR) & UART_LSR_TEMT)) {
+ schedule_timeout_interruptible(5);
+ if (time_after(jiffies, timeout))
+ break;
}
- mxser_shutdown(tty);
-
}

/*
@@ -1130,8 +1080,12 @@ static void mxser_close(struct tty_struct *tty, struct file *filp)
return;
if (tty_port_close_start(port, tty, filp) == 0)
return;
- mxser_close_port(tty, port);
+ mutex_lock(&port->mutex);
+ mxser_close_port(port);
mxser_flush_buffer(tty);
+ mxser_shutdown_port(port);
+ clear_bit(ASYNCB_INITIALIZED, &port->flags);
+ mutex_unlock(&port->mutex);
/* Right now the tty_port set is done outside of the close_end helper
as we don't yet have everyone using refcounts */
tty_port_close_end(port, tty);
@@ -1329,9 +1283,13 @@ static int mxser_set_serial_info(struct tty_struct *tty,
mxser_change_speed(tty, NULL);
spin_unlock_irqrestore(&info->slock, sl_flags);
}
- } else
- retval = mxser_startup(tty);
-
+ } else {
+ mutex_lock(&info->port.mutex);
+ retval = mxser_activate(&info->port, tty);
+ if (retval == 0)
+ set_bit(ASYNCB_INITIALIZED, &info->port.flags);
+ mutex_unlock(&info->port.mutex);
+ }
return retval;
}

@@ -2059,7 +2017,6 @@ static void mxser_hangup(struct tty_struct *tty)
struct mxser_port *info = tty->driver_data;

mxser_flush_buffer(tty);
- mxser_shutdown(tty);
tty_port_hangup(&info->port);
}

@@ -2363,6 +2320,8 @@ static const struct tty_operations mxser_ops = {
struct tty_port_operations mxser_port_ops = {
.carrier_raised = mxser_carrier_raised,
.dtr_rts = mxser_dtr_rts,
+ .activate = mxser_activate,
+ .shutdown = mxser_shutdown_port,
};

/*

2009-11-18 14:31:50

by Alan

[permalink] [raw]
Subject: [PATCH 04/12] mxser: Use the new locking rules to fix setserial properly

Propogate the init/shutdown mutex through the setserial logic. Use the proper
locks for the various bits still using the BKL. Kill the BKL in this driver.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/mxser.c | 145 +++++++++++++++++++++++++++-----------------------
1 files changed, 78 insertions(+), 67 deletions(-)


diff --git a/drivers/char/mxser.c b/drivers/char/mxser.c
index 9dac516..4c803c7 100644
--- a/drivers/char/mxser.c
+++ b/drivers/char/mxser.c
@@ -23,7 +23,6 @@
#include <linux/errno.h>
#include <linux/signal.h>
#include <linux/sched.h>
-#include <linux/smp_lock.h>
#include <linux/timer.h>
#include <linux/interrupt.h>
#include <linux/tty.h>
@@ -1229,6 +1228,7 @@ static int mxser_set_serial_info(struct tty_struct *tty,
struct serial_struct __user *new_info)
{
struct mxser_port *info = tty->driver_data;
+ struct tty_port *port = &info->port;
struct serial_struct new_serial;
speed_t baud;
unsigned long sl_flags;
@@ -1244,7 +1244,7 @@ static int mxser_set_serial_info(struct tty_struct *tty,
new_serial.port != info->ioaddr)
return -EINVAL;

- flags = info->port.flags & ASYNC_SPD_MASK;
+ flags = port->flags & ASYNC_SPD_MASK;

if (!capable(CAP_SYS_ADMIN)) {
if ((new_serial.baud_base != info->baud_base) ||
@@ -1258,16 +1258,17 @@ static int mxser_set_serial_info(struct tty_struct *tty,
* OK, past this point, all the error checking has been done.
* At this point, we start making changes.....
*/
- info->port.flags = ((info->port.flags & ~ASYNC_FLAGS) |
+ port->flags = ((port->flags & ~ASYNC_FLAGS) |
(new_serial.flags & ASYNC_FLAGS));
- info->port.close_delay = new_serial.close_delay * HZ / 100;
- info->port.closing_wait = new_serial.closing_wait * HZ / 100;
- tty->low_latency = (info->port.flags & ASYNC_LOW_LATENCY)
- ? 1 : 0;
- if ((info->port.flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST &&
+ port->close_delay = new_serial.close_delay * HZ / 100;
+ port->closing_wait = new_serial.closing_wait * HZ / 100;
+ tty->low_latency = (port->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
+ if ((port->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST &&
(new_serial.baud_base != info->baud_base ||
new_serial.custom_divisor !=
info->custom_divisor)) {
+ if (new_serial.custom_divisor == 0)
+ return -EINVAL;
baud = new_serial.baud_base / new_serial.custom_divisor;
tty_encode_baud_rate(tty, baud, baud);
}
@@ -1277,18 +1278,16 @@ static int mxser_set_serial_info(struct tty_struct *tty,

process_txrx_fifo(info);

- if (info->port.flags & ASYNC_INITIALIZED) {
- if (flags != (info->port.flags & ASYNC_SPD_MASK)) {
+ if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+ if (flags != (port->flags & ASYNC_SPD_MASK)) {
spin_lock_irqsave(&info->slock, sl_flags);
mxser_change_speed(tty, NULL);
spin_unlock_irqrestore(&info->slock, sl_flags);
}
} else {
- mutex_lock(&info->port.mutex);
- retval = mxser_activate(&info->port, tty);
+ retval = mxser_activate(port, tty);
if (retval == 0)
- set_bit(ASYNCB_INITIALIZED, &info->port.flags);
- mutex_unlock(&info->port.mutex);
+ set_bit(ASYNCB_INITIALIZED, &port->flags);
}
return retval;
}
@@ -1478,7 +1477,8 @@ static int __init mxser_read_register(int port, unsigned short *regs)

static int mxser_ioctl_special(unsigned int cmd, void __user *argp)
{
- struct mxser_port *port;
+ struct mxser_port *ip;
+ struct tty_port *port;
struct tty_struct *tty;
int result, status;
unsigned int i, j;
@@ -1494,38 +1494,39 @@ static int mxser_ioctl_special(unsigned int cmd, void __user *argp)

case MOXA_CHKPORTENABLE:
result = 0;
- lock_kernel();
for (i = 0; i < MXSER_BOARDS; i++)
for (j = 0; j < MXSER_PORTS_PER_BOARD; j++)
if (mxser_boards[i].ports[j].ioaddr)
result |= (1 << i);
- unlock_kernel();
return put_user(result, (unsigned long __user *)argp);
case MOXA_GETDATACOUNT:
- lock_kernel();
+ /* The receive side is locked by port->slock but it isn't
+ clear that an exact snapshot is worth copying here */
if (copy_to_user(argp, &mxvar_log, sizeof(mxvar_log)))
ret = -EFAULT;
- unlock_kernel();
return ret;
case MOXA_GETMSTATUS: {
struct mxser_mstatus ms, __user *msu = argp;
- lock_kernel();
for (i = 0; i < MXSER_BOARDS; i++)
for (j = 0; j < MXSER_PORTS_PER_BOARD; j++) {
- port = &mxser_boards[i].ports[j];
+ ip = &mxser_boards[i].ports[j];
+ port = &ip->port;
memset(&ms, 0, sizeof(ms));

- if (!port->ioaddr)
+ mutex_lock(&port->mutex);
+ if (!ip->ioaddr)
goto copy;

- tty = tty_port_tty_get(&port->port);
+ tty = tty_port_tty_get(port);

if (!tty || !tty->termios)
- ms.cflag = port->normal_termios.c_cflag;
+ ms.cflag = ip->normal_termios.c_cflag;
else
ms.cflag = tty->termios->c_cflag;
tty_kref_put(tty);
- status = inb(port->ioaddr + UART_MSR);
+ spin_lock_irq(&ip->slock);
+ status = inb(ip->ioaddr + UART_MSR);
+ spin_unlock_irq(&ip->slock);
if (status & UART_MSR_DCD)
ms.dcd = 1;
if (status & UART_MSR_DSR)
@@ -1533,13 +1534,11 @@ static int mxser_ioctl_special(unsigned int cmd, void __user *argp)
if (status & UART_MSR_CTS)
ms.cts = 1;
copy:
- if (copy_to_user(msu, &ms, sizeof(ms))) {
- unlock_kernel();
+ mutex_unlock(&port->mutex);
+ if (copy_to_user(msu, &ms, sizeof(ms)))
return -EFAULT;
- }
msu++;
}
- unlock_kernel();
return 0;
}
case MOXA_ASPP_MON_EXT: {
@@ -1551,41 +1550,48 @@ static int mxser_ioctl_special(unsigned int cmd, void __user *argp)
if (!me)
return -ENOMEM;

- lock_kernel();
for (i = 0, p = 0; i < MXSER_BOARDS; i++) {
for (j = 0; j < MXSER_PORTS_PER_BOARD; j++, p++) {
if (p >= ARRAY_SIZE(me->rx_cnt)) {
i = MXSER_BOARDS;
break;
}
- port = &mxser_boards[i].ports[j];
- if (!port->ioaddr)
+ ip = &mxser_boards[i].ports[j];
+ port = &ip->port;
+
+ mutex_lock(&port->mutex);
+ if (!ip->ioaddr) {
+ mutex_unlock(&port->mutex);
continue;
+ }

- status = mxser_get_msr(port->ioaddr, 0, p);
+ spin_lock_irq(&ip->slock);
+ status = mxser_get_msr(ip->ioaddr, 0, p);

if (status & UART_MSR_TERI)
- port->icount.rng++;
+ ip->icount.rng++;
if (status & UART_MSR_DDSR)
- port->icount.dsr++;
+ ip->icount.dsr++;
if (status & UART_MSR_DDCD)
- port->icount.dcd++;
+ ip->icount.dcd++;
if (status & UART_MSR_DCTS)
- port->icount.cts++;
+ ip->icount.cts++;

- port->mon_data.modem_status = status;
- me->rx_cnt[p] = port->mon_data.rxcnt;
- me->tx_cnt[p] = port->mon_data.txcnt;
- me->up_rxcnt[p] = port->mon_data.up_rxcnt;
- me->up_txcnt[p] = port->mon_data.up_txcnt;
+ ip->mon_data.modem_status = status;
+ me->rx_cnt[p] = ip->mon_data.rxcnt;
+ me->tx_cnt[p] = ip->mon_data.txcnt;
+ me->up_rxcnt[p] = ip->mon_data.up_rxcnt;
+ me->up_txcnt[p] = ip->mon_data.up_txcnt;
me->modem_status[p] =
- port->mon_data.modem_status;
- tty = tty_port_tty_get(&port->port);
+ ip->mon_data.modem_status;
+ spin_unlock_irq(&ip->slock);
+
+ tty = tty_port_tty_get(&ip->port);

if (!tty || !tty->termios) {
- cflag = port->normal_termios.c_cflag;
- iflag = port->normal_termios.c_iflag;
- me->baudrate[p] = tty_termios_baud_rate(&port->normal_termios);
+ cflag = ip->normal_termios.c_cflag;
+ iflag = ip->normal_termios.c_iflag;
+ me->baudrate[p] = tty_termios_baud_rate(&ip->normal_termios);
} else {
cflag = tty->termios->c_cflag;
iflag = tty->termios->c_iflag;
@@ -1604,16 +1610,15 @@ static int mxser_ioctl_special(unsigned int cmd, void __user *argp)
if (iflag & (IXON | IXOFF))
me->flowctrl[p] |= 0x0C;

- if (port->type == PORT_16550A)
+ if (ip->type == PORT_16550A)
me->fifo[p] = 1;

- opmode = inb(port->opmode_ioaddr) >>
- ((p % 4) * 2);
+ opmode = inb(ip->opmode_ioaddr)>>((p % 4) * 2);
opmode &= OP_MODE_MASK;
me->iftype[p] = opmode;
+ mutex_unlock(&port->mutex);
}
}
- unlock_kernel();
if (copy_to_user(argp, me, sizeof(*me)))
ret = -EFAULT;
kfree(me);
@@ -1650,6 +1655,7 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg)
{
struct mxser_port *info = tty->driver_data;
+ struct tty_port *port = &info->port;
struct async_icount cnow;
unsigned long flags;
void __user *argp = (void __user *)arg;
@@ -1674,20 +1680,20 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,
opmode != RS422_MODE &&
opmode != RS485_4WIRE_MODE)
return -EFAULT;
- lock_kernel();
mask = ModeMask[p];
shiftbit = p * 2;
+ spin_lock_irq(&info->slock);
val = inb(info->opmode_ioaddr);
val &= mask;
val |= (opmode << shiftbit);
outb(val, info->opmode_ioaddr);
- unlock_kernel();
+ spin_unlock_irq(&info->slock);
} else {
- lock_kernel();
shiftbit = p * 2;
+ spin_lock_irq(&info->slock);
opmode = inb(info->opmode_ioaddr) >> shiftbit;
+ spin_unlock_irq(&info->slock);
opmode &= OP_MODE_MASK;
- unlock_kernel();
if (put_user(opmode, (int __user *)argp))
return -EFAULT;
}
@@ -1700,14 +1706,14 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,

switch (cmd) {
case TIOCGSERIAL:
- lock_kernel();
+ mutex_lock(&port->mutex);
retval = mxser_get_serial_info(tty, argp);
- unlock_kernel();
+ mutex_unlock(&port->mutex);
return retval;
case TIOCSSERIAL:
- lock_kernel();
+ mutex_lock(&port->mutex);
retval = mxser_set_serial_info(tty, argp);
- unlock_kernel();
+ mutex_unlock(&port->mutex);
return retval;
case TIOCSERGETLSR: /* Get line status register */
return mxser_get_lsr_info(info, argp);
@@ -1753,31 +1759,33 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,
case MOXA_HighSpeedOn:
return put_user(info->baud_base != 115200 ? 1 : 0, (int __user *)argp);
case MOXA_SDS_RSTICOUNTER:
- lock_kernel();
+ spin_lock_irq(&info->slock);
info->mon_data.rxcnt = 0;
info->mon_data.txcnt = 0;
- unlock_kernel();
+ spin_unlock_irq(&info->slock);
return 0;

case MOXA_ASPP_OQUEUE:{
int len, lsr;

- lock_kernel();
len = mxser_chars_in_buffer(tty);
+ spin_lock(&info->slock);
lsr = inb(info->ioaddr + UART_LSR) & UART_LSR_THRE;
+ spin_unlock_irq(&info->slock);
len += (lsr ? 0 : 1);
- unlock_kernel();

return put_user(len, (int __user *)argp);
}
case MOXA_ASPP_MON: {
int mcr, status;

- lock_kernel();
+ spin_lock(&info->slock);
status = mxser_get_msr(info->ioaddr, 1, tty->index);
mxser_check_modem_status(tty, info, status);

mcr = inb(info->ioaddr + UART_MCR);
+ spin_unlock(&info->slock);
+
if (mcr & MOXA_MUST_MCR_XON_FLAG)
info->mon_data.hold_reason &= ~NPPI_NOTIFY_XOFFHOLD;
else
@@ -1792,7 +1800,7 @@ static int mxser_ioctl(struct tty_struct *tty, struct file *file,
info->mon_data.hold_reason |= NPPI_NOTIFY_CTSHOLD;
else
info->mon_data.hold_reason &= ~NPPI_NOTIFY_CTSHOLD;
- unlock_kernel();
+
if (copy_to_user(argp, &info->mon_data,
sizeof(struct mxser_mon)))
return -EFAULT;
@@ -1951,6 +1959,7 @@ static void mxser_wait_until_sent(struct tty_struct *tty, int timeout)
{
struct mxser_port *info = tty->driver_data;
unsigned long orig_jiffies, char_time;
+ unsigned long flags;
int lsr;

if (info->type == PORT_UNKNOWN)
@@ -1990,19 +1999,21 @@ static void mxser_wait_until_sent(struct tty_struct *tty, int timeout)
timeout, char_time);
printk("jiff=%lu...", jiffies);
#endif
- lock_kernel();
+ spin_lock_irqsave(&info->slock, flags);
while (!((lsr = inb(info->ioaddr + UART_LSR)) & UART_LSR_TEMT)) {
#ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
#endif
+ spin_unlock_irqrestore(&info->slock, flags);
schedule_timeout_interruptible(char_time);
if (signal_pending(current))
break;
if (timeout && time_after(jiffies, orig_jiffies + timeout))
break;
+ spin_lock_irqsave(&info->slock, flags);
}
+ spin_unlock_irqrestore(&info->slock, flags);
set_current_state(TASK_RUNNING);
- unlock_kernel();

#ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
printk("lsr = %d (jiff=%lu)...done\n", lsr, jiffies);

2009-11-18 14:32:04

by Alan

[permalink] [raw]
Subject: [PATCH 05/12] isicom: fix deadlock on shutdown

Alexander Strakh <[email protected]> reported

KERNEL_VERSION: 2.6.31
DESCRIBE:
Driver drivers/char/isicom.c might sleep in atomic context, because it
calls
tty_port_xmit_buf under spin_lock.

./drivers/char/isicom.c:
1307 static void isicom_hangup(struct tty_struct *tty)
1308 {
...
1315 spin_lock_irqsave(&port->card->card_lock, flags);
1316 isicom_shutdown_port(port);
...

Path to might_sleep macro from isicom_hangup:
1. isicom_hangup calls spin_lock_irqsave (drivers/char/isicom.c:1315) and
then
calls isicom_shutdown_port.
2. isiscom_shutdown_port calls tty_port_free_xmit_buf at
drivers/char/isicom.c:906
3. tty_port_free_xmit_buf calls mutex_lock at drivers/char/tty_port:48

Found by Linux Driver Verification Project.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/isicom.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index 1e91c30..300d5bd 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -887,7 +887,6 @@ static void isicom_shutdown_port(struct isi_port *port)
{
struct isi_board *card = port->card;

- tty_port_free_xmit_buf(&port->port);
if (--card->count < 0) {
pr_dbg("isicom_shutdown_port: bad board(0x%lx) count %d.\n",
card->base, card->count);
@@ -927,6 +926,7 @@ static void isicom_shutdown(struct tty_port *port)
outw(card->port_status, card->base + 0x02);
isicom_shutdown_port(ip);
spin_unlock_irqrestore(&card->card_lock, flags);
+ tty_port_free_xmit_buf(port);
}

static void isicom_close(struct tty_struct *tty, struct file *filp)

2009-11-18 14:32:17

by Alan

[permalink] [raw]
Subject: [PATCH 06/12] moxa: Use more tty_port ops

Rework a few bits of this into tty_port format

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 139 ++++++++-------------------------------------------
1 files changed, 22 insertions(+), 117 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index dd0083b..96b2985 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -206,8 +206,9 @@ static int moxa_tiocmset(struct tty_struct *tty, struct file *file,
static void moxa_poll(unsigned long);
static void moxa_set_tty_param(struct tty_struct *, struct ktermios *);
static void moxa_setup_empty_event(struct tty_struct *);
-static void moxa_shut_down(struct tty_struct *);
+static void moxa_shutdown(struct tty_port *);
static int moxa_carrier_raised(struct tty_port *);
+static void moxa_dtr_rts(struct tty_port *, int);
/*
* moxa board interface functions:
*/
@@ -409,6 +410,8 @@ static const struct tty_operations moxa_ops = {

static const struct tty_port_operations moxa_port_ops = {
.carrier_raised = moxa_carrier_raised,
+ .dtr_rts = moxa_dtr_rts,
+ .shutdown = moxa_shutdown,
};

static struct tty_driver *moxaDriver;
@@ -1112,14 +1115,12 @@ static void __exit moxa_exit(void)
module_init(moxa_init);
module_exit(moxa_exit);

-static void moxa_close_port(struct tty_struct *tty)
+static void moxa_shutdown(struct tty_port *port)
{
- struct moxa_port *ch = tty->driver_data;
- moxa_shut_down(tty);
+ struct moxa_port *ch = container_of(port, struct moxa_port, port);
+ MoxaPortDisable(ch);
MoxaPortFlushData(ch, 2);
- ch->port.flags &= ~ASYNC_NORMAL_ACTIVE;
- tty->driver_data = NULL;
- tty_port_tty_set(&ch->port, NULL);
+ clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
}

static int moxa_carrier_raised(struct tty_port *port)
@@ -1133,39 +1134,13 @@ static int moxa_carrier_raised(struct tty_port *port)
return dcd;
}

-static int moxa_block_till_ready(struct tty_struct *tty, struct file *filp,
- struct moxa_port *ch)
+static void moxa_dtr_rts(struct tty_port *port, int onoff)
{
- struct tty_port *port = &ch->port;
- DEFINE_WAIT(wait);
- int retval = 0;
- u8 dcd;
-
- while (1) {
- prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
- if (tty_hung_up_p(filp)) {
-#ifdef SERIAL_DO_RESTART
- retval = -ERESTARTSYS;
-#else
- retval = -EAGAIN;
-#endif
- break;
- }
- dcd = tty_port_carrier_raised(port);
- if (dcd)
- break;
-
- if (signal_pending(current)) {
- retval = -ERESTARTSYS;
- break;
- }
- schedule();
- }
- finish_wait(&port->open_wait, &wait);
-
- return retval;
+ struct moxa_port *ch = container_of(port, struct moxa_port, port);
+ MoxaPortLineCtrl(ch, onoff, onoff);
}

+
static int moxa_open(struct tty_struct *tty, struct file *filp)
{
struct moxa_board_conf *brd;
@@ -1194,6 +1169,7 @@ static int moxa_open(struct tty_struct *tty, struct file *filp)
ch->port.count++;
tty->driver_data = ch;
tty_port_tty_set(&ch->port, tty);
+ mutex_lock(&ch->port.mutex);
if (!(ch->port.flags & ASYNC_INITIALIZED)) {
ch->statusflags = 0;
moxa_set_tty_param(tty, tty->termios);
@@ -1202,57 +1178,21 @@ static int moxa_open(struct tty_struct *tty, struct file *filp)
MoxaSetFifo(ch, ch->type == PORT_16550A);
ch->port.flags |= ASYNC_INITIALIZED;
}
+ mutex_unlock(&ch->port.mutex);
mutex_unlock(&moxa_openlock);
-
- retval = 0;
- if (!(filp->f_flags & O_NONBLOCK) && !C_CLOCAL(tty))
- retval = moxa_block_till_ready(tty, filp, ch);
- mutex_lock(&moxa_openlock);
- if (retval) {
- if (ch->port.count) /* 0 means already hung up... */
- if (--ch->port.count == 0)
- moxa_close_port(tty);
- } else
- ch->port.flags |= ASYNC_NORMAL_ACTIVE;
- mutex_unlock(&moxa_openlock);
-
+
+ retval = tty_port_block_til_ready(&ch->port, tty, filp);
+ if (retval == 0)
+ set_bit(ASYNCB_NORMAL_ACTIVE, &ch->port.flags);
return retval;
}

static void moxa_close(struct tty_struct *tty, struct file *filp)
{
- struct moxa_port *ch;
- int port;
-
- port = tty->index;
- if (port == MAX_PORTS || tty_hung_up_p(filp))
- return;
-
- mutex_lock(&moxa_openlock);
- ch = tty->driver_data;
- if (ch == NULL)
- goto unlock;
- if (tty->count == 1 && ch->port.count != 1) {
- printk(KERN_WARNING "moxa_close: bad serial port count; "
- "tty->count is 1, ch->port.count is %d\n", ch->port.count);
- ch->port.count = 1;
- }
- if (--ch->port.count < 0) {
- printk(KERN_WARNING "moxa_close: bad serial port count, "
- "device=%s\n", tty->name);
- ch->port.count = 0;
- }
- if (ch->port.count)
- goto unlock;
-
+ struct moxa_port *ch = tty->driver_data;
ch->cflag = tty->termios->c_cflag;
- if (ch->port.flags & ASYNC_INITIALIZED) {
- moxa_setup_empty_event(tty);
- tty_wait_until_sent(tty, 30 * HZ); /* 30 seconds timeout */
- }
-
- moxa_close_port(tty);
-unlock:
+ mutex_lock(&moxa_openlock);
+ tty_port_close(&ch->port, tty, filp);
mutex_unlock(&moxa_openlock);
}

@@ -1300,14 +1240,6 @@ static int moxa_chars_in_buffer(struct tty_struct *tty)
struct moxa_port *ch = tty->driver_data;
int chars;

- /*
- * Sigh...I have to check if driver_data is NULL here, because
- * if an open() fails, the TTY subsystem eventually calls
- * tty_wait_until_sent(), which calls the driver's chars_in_buffer()
- * routine. And since the open() failed, we return 0 here. TDJ
- */
- if (ch == NULL)
- return 0;
lock_kernel();
chars = MoxaPortTxQueue(ch);
if (chars) {
@@ -1436,15 +1368,8 @@ static void moxa_hangup(struct tty_struct *tty)

mutex_lock(&moxa_openlock);
ch = tty->driver_data;
- if (ch == NULL) {
- mutex_unlock(&moxa_openlock);
- return;
- }
- ch->port.count = 0;
- moxa_close_port(tty);
+ tty_port_hangup(&ch->port);
mutex_unlock(&moxa_openlock);
-
- wake_up_interruptible(&ch->port.open_wait);
}

static void moxa_new_dcdstate(struct moxa_port *p, u8 dcd)
@@ -1597,26 +1522,6 @@ static void moxa_setup_empty_event(struct tty_struct *tty)
spin_unlock_bh(&moxa_lock);
}

-static void moxa_shut_down(struct tty_struct *tty)
-{
- struct moxa_port *ch = tty->driver_data;
-
- if (!(ch->port.flags & ASYNC_INITIALIZED))
- return;
-
- MoxaPortDisable(ch);
-
- /*
- * If we're a modem control device and HUPCL is on, drop RTS & DTR.
- */
- if (C_HUPCL(tty))
- MoxaPortLineCtrl(ch, 0, 0);
-
- spin_lock_bh(&moxa_lock);
- ch->port.flags &= ~ASYNC_INITIALIZED;
- spin_unlock_bh(&moxa_lock);
-}
-
/*****************************************************************************
* Driver level functions: *
*****************************************************************************/

2009-11-18 14:32:25

by Alan

[permalink] [raw]
Subject: [PATCH 07/12] moxa: rework the locking a bit

Introduce a lock for moxafunc() to protect the cases where were get collisions
between two function requests at the same time.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 27 ++++++++++++++++++++++-----
1 files changed, 22 insertions(+), 5 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index 96b2985..ba84c4d 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -248,9 +248,25 @@ static void moxa_wait_finish(void __iomem *ofsAddr)

static void moxafunc(void __iomem *ofsAddr, u16 cmd, u16 arg)
{
+ unsigned long flags;
+ spin_lock_irqsave(&moxafunc_lock, flags);
writew(arg, ofsAddr + FuncArg);
writew(cmd, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
+ spin_unlock_irqrestore(&moxafunc_lock, flags);
+}
+
+static int moxafuncret(void __iomem *ofsAddr, u16 cmd, u16 arg)
+{
+ unsigned long flags;
+ u16 ret;
+ spin_lock_irqsave(&moxafunc_lock, flags);
+ writew(arg, ofsAddr + FuncArg);
+ writew(cmd, ofsAddr + FuncCode);
+ moxa_wait_finish(ofsAddr);
+ ret = readw(ofsAddr + FuncArg);
+ spin_unlock_irqrestore(&moxafunc_lock, flags);
+ return ret;
}

static void moxa_low_water_check(void __iomem *ofsAddr)
@@ -417,6 +433,7 @@ static const struct tty_port_operations moxa_port_ops = {
static struct tty_driver *moxaDriver;
static DEFINE_TIMER(moxaTimer, moxa_poll, 0, 0);
static DEFINE_SPINLOCK(moxa_lock);
+static DEFINE_SPINLOCK(moxafunc_lock);

/*
* HW init
@@ -1823,10 +1840,12 @@ static int MoxaPortSetTermio(struct moxa_port *port, struct ktermios *termio,
baud = MoxaPortSetBaud(port, baud);

if (termio->c_iflag & (IXON | IXOFF | IXANY)) {
+ spin_lock_irq(&moxafunc_lock);
writeb(termio->c_cc[VSTART], ofsAddr + FuncArg);
writeb(termio->c_cc[VSTOP], ofsAddr + FuncArg1);
writeb(FC_SetXonXoff, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
+ spin_unlock_irqrestore(&moxafunc_lock);

}
return baud;
@@ -1879,12 +1898,10 @@ static int MoxaPortLineStatus(struct moxa_port *port)
int val;

ofsAddr = port->tableAddr;
- if (MOXA_IS_320(port->board)) {
- moxafunc(ofsAddr, FC_LineStatus, 0);
- val = readw(ofsAddr + FuncArg);
- } else {
+ if (MOXA_IS_320(port->board))
+ val = moxafuncret(ofsAddr, FC_LineStatus, 0);
+ else
val = readw(ofsAddr + FlagStat) >> 4;
- }
val &= 0x0B;
if (val & 8)
val |= 4;

2009-11-18 14:32:53

by Alan

[permalink] [raw]
Subject: [PATCH 08/12] Locking clean up

- The open lock is needed to fix up the case of a board reset occuring during
tty open but too early for a sane hangup response.
- The lock however got used for other cases
- Use the port mutex for get/setserial
- Fix up the confused lack of locking on the THROTTLE and other bits in the
private flags. Just use set/test/clear bit and it covers the cases we need

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 58 ++++++++++++++++++++++-----------------------------
1 files changed, 25 insertions(+), 33 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index ba84c4d..e6a79b0 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -151,10 +151,10 @@ struct mon_str {
};

/* statusflags */
-#define TXSTOPPED 0x1
-#define LOWWAIT 0x2
-#define EMPTYWAIT 0x4
-#define THROTTLE 0x8
+#define TXSTOPPED 1
+#define LOWWAIT 2
+#define EMPTYWAIT 3
+#define THROTTLE 4

#define SERIAL_DO_RESTART

@@ -235,6 +235,8 @@ static void MoxaSetFifo(struct moxa_port *port, int enable);
* I/O functions
*/

+static DEFINE_SPINLOCK(moxafunc_lock);
+
static void moxa_wait_finish(void __iomem *ofsAddr)
{
unsigned long end = jiffies + moxaFuncTout;
@@ -381,14 +383,14 @@ copy:
break;
}
case TIOCGSERIAL:
- mutex_lock(&moxa_openlock);
+ mutex_lock(&ch->port.mutex);
ret = moxa_get_serial_info(ch, argp);
- mutex_unlock(&moxa_openlock);
+ mutex_unlock(&ch->port.mutex);
break;
case TIOCSSERIAL:
- mutex_lock(&moxa_openlock);
+ mutex_lock(&ch->port.mutex);
ret = moxa_set_serial_info(ch, argp);
- mutex_unlock(&moxa_openlock);
+ mutex_unlock(&ch->port.mutex);
break;
default:
ret = -ENOIOCTLCMD;
@@ -433,7 +435,6 @@ static const struct tty_port_operations moxa_port_ops = {
static struct tty_driver *moxaDriver;
static DEFINE_TIMER(moxaTimer, moxa_poll, 0, 0);
static DEFINE_SPINLOCK(moxa_lock);
-static DEFINE_SPINLOCK(moxafunc_lock);

/*
* HW init
@@ -1208,9 +1209,7 @@ static void moxa_close(struct tty_struct *tty, struct file *filp)
{
struct moxa_port *ch = tty->driver_data;
ch->cflag = tty->termios->c_cflag;
- mutex_lock(&moxa_openlock);
tty_port_close(&ch->port, tty, filp);
- mutex_unlock(&moxa_openlock);
}

static int moxa_write(struct tty_struct *tty,
@@ -1226,7 +1225,7 @@ static int moxa_write(struct tty_struct *tty,
len = MoxaPortWriteData(tty, buf, count);
spin_unlock_bh(&moxa_lock);

- ch->statusflags |= LOWWAIT;
+ set_bit(LOWWAIT, &ch->statusflags);
return len;
}

@@ -1264,7 +1263,7 @@ static int moxa_chars_in_buffer(struct tty_struct *tty)
* Make it possible to wakeup anything waiting for output
* in tty_ioctl.c, etc.
*/
- if (!(ch->statusflags & EMPTYWAIT))
+ if (!test_bit(EMPTYWAIT, &ch->statusflags))
moxa_setup_empty_event(tty);
}
unlock_kernel();
@@ -1332,14 +1331,14 @@ static void moxa_throttle(struct tty_struct *tty)
{
struct moxa_port *ch = tty->driver_data;

- ch->statusflags |= THROTTLE;
+ set_bit(THROTTLE, &ch->statusflags);
}

static void moxa_unthrottle(struct tty_struct *tty)
{
struct moxa_port *ch = tty->driver_data;

- ch->statusflags &= ~THROTTLE;
+ clear_bit(THROTTLE, &ch->statusflags);
}

static void moxa_set_termios(struct tty_struct *tty,
@@ -1361,7 +1360,7 @@ static void moxa_stop(struct tty_struct *tty)
if (ch == NULL)
return;
MoxaPortTxDisable(ch);
- ch->statusflags |= TXSTOPPED;
+ set_bit(TXSTOPPED, &ch->statusflags);
}


@@ -1376,17 +1375,13 @@ static void moxa_start(struct tty_struct *tty)
return;

MoxaPortTxEnable(ch);
- ch->statusflags &= ~TXSTOPPED;
+ clear_bit(TXSTOPPED, &ch->statusflags);
}

static void moxa_hangup(struct tty_struct *tty)
{
- struct moxa_port *ch;
-
- mutex_lock(&moxa_openlock);
- ch = tty->driver_data;
+ struct moxa_port *ch = tty->driver_data;
tty_port_hangup(&ch->port);
- mutex_unlock(&moxa_openlock);
}

static void moxa_new_dcdstate(struct moxa_port *p, u8 dcd)
@@ -1412,24 +1407,24 @@ static int moxa_poll_port(struct moxa_port *p, unsigned int handle,
u16 intr;

if (tty) {
- if ((p->statusflags & EMPTYWAIT) &&
+ if (test_bit(EMPTYWAIT, &p->statusflags) &&
MoxaPortTxQueue(p) == 0) {
- p->statusflags &= ~EMPTYWAIT;
+ clear_bit(EMPTYWAIT, &p->statusflags);
tty_wakeup(tty);
}
- if ((p->statusflags & LOWWAIT) && !tty->stopped &&
+ if (test_bit(LOWWAIT, &p->statusflags) && !tty->stopped &&
MoxaPortTxQueue(p) <= WAKEUP_CHARS) {
- p->statusflags &= ~LOWWAIT;
+ clear_bit(LOWWAIT, &p->statusflags);
tty_wakeup(tty);
}

- if (inited && !(p->statusflags & THROTTLE) &&
+ if (inited && !test_bit(THROTTLE, &p->statusflags) &&
MoxaPortRxQueue(p) > 0) { /* RX */
MoxaPortReadData(p);
tty_schedule_flip(tty);
}
} else {
- p->statusflags &= ~EMPTYWAIT;
+ clear_bit(EMPTYWAIT, &p->statusflags);
MoxaPortFlushData(p, 0); /* flush RX */
}

@@ -1533,10 +1528,7 @@ static void moxa_set_tty_param(struct tty_struct *tty, struct ktermios *old_term
static void moxa_setup_empty_event(struct tty_struct *tty)
{
struct moxa_port *ch = tty->driver_data;
-
- spin_lock_bh(&moxa_lock);
- ch->statusflags |= EMPTYWAIT;
- spin_unlock_bh(&moxa_lock);
+ set_bit(EMPTYWAIT, &ch->statusflags);
}

/*****************************************************************************
@@ -1845,7 +1837,7 @@ static int MoxaPortSetTermio(struct moxa_port *port, struct ktermios *termio,
writeb(termio->c_cc[VSTOP], ofsAddr + FuncArg1);
writeb(FC_SetXonXoff, ofsAddr + FuncCode);
moxa_wait_finish(ofsAddr);
- spin_unlock_irqrestore(&moxafunc_lock);
+ spin_unlock_irq(&moxafunc_lock);

}
return baud;

2009-11-18 14:33:03

by Alan

[permalink] [raw]
Subject: [PATCH 09/12] moxa: Kill off the throttle method

The tty flag can be tested so the shadow flag isn't needed

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 21 +--------------------
1 files changed, 1 insertions(+), 20 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index e6a79b0..b640409 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -154,7 +154,6 @@ struct mon_str {
#define TXSTOPPED 1
#define LOWWAIT 2
#define EMPTYWAIT 3
-#define THROTTLE 4

#define SERIAL_DO_RESTART

@@ -194,8 +193,6 @@ static int moxa_write(struct tty_struct *, const unsigned char *, int);
static int moxa_write_room(struct tty_struct *);
static void moxa_flush_buffer(struct tty_struct *);
static int moxa_chars_in_buffer(struct tty_struct *);
-static void moxa_throttle(struct tty_struct *);
-static void moxa_unthrottle(struct tty_struct *);
static void moxa_set_termios(struct tty_struct *, struct ktermios *);
static void moxa_stop(struct tty_struct *);
static void moxa_start(struct tty_struct *);
@@ -415,8 +412,6 @@ static const struct tty_operations moxa_ops = {
.flush_buffer = moxa_flush_buffer,
.chars_in_buffer = moxa_chars_in_buffer,
.ioctl = moxa_ioctl,
- .throttle = moxa_throttle,
- .unthrottle = moxa_unthrottle,
.set_termios = moxa_set_termios,
.stop = moxa_stop,
.start = moxa_start,
@@ -1327,20 +1322,6 @@ static int moxa_tiocmset(struct tty_struct *tty, struct file *file,
return 0;
}

-static void moxa_throttle(struct tty_struct *tty)
-{
- struct moxa_port *ch = tty->driver_data;
-
- set_bit(THROTTLE, &ch->statusflags);
-}
-
-static void moxa_unthrottle(struct tty_struct *tty)
-{
- struct moxa_port *ch = tty->driver_data;
-
- clear_bit(THROTTLE, &ch->statusflags);
-}
-
static void moxa_set_termios(struct tty_struct *tty,
struct ktermios *old_termios)
{
@@ -1418,7 +1399,7 @@ static int moxa_poll_port(struct moxa_port *p, unsigned int handle,
tty_wakeup(tty);
}

- if (inited && !test_bit(THROTTLE, &p->statusflags) &&
+ if (inited && !test_bit(TTY_THROTTLED, &tty->flags) &&
MoxaPortRxQueue(p) > 0) { /* RX */
MoxaPortReadData(p);
tty_schedule_flip(tty);

2009-11-18 14:33:16

by Alan

[permalink] [raw]
Subject: [PATCH 10/12] moxa: Fix modem op locking

This is overkill and mostly not needed

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 25 ++++++++++---------------
1 files changed, 10 insertions(+), 15 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index b640409..f9816e6 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -139,7 +139,7 @@ struct moxa_port {
int cflag;
unsigned long statusflags;

- u8 DCDState;
+ u8 DCDState; /* Protected by the port lock */
u8 lineCtrl;
u8 lowChkFlag;
};
@@ -1141,9 +1141,9 @@ static int moxa_carrier_raised(struct tty_port *port)
struct moxa_port *ch = container_of(port, struct moxa_port, port);
int dcd;

- spin_lock_bh(&moxa_lock);
+ spin_lock_irq(&port->lock);
dcd = ch->DCDState;
- spin_unlock_bh(&moxa_lock);
+ spin_unlock_irq(&port->lock);
return dcd;
}

@@ -1267,16 +1267,9 @@ static int moxa_chars_in_buffer(struct tty_struct *tty)

static int moxa_tiocmget(struct tty_struct *tty, struct file *file)
{
- struct moxa_port *ch;
+ struct moxa_port *ch = tty->driver_data;
int flag = 0, dtr, rts;

- mutex_lock(&moxa_openlock);
- ch = tty->driver_data;
- if (!ch) {
- mutex_unlock(&moxa_openlock);
- return -EINVAL;
- }
-
MoxaPortGetLineOut(ch, &dtr, &rts);
if (dtr)
flag |= TIOCM_DTR;
@@ -1289,7 +1282,6 @@ static int moxa_tiocmget(struct tty_struct *tty, struct file *file)
flag |= TIOCM_DSR;
if (dtr & 4)
flag |= TIOCM_CD;
- mutex_unlock(&moxa_openlock);
return flag;
}

@@ -1368,15 +1360,20 @@ static void moxa_hangup(struct tty_struct *tty)
static void moxa_new_dcdstate(struct moxa_port *p, u8 dcd)
{
struct tty_struct *tty;
+ unsigned long flags;
dcd = !!dcd;

+ spin_lock_irqsave(&p->port.lock, flags);
if (dcd != p->DCDState) {
+ p->DCDState = dcd;
+ spin_unlock_irqrestore(&p->port.lock, flags);
tty = tty_port_tty_get(&p->port);
if (tty && C_CLOCAL(tty) && !dcd)
tty_hangup(tty);
tty_kref_put(tty);
}
- p->DCDState = dcd;
+ else
+ spin_unlock_irqrestore(&p->port.lock, flags);
}

static int moxa_poll_port(struct moxa_port *p, unsigned int handle,
@@ -1878,9 +1875,7 @@ static int MoxaPortLineStatus(struct moxa_port *port)
val &= 0x0B;
if (val & 8)
val |= 4;
- spin_lock_bh(&moxa_lock);
moxa_new_dcdstate(port, val & 8);
- spin_unlock_bh(&moxa_lock);
val &= 7;
return val;
}

2009-11-18 14:33:23

by Alan

[permalink] [raw]
Subject: [PATCH 11/12] moxa: Kill the use of lock_kernel

It isn't needed here any more

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 16 ++--------------
1 files changed, 2 insertions(+), 14 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index f9816e6..f8e673a 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -34,7 +34,6 @@
#include <linux/tty.h>
#include <linux/tty_flip.h>
#include <linux/major.h>
-#include <linux/smp_lock.h>
#include <linux/string.h>
#include <linux/fcntl.h>
#include <linux/ptrace.h>
@@ -202,7 +201,6 @@ static int moxa_tiocmset(struct tty_struct *tty, struct file *file,
unsigned int set, unsigned int clear);
static void moxa_poll(unsigned long);
static void moxa_set_tty_param(struct tty_struct *, struct ktermios *);
-static void moxa_setup_empty_event(struct tty_struct *);
static void moxa_shutdown(struct tty_port *);
static int moxa_carrier_raised(struct tty_port *);
static void moxa_dtr_rts(struct tty_port *, int);
@@ -1251,17 +1249,13 @@ static int moxa_chars_in_buffer(struct tty_struct *tty)
struct moxa_port *ch = tty->driver_data;
int chars;

- lock_kernel();
chars = MoxaPortTxQueue(ch);
- if (chars) {
+ if (chars)
/*
* Make it possible to wakeup anything waiting for output
* in tty_ioctl.c, etc.
*/
- if (!test_bit(EMPTYWAIT, &ch->statusflags))
- moxa_setup_empty_event(tty);
- }
- unlock_kernel();
+ set_bit(EMPTYWAIT, &ch->statusflags);
return chars;
}

@@ -1503,12 +1497,6 @@ static void moxa_set_tty_param(struct tty_struct *tty, struct ktermios *old_term
tty_encode_baud_rate(tty, baud, baud);
}

-static void moxa_setup_empty_event(struct tty_struct *tty)
-{
- struct moxa_port *ch = tty->driver_data;
- set_bit(EMPTYWAIT, &ch->statusflags);
-}
-
/*****************************************************************************
* Driver level functions: *
*****************************************************************************/

2009-11-18 14:33:38

by Alan

[permalink] [raw]
Subject: [PATCH 12/12] moxa: split open lock

moxa_openlock is used for several situations where we want to handle the
case of an ioctl that crosses many ports (not just the open tty), and also
cases where an open races a deinit (eg a pci unplug) and we hangup a port
before we can cope with that.

The non open race cases can use the moxa_lock spinlock. This simplifies sorting
out the remaining mess.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/char/moxa.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)


diff --git a/drivers/char/moxa.c b/drivers/char/moxa.c
index f8e673a..16b8aed 100644
--- a/drivers/char/moxa.c
+++ b/drivers/char/moxa.c
@@ -163,6 +163,7 @@ static struct mon_str moxaLog;
static unsigned int moxaFuncTout = HZ / 2;
static unsigned int moxaLowWaterChk;
static DEFINE_MUTEX(moxa_openlock);
+static DEFINE_SPINLOCK(moxa_lock);
/* Variables for insmod */
#ifdef MODULE
static unsigned long baseaddr[MAX_BOARDS];
@@ -313,22 +314,20 @@ static int moxa_ioctl(struct tty_struct *tty, struct file *file,
struct moxa_port *p;
unsigned int i, j;

- mutex_lock(&moxa_openlock);
for (i = 0; i < MAX_BOARDS; i++) {
p = moxa_boards[i].ports;
for (j = 0; j < MAX_PORTS_PER_BOARD; j++, p++, argm++) {
memset(&tmp, 0, sizeof(tmp));
+ spin_lock_bh(&moxa_lock);
if (moxa_boards[i].ready) {
tmp.inq = MoxaPortRxQueue(p);
tmp.outq = MoxaPortTxQueue(p);
}
- if (copy_to_user(argm, &tmp, sizeof(tmp))) {
- mutex_unlock(&moxa_openlock);
+ spin_unlock_bh(&moxa_lock);
+ if (copy_to_user(argm, &tmp, sizeof(tmp)))
return -EFAULT;
- }
}
}
- mutex_unlock(&moxa_openlock);
break;
} case MOXA_GET_OQUEUE:
status = MoxaPortTxQueue(ch);
@@ -344,16 +343,20 @@ static int moxa_ioctl(struct tty_struct *tty, struct file *file,
struct moxa_port *p;
unsigned int i, j;

- mutex_lock(&moxa_openlock);
for (i = 0; i < MAX_BOARDS; i++) {
p = moxa_boards[i].ports;
for (j = 0; j < MAX_PORTS_PER_BOARD; j++, p++, argm++) {
struct tty_struct *ttyp;
memset(&tmp, 0, sizeof(tmp));
- if (!moxa_boards[i].ready)
+ spin_lock_bh(&moxa_lock);
+ if (!moxa_boards[i].ready) {
+ spin_unlock_bh(&moxa_lock);
goto copy;
+ }

status = MoxaPortLineStatus(p);
+ spin_unlock_bh(&moxa_lock);
+
if (status & 1)
tmp.cts = 1;
if (status & 2)
@@ -368,13 +371,10 @@ static int moxa_ioctl(struct tty_struct *tty, struct file *file,
tmp.cflag = ttyp->termios->c_cflag;
tty_kref_put(tty);
copy:
- if (copy_to_user(argm, &tmp, sizeof(tmp))) {
- mutex_unlock(&moxa_openlock);
+ if (copy_to_user(argm, &tmp, sizeof(tmp)))
return -EFAULT;
- }
}
}
- mutex_unlock(&moxa_openlock);
break;
}
case TIOCGSERIAL:
@@ -427,7 +427,6 @@ static const struct tty_port_operations moxa_port_ops = {

static struct tty_driver *moxaDriver;
static DEFINE_TIMER(moxaTimer, moxa_poll, 0, 0);
-static DEFINE_SPINLOCK(moxa_lock);

/*
* HW init

2009-11-20 12:17:05

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 04/12] mxser: Use the new locking rules to fix setserial properly

On Wed, Nov 18, 2009 at 02:15:23PM +0000, Alan Cox wrote:
> Propogate the init/shutdown mutex through the setserial logic. Use the proper
> locks for the various bits still using the BKL. Kill the BKL in this driver.
>

> drivers/char/mxser.c | 145 +++++++++++++++++++++++++++-----------------------

> @@ -1951,6 +1959,7 @@ static void mxser_wait_until_sent(struct tty_struct *tty, int timeout)

> + spin_lock_irqsave(&info->slock, flags);
> while (!((lsr = inb(info->ioaddr + UART_LSR)) & UART_LSR_TEMT)) {
> #ifdef SERIAL_DEBUG_RS_WAIT_UNTIL_SENT
> printk("lsr = %d (jiff=%lu)...", lsr, jiffies);
> #endif
> + spin_unlock_irqrestore(&info->slock, flags);
> schedule_timeout_interruptible(char_time);
> if (signal_pending(current))
> break;
> if (timeout && time_after(jiffies, orig_jiffies + timeout))
> break;

If we break out of this while loop we get a double unlock.

> + spin_lock_irqsave(&info->slock, flags);
> }
> + spin_unlock_irqrestore(&info->slock, flags);
> set_current_state(TASK_RUNNING);
> - unlock_kernel();

regards,
dan carpenter