Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursivespinlocks

[Darren Hart]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/