Date: Tue, 16 Feb 2016 09:28:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Michal Meloun <mmel@freebsd.org> Cc: src-committers@freebsd.org, Marius Strobl <marius@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r295557 - head/sys/dev/uart Message-ID: <20160216080249.F1233@besplex.bde.org> In-Reply-To: <56C1BDE2.8090300@freebsd.org> References: <201602120514.u1C5EwWt053622@repo.freebsd.org> <20160212164755.GC4980@alchemy.franken.de> <20160213041246.V1974@besplex.bde.org> <20160213005801.GF15359@alchemy.franken.de> <56C1BDE2.8090300@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 15 Feb 2016, Michal Meloun wrote: > 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 varyin= g >>>> 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 >>> ... >>>> 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. I haven't seen any with this bug. But I haven't seen many newer ones or any in workstations or servers since I only use consumer-grade motherboards -- maybe those are better :-). Why would a new ASIC reimplement bugs that were in the original 8250? (IIRC, there was an 8250 with lots of bugs and limited production, and an 8250A that was better. FreeBSD is much newer than the 8250A so it might never have been used on an 8250.) >>> 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 unecessar= y >>> 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 call= ed >>> 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 agai= n. >>> 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 characte= r >>> 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. Certainly that is a concern if there are chips with the bug. > No, I=92m 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). Nah, compatible means only having bugs that are compatible, and a noremal xx50 doesn't have this bug :-). Strict compatibility with the original 8250 would give lots of bugs but it is unlikely that anything new is compatible with that. Driver bugs are still more likely. This reminds me that the most likely source of them is edge triggered ISA interrupts and drivers not doing enough in the interrupt handler to ensure getting a new interrupt. I think you remember correctly that bad priority was one of the bugs in the original 8250. > 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. It is blocked by uart_lock(), right? > 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. It is not empty even on x86, although it probably should be. BTW, if arm needs the barrier, then how does it work with bus_space_barrier() referenced in just 25 files in all of /sys/dev? On x86, bus_space_barrier is a dummy locked memory increment by 0. Similar code for atomic ops was recently changed to null in all cases except for atomic_thread_fence_seq_cst() where it was optimized to use a better memory address. atomic_thread_fence_seq_cst() is not used by any other atomic primitive; it is used just twice in all of /sys (in sched_ule.c). Ordinary load/store on x86 is automatically strongly ordered. It is unclear what happens for memory-mapped and i/o-mapped devices. I think on x86, accesses to i/o-mapped devices are ordered even more strongly than for memory (and with respect to memory). The reason for existence of memory-mapped devices is defeated if their memory doesn't actually look like memory. I think accesses to them are ordered strongly on x86 too. However, something even stronger than atomic_thread_fence_seq_cst() might be needed to get an order related to physical events -- something to flush write buffers. i/o is slow enough even without the barriers, an on modern x86 the time taken by the useless barrier is on the noise compared with the time taken by the i/o. E.g., on a 4+GHz Haswell system, for this write loop in ns8250_bus_transmit(): X =09for (i =3D 0; i < sc->sc_txdatasz; i++) { X =09=09uart_setreg(bas, REG_DATA, sc->sc_txbuf[i]); X =09=09uart_barrier(bas); X =09} - uart_setreg() takes 1.5+ usec (6000+ cycles) for an ISA or PCI i/o-mapped bus write (plus a few more for software overheads) - uart_barrier() takes about another 20 cycles Why does uart use a loop with barrier after every i/o? bus_space_write_multi_N() doesn't use any barriers internally on x86. I'm not sure what it does on arm. If a barrier is needed after every i/o, then none of these 'multi' i/o functions can work in general. My version of sio uses bus_space_write_multi_1() here. On the same Haswell system, this takes the same 1.5+ usec per byte if i/o-mapped,=20 but for a PCI bus device it takes only 150 nsec per byte when memory mapped -- 10+ times faster than for uart. Unfortunately, PCI memory- mapped writes are the only faster case on the Haswell system. Older systems are much faster. The same serial card on a 16-year old 367 MHz system with the PCI bus overclocked 20% can do the fast case in 125 nsec and the slow cases in 400 nsec. 125 nsec is only 45 cycles at 367 MHz. uart's pessimizations work much better when the CPU is slower -- now the ~20 extra cycles and a few more for other overheads is about the same as the 45 cycles for the i/o, instead of in the noise. > 8 - HW: character from THR is transferred to shift register and UART > signals TX empty interrupt again. > 9 - Goto 3. This seems to be working as intended. > 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=92t want to remove it. It is indeed a valuable indicator. I think it just detected that the intended behaviour is a pessimizations. On fast systems, the overhead for extra interrupts is also in the noise. No one notices that interrupt overheads are a couple of thousands of cycles more than they should be when a single i/o takes 6000+ cycles. > Also, at this time, UART driver is last one known to generate spurious > interrupts in ARM world. >=20 > So, what=92s now? I can #ifdef __arm__ change made in r295557 (for maximu= m > safety), if you want this. Or we can just wait to see if someone reports > a problem ... Use better methods. Perhaps the detection of "extra" interrupts is not really good. Systems with shared interrupts can't even tell when they get "extra" interrupts. At best their interrupt handlers poll the device status registers as efficiently as possible. But when reading a single device register takes 6000+ cycles, "as efficiently as possible" is not very efficient. Bruce From owner-svn-src-head@freebsd.org Mon Feb 15 23:10:14 2016 Return-Path: <owner-svn-src-head@freebsd.org> Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CA8C1AA9226; Mon, 15 Feb 2016 23:10:14 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-io0-x22e.google.com (mail-io0-x22e.google.com [IPv6:2607:f8b0:4001:c06::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 928631597; Mon, 15 Feb 2016 23:10:14 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: by mail-io0-x22e.google.com with SMTP id z135so103887342iof.0; Mon, 15 Feb 2016 15:10:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=Hz0oK6BF9XkI+dyBVROfjr7WojAxngzrPWvzOXfPlmM=; b=vSuvWXGd8UM9kTBfmBqSfBypmU4Ll4xF2ZsVLKggLkpjdLrXzUgemggngijRGxHodN 9fEdG1ijQFxUFeBQpthwTF8DBqqD1LM2y5KYy3n3WGQCaL/9UIbo57Ug22xru4hPW8v0 H3bKmbjkPpBtjgH2m33bw0n17b/SskEywKCT8EguHTUoOvDYlYlHpMa0aFb3VOyKLB3r /nJmdXfUyl3RA9ZqL9bkNfBPxPGTcMSa9MNts9pum34HCpd2bVZRVBcmR4do5cjT85xV C9jgfA6sQ2/qZGw1FiiOwDp1V8ZZ9IbZdIQhwvJogA/0e+5kg96n0jvG23XUa/jodNqE r0aA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=Hz0oK6BF9XkI+dyBVROfjr7WojAxngzrPWvzOXfPlmM=; b=ZFMIOyes5SkNqOk2aUDMgyeUMB5q7Pi+SZZrqhsBJD73CENfG1Sn/hspmHU7Yrhhga sURPJ5sCqriBeGwFvJBLKhslTzHcT1ZuOex4lJ22qfKDkIexbyYN44oJ4fJCtdIaUBEh cUJMJQwiBf7fOv9+WtM3hkvkBbjSR/DP0uQ2xFvyd/dhgrz8YYNMLQDvHAC7OTmd85UF K86U1OXi/pJjgxuwgdAZxFzHhGfSS6ttz2cxQFSrSPylqby2Om9xso62SA8bvxdrFFPd MO1DvVsxKdR6DvslLUXMYRipfXy75M0NzwWx8GMefY8CN/VMvgCi5864DySev8kV3JHx 0lLQ== X-Gm-Message-State: AG10YOSraJv94/CjE+gfkfCOAeEjQu4i0T8VEuTkCLghRn3BoSRIauE9Ob4w01o9ftIu6rg24u9aZ4p7EB90eg== MIME-Version: 1.0 X-Received: by 10.107.162.144 with SMTP id l138mr7557511ioe.123.1455577814032; Mon, 15 Feb 2016 15:10:14 -0800 (PST) Received: by 10.36.14.19 with HTTP; Mon, 15 Feb 2016 15:10:13 -0800 (PST) In-Reply-To: <20160216080249.F1233@besplex.bde.org> References: <201602120514.u1C5EwWt053622@repo.freebsd.org> <20160212164755.GC4980@alchemy.franken.de> <20160213041246.V1974@besplex.bde.org> <20160213005801.GF15359@alchemy.franken.de> <56C1BDE2.8090300@freebsd.org> <20160216080249.F1233@besplex.bde.org> Date: Mon, 15 Feb 2016 15:10:13 -0800 Message-ID: <CAJ-Vmon80HwrPcZ54-ZAZ4xf311Cw6H8QKeXTK18GC6ukMGsvw@mail.gmail.com> Subject: Re: svn commit: r295557 - head/sys/dev/uart From: Adrian Chadd <adrian.chadd@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Michal Meloun <mmel@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Marius Strobl <marius@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Content-Type: text/plain; charset=UTF-8 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current <svn-src-head.freebsd.org> List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>; List-Post: <mailto:svn-src-head@freebsd.org> List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help> List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=subscribe> X-List-Received-Date: Mon, 15 Feb 2016 23:10:14 -0000 On 15 February 2016 at 14:28, Bruce Evans <brde@optusnet.com.au> wrote: > > BTW, if arm needs the barrier, then how does it work with > bus_space_barrier() referenced in just 25 files in all of /sys/dev? likely the same reason it does for mips - everything's cache coherent on hardware that has pci/pcie slots, or people just didn't notice. For ppc they put an implicit barrier into the bus calls because too many drivers don't do barriers (but USB does, yay) and fixing it all was likely a terrible task. I'm doing it for the MIPS related pieces that need it, but it's slow going. -adrian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160216080249.F1233>