On 06/01/2012 11:36 AM, Darren Hart wrote:

On 06/01/2012 01:30 AM, Tomoya MORINAGA wrote:

quoted

On Thu, May 31, 2012 at 5:54 PM, Darren Hart [off-list ref] wrote:

quoted

@@ -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

quoted

quoted

@@ -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.

quoted

thanks.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel