Removed save_flags()/cli()/sti() and used (light weight) spin locks
Signed-off-by: Amol Lad <[email protected]>
---
--- linux-2.6.19-rc1-orig/drivers/char/riscom8.c 2006-10-05 14:00:43.000000000 +0530
+++ linux-2.6.19-rc1/drivers/char/riscom8.c 2006-10-13 17:06:44.000000000 +0530
@@ -81,6 +81,7 @@
static struct riscom_board * IRQ_to_board[16];
static struct tty_driver *riscom_driver;
+spinlock_t riscom_lock = SPIN_LOCK_UNLOCKED;
static unsigned long baud_table[] = {
0, 50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800, 2400, 4800,
@@ -231,13 +232,13 @@ static void __init rc_init_CD180(struct
{
unsigned long flags;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_out(bp, RC_CTOUT, 0); /* Clear timeout */
rc_wait_CCR(bp); /* Wait for CCR ready */
rc_out(bp, CD180_CCR, CCR_HARDRESET); /* Reset CD180 chip */
- sti();
+ spin_unlock_irq(&riscom_lock);
rc_long_delay(HZ/20); /* Delay 0.05 sec */
- cli();
+ spin_lock_irq(&riscom_lock);
rc_out(bp, CD180_GIVR, RC_ID); /* Set ID for this chip */
rc_out(bp, CD180_GICR, 0); /* Clear all bits */
rc_out(bp, CD180_PILR1, RC_ACK_MINT); /* Prio for modem intr */
@@ -248,7 +249,7 @@ static void __init rc_init_CD180(struct
rc_out(bp, CD180_PPRH, (RC_OSCFREQ/(1000000/RISCOM_TPS)) >> 8);
rc_out(bp, CD180_PPRL, (RC_OSCFREQ/(1000000/RISCOM_TPS)) & 0xff);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
/* Main probing routine, also sets irq. */
@@ -832,7 +833,7 @@ static int rc_setup_port(struct riscom_b
port->xmit_buf = (unsigned char *) tmp;
}
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->tty)
clear_bit(TTY_IO_ERROR, &port->tty->flags);
@@ -844,7 +845,7 @@ static int rc_setup_port(struct riscom_b
rc_change_speed(bp, port);
port->flags |= ASYNC_INITIALIZED;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return 0;
}
@@ -955,19 +956,19 @@ static int block_til_ready(struct tty_st
*/
retval = 0;
add_wait_queue(&port->open_wait, &wait);
- cli();
+ spin_lock_irq(&riscom_lock);
if (!tty_hung_up_p(filp))
port->count--;
- sti();
+ spin_unlock_irq(&riscom_lock);
port->blocked_open++;
while (1) {
- cli();
+ spin_lock_irq(&riscom_lock);
rc_out(bp, CD180_CAR, port_No(port));
CD = rc_in(bp, CD180_MSVR) & MSVR_CD;
rc_out(bp, CD180_MSVR, MSVR_RTS);
bp->DTR &= ~(1u << port_No(port));
rc_out(bp, RC_DTR, bp->DTR);
- sti();
+ spin_unlock_irq(&riscom_lock);
set_current_state(TASK_INTERRUPTIBLE);
if (tty_hung_up_p(filp) ||
!(port->flags & ASYNC_INITIALIZED)) {
@@ -1040,7 +1041,7 @@ static void rc_close(struct tty_struct *
if (!port || rc_paranoia_check(port, tty->name, "close"))
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (tty_hung_up_p(filp))
goto out;
@@ -1107,7 +1108,8 @@ static void rc_close(struct tty_struct *
}
port->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CLOSING);
wake_up_interruptible(&port->close_wait);
-out: restore_flags(flags);
+out:
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static int rc_write(struct tty_struct * tty,
@@ -1126,34 +1128,34 @@ static int rc_write(struct tty_struct *
if (!tty || !port->xmit_buf)
return 0;
- save_flags(flags);
+ local_save_flags(flags);
while (1) {
- cli();
+ spin_lock_irq(&riscom_lock);
c = min_t(int, count, min(SERIAL_XMIT_SIZE - port->xmit_cnt - 1,
SERIAL_XMIT_SIZE - port->xmit_head));
if (c <= 0) {
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
break;
}
memcpy(port->xmit_buf + port->xmit_head, buf, c);
port->xmit_head = (port->xmit_head + c) & (SERIAL_XMIT_SIZE-1);
port->xmit_cnt += c;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
buf += c;
count -= c;
total += c;
}
- cli();
+ spin_lock_irq(&riscom_lock);
if (port->xmit_cnt && !tty->stopped && !tty->hw_stopped &&
!(port->IER & IER_TXRDY)) {
port->IER |= IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
}
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return total;
}
@@ -1169,7 +1171,7 @@ static void rc_put_char(struct tty_struc
if (!tty || !port->xmit_buf)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->xmit_cnt >= SERIAL_XMIT_SIZE - 1)
goto out;
@@ -1177,7 +1179,8 @@ static void rc_put_char(struct tty_struc
port->xmit_buf[port->xmit_head++] = ch;
port->xmit_head &= SERIAL_XMIT_SIZE - 1;
port->xmit_cnt++;
-out: restore_flags(flags);
+out:
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_flush_chars(struct tty_struct * tty)
@@ -1192,11 +1195,11 @@ static void rc_flush_chars(struct tty_st
!port->xmit_buf)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->IER |= IER_TXRDY;
rc_out(port_Board(port), CD180_CAR, port_No(port));
rc_out(port_Board(port), CD180_IER, port->IER);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static int rc_write_room(struct tty_struct * tty)
@@ -1231,9 +1234,9 @@ static void rc_flush_buffer(struct tty_s
if (rc_paranoia_check(port, tty->name, "rc_flush_buffer"))
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->xmit_cnt = port->xmit_head = port->xmit_tail = 0;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
wake_up_interruptible(&tty->write_wait);
tty_wakeup(tty);
@@ -1251,11 +1254,11 @@ static int rc_tiocmget(struct tty_struct
return -ENODEV;
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_out(bp, CD180_CAR, port_No(port));
status = rc_in(bp, CD180_MSVR);
result = rc_in(bp, RC_RI) & (1u << port_No(port)) ? 0 : TIOCM_RNG;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
result |= ((status & MSVR_RTS) ? TIOCM_RTS : 0)
| ((status & MSVR_DTR) ? TIOCM_DTR : 0)
| ((status & MSVR_CD) ? TIOCM_CAR : 0)
@@ -1276,7 +1279,7 @@ static int rc_tiocmset(struct tty_struct
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (set & TIOCM_RTS)
port->MSVR |= MSVR_RTS;
if (set & TIOCM_DTR)
@@ -1290,7 +1293,7 @@ static int rc_tiocmset(struct tty_struct
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_MSVR, port->MSVR);
rc_out(bp, RC_DTR, bp->DTR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return 0;
}
@@ -1299,7 +1302,7 @@ static inline void rc_send_break(struct
struct riscom_board *bp = port_Board(port);
unsigned long flags;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->break_length = RISCOM_TPS / HZ * length;
port->COR2 |= COR2_ETC;
port->IER |= IER_TXRDY;
@@ -1309,7 +1312,7 @@ static inline void rc_send_break(struct
rc_wait_CCR(bp);
rc_out(bp, CD180_CCR, CCR_CORCHG2);
rc_wait_CCR(bp);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static inline int rc_set_serial_info(struct riscom_port * port,
@@ -1352,9 +1355,9 @@ static inline int rc_set_serial_info(str
port->closing_wait = tmp.closing_wait;
}
if (change_speed) {
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_change_speed(bp, port);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
return 0;
}
@@ -1435,7 +1438,7 @@ static void rc_throttle(struct tty_struc
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->MSVR &= ~MSVR_RTS;
rc_out(bp, CD180_CAR, port_No(port));
if (I_IXOFF(tty)) {
@@ -1444,7 +1447,7 @@ static void rc_throttle(struct tty_struc
rc_wait_CCR(bp);
}
rc_out(bp, CD180_MSVR, port->MSVR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_unthrottle(struct tty_struct * tty)
@@ -1458,7 +1461,7 @@ static void rc_unthrottle(struct tty_str
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->MSVR |= MSVR_RTS;
rc_out(bp, CD180_CAR, port_No(port));
if (I_IXOFF(tty)) {
@@ -1467,7 +1470,7 @@ static void rc_unthrottle(struct tty_str
rc_wait_CCR(bp);
}
rc_out(bp, CD180_MSVR, port->MSVR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_stop(struct tty_struct * tty)
@@ -1481,11 +1484,11 @@ static void rc_stop(struct tty_struct *
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->IER &= ~IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_start(struct tty_struct * tty)
@@ -1499,13 +1502,13 @@ static void rc_start(struct tty_struct *
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->xmit_cnt && port->xmit_buf && !(port->IER & IER_TXRDY)) {
port->IER |= IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
}
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
/*
@@ -1557,9 +1560,9 @@ static void rc_set_termios(struct tty_st
tty->termios->c_iflag == old_termios->c_iflag)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_change_speed(port_Board(port), port);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
if ((old_termios->c_cflag & CRTSCTS) &&
!(tty->termios->c_cflag & CRTSCTS)) {
@@ -1648,11 +1651,10 @@ static void rc_release_drivers(void)
{
unsigned long flags;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&riscom_lock, flags);
tty_unregister_driver(riscom_driver);
put_tty_driver(riscom_driver);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
#ifndef MODULE
On Fri, 2006-10-13 at 17:10 +0530, Amol Lad wrote:
> Removed save_flags()/cli()/sti() and used (light weight) spin locks
Hi,
I applaud removing such legacy-should-be-dead-by-now functions; however,
I'm not entirely sure your conversion is correct. While you convert all
places that do cli/sti with locks.... you do not seem to be adding the
locks to the places that the cli/sti pairs protected against (the IRQ
handlers).... so it looks like your conversion is incomplete ;(
Greetings,
Arjan van de Ven
Removed save_flags()/cli()/sti() and used (light weight ) spin lock.
Signed-off-by: Amol Lad <[email protected]>
---
> I applaud removing such legacy-should-be-dead-by-now functions; however,
> I'm not entirely sure your conversion is correct. While you convert all
> places that do cli/sti with locks.... you do not seem to be adding the
> locks to the places that the cli/sti pairs protected against (the IRQ
> handlers).... so it looks like your conversion is incomplete ;(
It's added. I'm not able to fine-grain the lock in the interrupt
handler. After reading the code a bit I thought it's not safe for me to
fine-grain it.
---
--- linux-2.6.19-rc1-orig/drivers/char/riscom8.c 2006-10-05 14:00:43.000000000 +0530
+++ linux-2.6.19-rc1/drivers/char/riscom8.c 2006-10-13 17:58:57.000000000 +0530
@@ -81,6 +81,7 @@
static struct riscom_board * IRQ_to_board[16];
static struct tty_driver *riscom_driver;
+spinlock_t riscom_lock = SPIN_LOCK_UNLOCKED;
static unsigned long baud_table[] = {
0, 50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800, 2400, 4800,
@@ -231,13 +232,13 @@ static void __init rc_init_CD180(struct
{
unsigned long flags;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_out(bp, RC_CTOUT, 0); /* Clear timeout */
rc_wait_CCR(bp); /* Wait for CCR ready */
rc_out(bp, CD180_CCR, CCR_HARDRESET); /* Reset CD180 chip */
- sti();
+ spin_unlock_irq(&riscom_lock);
rc_long_delay(HZ/20); /* Delay 0.05 sec */
- cli();
+ spin_lock_irq(&riscom_lock);
rc_out(bp, CD180_GIVR, RC_ID); /* Set ID for this chip */
rc_out(bp, CD180_GICR, 0); /* Clear all bits */
rc_out(bp, CD180_PILR1, RC_ACK_MINT); /* Prio for modem intr */
@@ -248,7 +249,7 @@ static void __init rc_init_CD180(struct
rc_out(bp, CD180_PPRH, (RC_OSCFREQ/(1000000/RISCOM_TPS)) >> 8);
rc_out(bp, CD180_PPRL, (RC_OSCFREQ/(1000000/RISCOM_TPS)) & 0xff);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
/* Main probing routine, also sets irq. */
@@ -557,13 +558,15 @@ static irqreturn_t rc_interrupt(int irq,
struct riscom_board *bp;
unsigned long loop = 0;
int handled = 0;
+ unsigned long flags;
bp = IRQ_to_board[irq];
if (!bp || !(bp->flags & RC_BOARD_ACTIVE)) {
return IRQ_NONE;
}
-
+
+ spin_lock_irqsave(&riscom_lock, flags);
while ((++loop < 16) && ((status = ~(rc_in(bp, RC_BSR))) &
(RC_BSR_TOUT | RC_BSR_TINT |
RC_BSR_MINT | RC_BSR_RINT))) {
@@ -609,6 +612,7 @@ static irqreturn_t rc_interrupt(int irq,
rc_out(bp, CD180_EOIR, 0); /* Mark end of interrupt */
rc_out(bp, RC_CTOUT, 0); /* Clear timeout flag */
}
+ spin_unlock_irqrestore(&riscom_lock, flags);
return IRQ_RETVAL(handled);
}
@@ -832,7 +836,7 @@ static int rc_setup_port(struct riscom_b
port->xmit_buf = (unsigned char *) tmp;
}
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->tty)
clear_bit(TTY_IO_ERROR, &port->tty->flags);
@@ -844,7 +848,7 @@ static int rc_setup_port(struct riscom_b
rc_change_speed(bp, port);
port->flags |= ASYNC_INITIALIZED;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return 0;
}
@@ -955,19 +959,19 @@ static int block_til_ready(struct tty_st
*/
retval = 0;
add_wait_queue(&port->open_wait, &wait);
- cli();
+ spin_lock_irq(&riscom_lock);
if (!tty_hung_up_p(filp))
port->count--;
- sti();
+ spin_unlock_irq(&riscom_lock);
port->blocked_open++;
while (1) {
- cli();
+ spin_lock_irq(&riscom_lock);
rc_out(bp, CD180_CAR, port_No(port));
CD = rc_in(bp, CD180_MSVR) & MSVR_CD;
rc_out(bp, CD180_MSVR, MSVR_RTS);
bp->DTR &= ~(1u << port_No(port));
rc_out(bp, RC_DTR, bp->DTR);
- sti();
+ spin_unlock_irq(&riscom_lock);
set_current_state(TASK_INTERRUPTIBLE);
if (tty_hung_up_p(filp) ||
!(port->flags & ASYNC_INITIALIZED)) {
@@ -1040,7 +1044,7 @@ static void rc_close(struct tty_struct *
if (!port || rc_paranoia_check(port, tty->name, "close"))
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (tty_hung_up_p(filp))
goto out;
@@ -1107,7 +1111,8 @@ static void rc_close(struct tty_struct *
}
port->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CLOSING);
wake_up_interruptible(&port->close_wait);
-out: restore_flags(flags);
+out:
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static int rc_write(struct tty_struct * tty,
@@ -1126,34 +1131,34 @@ static int rc_write(struct tty_struct *
if (!tty || !port->xmit_buf)
return 0;
- save_flags(flags);
+ local_save_flags(flags);
while (1) {
- cli();
+ spin_lock_irq(&riscom_lock);
c = min_t(int, count, min(SERIAL_XMIT_SIZE - port->xmit_cnt - 1,
SERIAL_XMIT_SIZE - port->xmit_head));
if (c <= 0) {
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
break;
}
memcpy(port->xmit_buf + port->xmit_head, buf, c);
port->xmit_head = (port->xmit_head + c) & (SERIAL_XMIT_SIZE-1);
port->xmit_cnt += c;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
buf += c;
count -= c;
total += c;
}
- cli();
+ spin_lock_irq(&riscom_lock);
if (port->xmit_cnt && !tty->stopped && !tty->hw_stopped &&
!(port->IER & IER_TXRDY)) {
port->IER |= IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
}
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return total;
}
@@ -1169,7 +1174,7 @@ static void rc_put_char(struct tty_struc
if (!tty || !port->xmit_buf)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->xmit_cnt >= SERIAL_XMIT_SIZE - 1)
goto out;
@@ -1177,7 +1182,8 @@ static void rc_put_char(struct tty_struc
port->xmit_buf[port->xmit_head++] = ch;
port->xmit_head &= SERIAL_XMIT_SIZE - 1;
port->xmit_cnt++;
-out: restore_flags(flags);
+out:
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_flush_chars(struct tty_struct * tty)
@@ -1192,11 +1198,11 @@ static void rc_flush_chars(struct tty_st
!port->xmit_buf)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->IER |= IER_TXRDY;
rc_out(port_Board(port), CD180_CAR, port_No(port));
rc_out(port_Board(port), CD180_IER, port->IER);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static int rc_write_room(struct tty_struct * tty)
@@ -1231,9 +1237,9 @@ static void rc_flush_buffer(struct tty_s
if (rc_paranoia_check(port, tty->name, "rc_flush_buffer"))
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->xmit_cnt = port->xmit_head = port->xmit_tail = 0;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
wake_up_interruptible(&tty->write_wait);
tty_wakeup(tty);
@@ -1251,11 +1257,11 @@ static int rc_tiocmget(struct tty_struct
return -ENODEV;
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_out(bp, CD180_CAR, port_No(port));
status = rc_in(bp, CD180_MSVR);
result = rc_in(bp, RC_RI) & (1u << port_No(port)) ? 0 : TIOCM_RNG;
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
result |= ((status & MSVR_RTS) ? TIOCM_RTS : 0)
| ((status & MSVR_DTR) ? TIOCM_DTR : 0)
| ((status & MSVR_CD) ? TIOCM_CAR : 0)
@@ -1276,7 +1282,7 @@ static int rc_tiocmset(struct tty_struct
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (set & TIOCM_RTS)
port->MSVR |= MSVR_RTS;
if (set & TIOCM_DTR)
@@ -1290,7 +1296,7 @@ static int rc_tiocmset(struct tty_struct
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_MSVR, port->MSVR);
rc_out(bp, RC_DTR, bp->DTR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
return 0;
}
@@ -1299,7 +1305,7 @@ static inline void rc_send_break(struct
struct riscom_board *bp = port_Board(port);
unsigned long flags;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->break_length = RISCOM_TPS / HZ * length;
port->COR2 |= COR2_ETC;
port->IER |= IER_TXRDY;
@@ -1309,7 +1315,7 @@ static inline void rc_send_break(struct
rc_wait_CCR(bp);
rc_out(bp, CD180_CCR, CCR_CORCHG2);
rc_wait_CCR(bp);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static inline int rc_set_serial_info(struct riscom_port * port,
@@ -1352,9 +1358,9 @@ static inline int rc_set_serial_info(str
port->closing_wait = tmp.closing_wait;
}
if (change_speed) {
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_change_speed(bp, port);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
return 0;
}
@@ -1435,7 +1441,7 @@ static void rc_throttle(struct tty_struc
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->MSVR &= ~MSVR_RTS;
rc_out(bp, CD180_CAR, port_No(port));
if (I_IXOFF(tty)) {
@@ -1444,7 +1450,7 @@ static void rc_throttle(struct tty_struc
rc_wait_CCR(bp);
}
rc_out(bp, CD180_MSVR, port->MSVR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_unthrottle(struct tty_struct * tty)
@@ -1458,7 +1464,7 @@ static void rc_unthrottle(struct tty_str
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->MSVR |= MSVR_RTS;
rc_out(bp, CD180_CAR, port_No(port));
if (I_IXOFF(tty)) {
@@ -1467,7 +1473,7 @@ static void rc_unthrottle(struct tty_str
rc_wait_CCR(bp);
}
rc_out(bp, CD180_MSVR, port->MSVR);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_stop(struct tty_struct * tty)
@@ -1481,11 +1487,11 @@ static void rc_stop(struct tty_struct *
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
port->IER &= ~IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
static void rc_start(struct tty_struct * tty)
@@ -1499,13 +1505,13 @@ static void rc_start(struct tty_struct *
bp = port_Board(port);
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
if (port->xmit_cnt && port->xmit_buf && !(port->IER & IER_TXRDY)) {
port->IER |= IER_TXRDY;
rc_out(bp, CD180_CAR, port_No(port));
rc_out(bp, CD180_IER, port->IER);
}
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
/*
@@ -1557,9 +1563,9 @@ static void rc_set_termios(struct tty_st
tty->termios->c_iflag == old_termios->c_iflag)
return;
- save_flags(flags); cli();
+ spin_lock_irqsave(&riscom_lock, flags);
rc_change_speed(port_Board(port), port);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
if ((old_termios->c_cflag & CRTSCTS) &&
!(tty->termios->c_cflag & CRTSCTS)) {
@@ -1648,11 +1654,10 @@ static void rc_release_drivers(void)
{
unsigned long flags;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&riscom_lock, flags);
tty_unregister_driver(riscom_driver);
put_tty_driver(riscom_driver);
- restore_flags(flags);
+ spin_unlock_irqrestore(&riscom_lock, flags);
}
#ifndef MODULE
Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> Removed save_flags()/cli()/sti() and used (light weight) spin locks
>
Have you tested this and verified there are no recursive locking cases
in your changes ?
On Fri, 2006-10-13 at 16:00 +0100, Alan Cox wrote:
> Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> > Removed save_flags()/cli()/sti() and used (light weight) spin locks
> >
>
> Have you tested this and verified there are no recursive locking cases
> in your changes ?
I doxygend riscom8.c and used call graphs to verify there are no
recursive locks. I did a code review also
Due to lack of hardware, I was not able to do any runtime checks.
Ar Sad, 2006-10-14 am 13:09 +0530, ysgrifennodd Amol Lad:
> On Fri, 2006-10-13 at 16:00 +0100, Alan Cox wrote:
> > Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> > > Removed save_flags()/cli()/sti() and used (light weight) spin locks
> > >
> >
> > Have you tested this and verified there are no recursive locking cases
> > in your changes ?
>
> I doxygend riscom8.c and used call graphs to verify there are no
> recursive locks. I did a code review also
Thanks for the info. I'll give the code a second review (also not having
hardware) and then send you/Andrew either an Ack or corrections.
Alan
Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> Removed save_flags()/cli()/sti() and used (light weight) spin locks
>
Three things I see that look problematic
1. The irq locking isn't doing anything as the IRQ handler itself is not
taking the lock
2. If the irq handler itself dumbly locks to fix this then we get
tty_flip_buffer_push() re-entering the other code paths and deadlocking
if low latency is enabled
3. Some of the use of local_save/spin_lock_irq seems over-clever and
unneeded
Fixable but how about we just delete the file since it has been broken
for ages and nobody can be using it ?
On Sat, Oct 14, 2006 at 03:20:02PM +0100, Alan Cox wrote:
> 1. The irq locking isn't doing anything as the IRQ handler itself is not
> taking the lock
Looks like you reviewed the first version instead of the version Amol
sent after Arjan critiqued that.
> 2. If the irq handler itself dumbly locks to fix this then we get
> tty_flip_buffer_push() re-entering the other code paths and deadlocking
> if low latency is enabled
Yep, still a problem with the revised patch.
> 3. Some of the use of local_save/spin_lock_irq seems over-clever and
> unneeded
>
> Fixable but how about we just delete the file since it has been broken
> for ages and nobody can be using it ?
Only broken on SMP ...
I wouldn't mind writing a new driver (using the serial core) if someone
wants to send me one. I need a multiport serial card anyway ...
Ar Sad, 2006-10-14 am 11:49 -0600, ysgrifennodd Matthew Wilcox:
> Only broken on SMP ...
>
> I wouldn't mind writing a new driver (using the serial core) if someone
> wants to send me one. I need a multiport serial card anyway ...
You still have ISA bus ?
On Sat, Oct 14, 2006 at 07:41:20PM +0100, Alan Cox wrote:
> Ar Sad, 2006-10-14 am 11:49 -0600, ysgrifennodd Matthew Wilcox:
> > Only broken on SMP ...
> >
> > I wouldn't mind writing a new driver (using the serial core) if someone
> > wants to send me one. I need a multiport serial card anyway ...
>
> You still have ISA bus ?
Oh ... well ... not really. My only remaining ISA motherboard was a
casualty of my last move (15 months ago). I have a pair of PA-RISC
machines with EISA slots, but if this thing does DMA, I have to track
down some docs and write some code to get that working.
On Sat, 2006-10-14 at 15:20 +0100, Alan Cox wrote:
> Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> > Removed save_flags()/cli()/sti() and used (light weight) spin locks
> >
>
> Three things I see that look problematic
> 2. If the irq handler itself dumbly locks to fix this then we get
> tty_flip_buffer_push() re-entering the other code paths and deadlocking
> if low latency is enabled
The comment above tty_flip_buffer_push() says "This function must not be
called from IRQ context if tty->low_latency is set". In this case
tty_flip_buffer_push() is called from IRQ context. Do you think
tty->low_latency can still be set for this case ? or I misunderstood
your statement completely...
> 3. Some of the use of local_save/spin_lock_irq seems over-clever and
> unneeded
Let me know, I'll fix them too.
>
> Fixable but how about we just delete the file since it has been broken
> for ages and nobody can be using it ?
>
If we cannot delete the file then we must fix it. It's not good the
drivers using old and deprecated interfaces in current phase of kernel
development.