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

[Michal Meloun]

Dne 13.02.2016 v 1:58 Marius Strobl napsal(a):
> 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
> 

No, I’m not sure, nobody can be sure if we talking about ns8250
compatible device(s). Also, all UARTs known to me, generates an
interrupt on TX unmasking (assuming level sensitive interrupt).
Only IIR can reports bad priority so some very old 8250 (if
memory still serve me).

I only found following scenario on multiple ARM SOCs.

Please note that ARM architecture does not have vectored interrupts,
CPU must read actual interrupt source from external interrupt
controller (GIC) register. This register contain predefined value if
none of interrupts are active.

1 - CPU1: enters ns8250_bus_transmit() and sets IER_ETXRDY.
2 - HW: UART interrupt is asserted, processed by GIC and signaled
    to CPU2.
3 - CPU2: enters interrupt service.
4 - CPU1: writes character to into REG_DATA register.
5 - HW: UART clear its interrupt request
6 - CPU2: reads interrupt source register. No active interrupt is
    found, spurious interrupt is signaled, and CPU leaves interrupted
    state.
7 - CPU1: executes uart_barrier(). This function is not empty on ARM,
    and can be slow in some cases.
8 - HW: character from THR is transferred to shift register and UART
    signals TX empty interrupt again.
9 - Goto 3.

Currently, GIC interrupts service routine (see [1]) reports spurious
interrupt issue (interrupt requests disappears itself, without any HW
action). This is very valuable indicator of driver problem for us (note,
ARM needs special synchronization for related inter-device writes, see
[2]), and I don’t want to remove it.

Also, at this time, UART driver is last one known to generate spurious
interrupts in ARM world.

So, what’s now? I can #ifdef __arm__ change made in r295557 (for maximum
safety), if you want this. Or we can just wait to see if someone reports
a problem ...

Michal

[1]
https://svnweb.freebsd.org/base/head/sys/arm/arm/gic.c?revision=294422&view=markup#l538
[2] https://reviews.freebsd.org/D4240