[Apologies for any duplicate mails received -- some MTAs keep choking on this. Squshing to ASCII in case that helps. [fixed typo'd address for linux-arm-kernel] [Greg, to minimise confusion while these patches are still under discussion, can you drop the following patches from tty-next: * f2ee6df serial/amba-pl011: Leave the TX IRQ alone when the UART is not open * 734745c serial/amba-pl011: Activate TX IRQ passively You can pull in my updated series if you want, but at this point further changes are likely: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330619.html See below for context.] On Tue, Mar 17, 2015 at 02:42:48PM +0000, Jakub Kicinski wrote: > On Tue, 17 Mar 2015 13:58:44 +0000, Dave P Martin wrote: > > On Mon, Mar 16, 2015 at 11:15:29PM +0000, Jakub Kicinski wrote: [...] > > It is indeed overcomplicated -- I reposted a simplified version last > > week, see: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330619.html > > > > (The message threading is broken on the server due to the patches being > > received out of order -- there are two patches following the cover > > letter the link points to.) > > I'm a bit confused by this. The first version of the patch seems to be > already present in Greg's tty-next [1] as well as linux-next. However, > the v2 of your patch which you pointed me to is based on tty-next > without it applied. I did see the v2 but assumed the version already > committed to the repository is the final one... > > How would this be solved? Are rebases allowed in tty-next? -next is an automated merge of lots of people's branches. This helps give advance warning of changes that are likely to break the kernel, or each other. Stuff in -next isn't necessarily accepted by anyone for actual merging yet. All history can get rewritten until patches land in torvalds/master. Many maintainers keep separate branches for sending to -next, so that immature patches can get wider visibility before they accept them for merging; a separate branch (called for-linus or similar) is often used for sending pull requests to Linus. > > > Can you try to rebase? > > > > My update still keeps the softirq stuff. I wanted to avoid adding > > status polling inside the interrupt handler's core loop, due to > > concerns about performance overhead: without the polling, the > > writes to DR are fire-and-forget, whereas polling FR each time > > involves a whole round-trip to the UART which may involve significant > > extra time cost and/or IRQ disable latency; however. > > > > Your approach does mitigate some of the cost by only starting to > > poll after count chars have been transmitted, and will typically > > halve the number of IRQs taken -- which could lead to a net benefit. > > It would be good to see some benchmarks to understand how much > > difference it makes to performance. > > I will *try* to come up with some figures but I don't have any > high-speed UARTs so my tests run at 115k tops. I think dropping the FF > poll at the end of load from IRQ could be a good idea. And I have to > respin anyway because I just noticed I added a double new line in > pl011_tx_chars()... Maybe you can drop some getnstimeofday() into pl011_int() and/or pl011_tx_chars(), to collect some simple stats? I might have a go at this if I get a moment -- but any info you could obtain would be helpful. For one thing, the behvaiour might vary a bit between platforms, even for a fixed baud rate. > > Looping in pl011_tx_chars() until either the FIFO is full or > > we run out of chars to transmit gets rid of the need for the > > softirq, so I wouldn't be opposed to keeping that if the > > performance impact is not too significant. > > TBH the softirq stuff I don't quite understand. I had never seen it > fire in my tests. Can you explain its purpose? Is there HW out there > which fails to fire the IRQ (but it wouldn't work until now anyhow)? At the moment it's partly theoretical: I have seen the TX IRQ fail to fire on some software models -- it's not that uncommon for a model not to bother emulating the actual transmission delay, so some effectively drain every char written to the FIFO instantly. It could happen in some hardware configurations too -- I once had an embedded UART running at 3Mbps, fed by a 15MHz ARM7TDMI at one point. Could have gone faster, but the FTDI uart at the other end of the link used a different base clock, making it hard to match the rates closely enough at high speeds. Even on fast modern platforms, DVFS may slow down the CPU quite aggressively on some systems. That said, my current approach of backing off and scheduling a softirq doesn't necessarily bring a lot of benefit. The main difference is that writing a huge buffer to a UART with an unfillable TX FIFO will just leave execution stuck in pl011_int() holding the port lock for a long time until all the data has been sent. But I think that would happen even with the original driver code -- TXIS would stay asserted, so pl011_int() would just keep calling _tx_chars() without releasing the port lock or returning. I think I'll have a try at getting rid of the softirq, following your example, and see how things look. A system where feeding the UART really does take ~100%CPU really has a design problem, which we can't expect to fix in the pl011 driver... [...] > > On port shutdown, the TX interrupt will be masked by IMSC, but should > > remain asserted inside the PL011, triggering an interrupt the next time > > it is unmasked. > > > > If you observed something that conflicts with these assumptions, > > I'd be interested to know what's going on... > > Actually seeing this bug in practice was what motivated this work. > I triggered it by catting a long file over the UART and then hitting > ^C. UART died every time... That being said I test this on BCM2708 > which is also notorious for transmitting while device in loopback > configuration, so perhaps it's implementation-dependent... Can you describe in more detail what you did? I don't think that ^C should break things ever, but I'd like to try to reproduce it just in case there is some faulty logic somewhere... Cheers ---Dave > > [1] > https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=734745caeb9f155ab58918834a8c70e83fa6afd3 > -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
