On 06/01/2012 01:30 AM, Tomoya MORINAGA wrote: > On Thu, May 31, 2012 at 5:54 PM, Darren Hart <dvhart@xxxxxxxxxxxxxxx> wrote: >> @@ -1376,7 +1379,8 @@ static void pch_uart_set_termios(struct uart_port *port, >> >> baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk / 16); >> >> - spin_lock_irqsave(&port->lock, flags); >> + spin_lock_irqsave(&priv->lock, flags); >> + spin_lock(&port->lock); >> >> uart_update_timeout(port, termios->c_cflag, baud); >> rtn = pch_uart_hal_set_line(priv, baud, parity, bits, stb); >> @@ -1389,7 +1393,8 @@ static void pch_uart_set_termios(struct uart_port *port, >> tty_termios_encode_baud_rate(termios, baud, baud); >> >> out: >> - spin_unlock_irqrestore(&port->lock, flags); >> + spin_unlock(&port->lock); >> + spin_unlock_irqrestore(&priv->lock, flags); >> } > > Are both port->lock and priv->lock really necessary ? The priv lock protects the pch_uart_hal* calls and the io access. The port lock protects the uart_update_timeout call. I'm assuming the 8250.c driver is correct in holding the port lock before making this call and making other modifcations to the port struct. So yes, I believe both are required. The port->lock was used as the lock to protect the private data in the interrupt handler, pch_uart_interrupt. If we could avoid holding that lock across the entire function, limiting it to just around the pch_uart_hal calls (perhaps by adding it to the hal calls and adding lockless __pch_uart* calls) we could avoid the recursive lock that occurs with handle_rx. I still think a priv-lock is a good idea though, even if just to clarify what is being protected. Thoughts? > > >> @@ -1572,7 +1578,9 @@ pch_console_write(struct console *co, const char *s, unsigned int count) >> >> if (locked) >> spin_unlock(&priv->port.lock); >> + spin_unlock(&priv->lock); >> local_irq_restore(flags); >> + >> } > > Looks spare blank line. Thanks, will fix for V2 after this discussion wraps up. > > thanks. -- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
