On 06/01/2012 11:36 AM, Darren Hart wrote: > > > 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? Are there still concerns about the additional lock? I'll resend V2 tomorrow with the single whitespace fix if I don't hear anything back today. Thanks, Darren > >> >> >>> @@ -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
