On Fri, 2016-07-29 at 09:38:58 +0200, Helmut Grohne wrote: > On Thu, Jul 28, 2016 at 01:07:41PM -0700, Sören Brinkmann wrote: > > On Thu, 2016-07-28 at 10:38:25 +0200, Helmut Grohne wrote: > > > Note that e3538c37ee38 partially hides a hardware race condition that > > > becomes visible again after applying this patch. > > > > Is there any harm in this functional change? I'm more afraid of exposing > > the race again, TBH. > > The race condition is only partially hidden. A proper fix for that is > necessary anyway and I'll be sending that soon. > > I argue that e3538c37ee38 leaves cdns_uart_console_write in a misleading > state and that it should be fixed one way or another. The patch I sent > tries to restore the original behaviour of leaving the TX_EN and TX_DIS > bits unchanged. > > The other way is to acknowledge that the transmitter should really be > enabled all the time (and e.g. cdns_uart_resume leaves it enabled for > instance). In this case the extra > > writel(ctrl, port->membase + CDNS_UART_CR); > > is writing the same value as the previous write and should be removed. > > Going that route poses more questions though: What is disabling the > transmitter? And isn't that disabling wrong in the first place? > > Turns out that cdns_uart_startup disables the transmitter (and > 6e14f7c1f2c2 "tty: xuartps: Improve startup function" says that this > behaviour is intentional). It also turns out that cdns_uart_startup is > called rather frequently when booting with systemd. > > So the other way really means changing the behaviour of > cdns_uart_startup and you, Sören, argued in favour of its current > behaviour. > > I'm confused as to which assumptions are wrong. Thus reverting the > obviously unintended functional change seemed least controversial to me. TBH, I don't recall all the details. But I found my system locking up when booting with systemd, with the UART in a state waiting for draining some FIFOs with transmiter and/or receiver disabled. Hence, I'm dubious of "fixing" anything by disabling RX/TX unless somebody sees a real issue this is fixing. If this is just a theoretical exercise, I'd rather keep it as is, as I haven't seen any issues with the UART since those changes. And debugging such races is not fun. Sören -- 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
