linux-kernel - Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive spinlocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4FC90BAD.3080606@linux.intel.com>
Date:	Fri, 01 Jun 2012 11:36:29 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Feng Tang <feng.tang@...el.com>,
	Alexander Stein <alexander.stein@...tec-electronic.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alan Cox <alan@...ux.intel.com>, linux-serial@...r.kernel.org
Subject: Re: [RFC PATCH] pch_uart: Add eg20t_port lock field, avoid recursive
 spinlocks



On 06/01/2012 01:30 AM, Tomoya MORINAGA wrote:
> On Thu, May 31, 2012 at 5:54 PM, Darren Hart <dvhart@...ux.intel.com> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/