Re: [PATCH] tty: amba-pl011: Make TX optimisation conditional

[Phil Elwell <phil@raspberrypi.org>]

Hi Rogier,

On 12/07/2019 13:10, Rogier Wolff wrote:
> On Fri, Jul 12, 2019 at 12:21:05PM +0100, Dave Martin wrote:
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 89ade21..1902071 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -1307,6 +1307,13 @@ static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
>>  /* Start TX with programmed I/O only (no DMA) */
>>  static void pl011_start_tx_pio(struct uart_amba_port *uap)
>>  {
>> +	/*
>> +	 * Avoid FIFO overfills if the TX IRQ is active:
>> +	 * pl011_int() will comsume chars waiting in the xmit queue anyway.
>> +	 */
>> +	if (uap->im & UART011_TXIM)
>> +		return;
>> +
> 
> I'm no expert on PL011, have no knowledge of the current bug, but have
> programmed serial drivers in the past.
> 
> This looks "dangerous" to me.
> 
> The normal situation is that you push the first few characters into
> the FIFO with PIO and then the interrupt will trigger once the FIFO
> empties and then you can refil the FIFO until the buffer empties.
> 
> The danger in THIS fix is that you might have a race that causes those
> first few PIO-ed characters not to be put in the hardware resulting in
> the interrupt never triggering.... If you can software-trigger the
> interrupt just before the "return" here that'd be a way to fix things.

I'm also not a serial driver expert, but I think this simplified patch is safe.
The reason is that the UART011_TXIM flag is only set after the pio thread has failed
to write some data into the FIFO because it is full, which would guarantee that
an interrupt is generated once the fill level drops below the half-way mark.

> I'm ok with a reaction like "I've thought about this, it's not a
> problem, now shut up".

I don't think that reaction would be justified - these things are difficult, and having
many minds on the problem helps to avoid bugs like this.

Phil