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.
Helmut
--
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
