svn commit: r295557 - head/sys/dev/uart

[Marius Strobl]

On Sat, Feb 13, 2016 at 06:53:25AM +1100, Bruce Evans wrote:
> On Fri, 12 Feb 2016, Marius Strobl wrote:
> 
> > On Fri, Feb 12, 2016 at 05:14:58AM +0000, Michal Meloun wrote:
> >> Author: mmel
> >> Date: Fri Feb 12 05:14:58 2016
> >> New Revision: 295557
> >> URL: https://svnweb.freebsd.org/changeset/base/295557
> >>
> >> Log:
> >>   UART: Fix spurious interrupts generated by ns8250 and lpc drivers:
> >>    - don't enable transmitter empty interrupt before filling TX FIFO.
> >
> > Are you sure this doesn't create a race that leads to lost TX ready
> > interrupts? For a single character, the TX FIFO very well may be empty
> > again at the point in time IER_ETXRDY is enabled now.  With the varying
> > behavior of 8250/16x50 chips - some of which is documented in sio(4) -
> 
> That is mostly FUD.  More likely driver bugs than chip bugs.
> 
> A non-broken xx50 interrupts after you (re)enable tx interrupts, iff
> the fifo is already empty.  This gives a "spurious" interrupt.  But
> perhaps depending on this is too fragile.  Normal operation is to keep
> the tx interrupt enabled and depend on writing to the fifo causing a
> tx interrupt later.  But it is a more common chip bug for tx interrupts
> later to not go away when they should (normally by reading the IIR),
> so some drivers toggle the tx interrupt enable dynamically.
> 
> An example of a driver bug is only enabling tx interrupts for this.
> It takes a transition of the interrupt enable bit from off to on to
> get the interrupt.  Other driver bugs may result in a missing transition
> because the bit was supposed to be off but is actually on.
> 
> > I'd expect there are many that no longer generate a TX ready at all
> > with this change in place. In this case, receiving spurious interrupts
> > (which ones? IIR_NOPEND? IIR_TXRDY?) with some devices appears to be
> > the lesser evil.
> 
> Not many.  Only broken ones.

In my experience many xx50 are broken, especially the integrated
on-board ones you still have in workstations and servers today.

> The "spurious" interrupts are just normal
> ones from bon-broken chips:
> 
> - uart first does a potentially-unbounded busy-wait before the doing
>    anything to ensure that the fifo is empty.  This should be unecessary
>    since this function should not be called unless sc_txbusy is 0 and
>    sc_txbusy should be nonzero if the fifo is not empty.  If it is called
>    when the fifo is not emptu, then the worst-case busy-wait is approx.
>    640 seconds for a 128-byte fifo at 2 bps. The 'broken_txfifo case'
>    busy-waits for a long time in normal operation.
> - enabling the tx interrupt causes one immediately on non-broken uarts
> - the interrupt handler is normally called immediately.  Then it always
>    blocks on uart_lock()
> - then the main code fills the fifo and unlocks
> - then the interrupt handler runs.  It normally finds that the fifo is
>    not empty (since it has just been filled) and does nothing
> - another tx interrupt occurs later and the interrupt handler runs again.
>    It normally doesn't hit the lock again, and normally finds the fifo
>    empty, so it does something.

You correctly describe what happens at r295556 with a non-broken xx50.
That revision causes a spurious interrupt with non-broken xx50 but
also ensures that the relevant TX interrupt isn't missed with broken
xx50 that do not issue an interrupt when enabling IER_ETXRDY. Besides,
as you say, the general approach of dynamically enabling TX interrupts
works around the common brokenness of these interrupts no longer going
away when they should.

> But you are probably correct that a 1-byte write to the fifo often
> loses the race.  This depends on how fast the hardware moves the byte
> from the fifo to the tx register.  Actually, since we didn't wait
> for the tx register to become empty, it will often take a full character
> time before the move.  After that, I think it might take 1 bit time but
> no more.

My concern is that with r295557, when this race is lost no TX interrupt
is seen at all with broken xx50 that do not trigger an interrupt when
enabling IER_ETXRDY.

Marius