Serial 8250: clear the lsr_break_flag at open

Serial 8250: clear the lsr_break_flag at open

[Corey Minyard]

Sorry for the double send, but I messed up on the subject.  I'll
get used to mutt one of these days.

I think I've spotted a bug in the 8250 code, but I'm not really
sure.  I'm having a hard time understanding why the lsr_break_flag
is necessary.

Subject: Serial 8250: clear the lsr_break_flag at open

The lsr_break_flag in the 8250 driver is not cleared when the port is
opened.  This means that on a serial console, if a break has occurred
while the port is closed, the first call to receive_chars() will
result in a break being delivered at that point.  Clear the flag at
open to fix the problem.

Signed-off-by: Corey Minyard <minyard@acm.org>

Index: linux-2.6.21/drivers/serial/8250.c
===================================================================
--- linux-2.6.21.orig/drivers/serial/8250.c
+++ linux-2.6.21/drivers/serial/8250.c
@@ -1638,6 +1638,7 @@ static int serial8250_startup(struct uar
 
 	up->capabilities = uart_config[up->port.type].flags;
 	up->mcr = 0;
+	up->lsr_break_flag = 0;
 
 	if (up->port.type == PORT_16C950) {
 		/* Wake up and initialize UART */

Re: Serial 8250: clear the lsr_break_flag at open

[Russell King]

On Mon, Apr 30, 2007 at 05:08:59PM -0500, Corey Minyard wrote:
> I think I've spotted a bug in the 8250 code, but I'm not really
> sure.

Yes, clearing lsr_break_flag prior to opening the port is probably a good
thing.

> I'm having a hard time understanding why the lsr_break_flag
> is necessary.

Merely reading the LSR clears status bits.  We read the LSR repeatedly
so that we can monitor the transmit FIFO when outputting serial console
messages.

This means that if you have a busy serial console, and you want to send
it a sysrq request, there's a chance that the break flag in the LSR will
be cleared by the transmit FIFO status polling code thereby being lost.

So, we need to remember that status, and we do this via the lsr_break_flag.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Serial 8250: clear the lsr_break_flag at open

[Corey Minyard]

Russell King wrote:
> On Mon, Apr 30, 2007 at 05:08:59PM -0500, Corey Minyard wrote:
>   
>> I'm having a hard time understanding why the lsr_break_flag
>> is necessary.
>>     
>
> Merely reading the LSR clears status bits.  We read the LSR repeatedly
> so that we can monitor the transmit FIFO when outputting serial console
> messages.
>
> This means that if you have a busy serial console, and you want to send
> it a sysrq request, there's a chance that the break flag in the LSR will
> be cleared by the transmit FIFO status polling code thereby being lost.
>
> So, we need to remember that status, and we do this via the lsr_break_flag.
>   
I should have said a little more.  I couldn't find anywhere in any docs
for this that said it was a destructive read.  I've done some experiments
and that seems to be the case, though.

So two things:

There are other bits in this register that also appear to be destroyed on
read: framing, parity, and overrun.  Should those be saved, too?

There are several places where the LSR is read and nothing is done
for this, in serial8250_start_tx, serial8250_backup_timeout, and
serial8250_tx_empty.  It seems like these would need to be handled,
too.

If this is really a problem, I'd be glad to generate another patch.

-corey

Re: Serial 8250: clear the lsr_break_flag at open

[Russell King]

On Tue, May 01, 2007 at 08:23:14AM -0500, Corey Minyard wrote:
> Russell King wrote:
> >On Mon, Apr 30, 2007 at 05:08:59PM -0500, Corey Minyard wrote:
> >  
> >>I'm having a hard time understanding why the lsr_break_flag
> >>is necessary.
> >>    
> >
> >Merely reading the LSR clears status bits.  We read the LSR repeatedly
> >so that we can monitor the transmit FIFO when outputting serial console
> >messages.
> >
> >This means that if you have a busy serial console, and you want to send
> >it a sysrq request, there's a chance that the break flag in the LSR will
> >be cleared by the transmit FIFO status polling code thereby being lost.
> >
> >So, we need to remember that status, and we do this via the lsr_break_flag.
> >  
> I should have said a little more.  I couldn't find anywhere in any docs
> for this that said it was a destructive read.

The TI 16550A data sheet says:

* Bit 1: This bit is the overrun error (OE) indicator. ... The OE indicator
  is cleared every time the CPU reads the contents of the LSR.

* Bit 2.: This bit is the parity error (PE) indicator. ... The PE bit is
  cleared every time the CPU reads the contents of the LSR.

* Bit 3: This bit is the framing error (FE) indicator. ... The FE bit is
  cleared every time the CPU reads the contents of the LSR.

* Bit 4: This bit is the break interrupt (BI) indicator. ... The BI bit is
  cleared every time the CPU reads the contents of the LSR.

> So two things:
> 
> There are other bits in this register that also appear to be destroyed on
> read: framing, parity, and overrun.  Should those be saved, too?

Yes.

> There are several places where the LSR is read and nothing is done
> for this, in serial8250_start_tx, serial8250_backup_timeout, and
> serial8250_tx_empty.  It seems like these would need to be handled,
> too.

The backup code is something I never properly reviewed, so no comments
there.  The tx_empty code I assumed would be a relatively rare event,
except when closing the port (at which point you don't particularly care
about errors anyway, not even the break flag since chances are you'll
miss the following character.)

Given that people might want to poll it for various reasons, I guess
saving the status away should be done.  However, there's a slight issue
with working out which character the error is associated with.  Careful
locking may be the answer to that though.

As for start_tx, yes, though slightly harder to check.  Maybe the code
should be modified to reduce the number of potential LSR reads by reading
the IIR first, and only if that shows no interrupt pending should the LSR
be read (and the error flags remembered.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Serial 8250: clear the lsr_break_flag at open

[Corey Minyard]

Russell King wrote:
>
> The backup code is something I never properly reviewed, so no comments
> there.  The tx_empty code I assumed would be a relatively rare event,
> except when closing the port (at which point you don't particularly care
> about errors anyway, not even the break flag since chances are you'll
> miss the following character.)
>   
That "if" statement in the backup code does look a little dodgy, more
than is perhaps required.  I think it's correct, but I need to add
a lock there in my patch to protect the LSR check.
> Given that people might want to poll it for various reasons, I guess
> saving the status away should be done.  However, there's a slight issue
> with working out which character the error is associated with.  Careful
> locking may be the answer to that though.
>   
I think as long as you hold the port lock while you grab the LSR and
set the saved flags it will work.
> As for start_tx, yes, though slightly harder to check.  Maybe the code
> should be modified to reduce the number of potential LSR reads by reading
> the IIR first, and only if that shows no interrupt pending should the LSR
> be read (and the error flags remembered.)
>   
The version of start_tx in 2.6.21 does check IIR first, and it only
checks the LSR if UART_BUG_TXEN is set, so I assume that's not
a big deal.

I'll sleep on it tonight, look it over tomorrow morning, and resend the
patch.

Thanks,

-corey