On Thu, Mar 12, 2015 at 12:55:19PM +0000, Dave Martin wrote:
> I don't think my patches change this situation. For normal printk,
> pl011_console_write() will disable IRQs at the CPU and take port->lock:
> so either pl011_int() finishes all its work safely before the console
> output is done, or the console output is done first.
>
> In the latter case, the condition that triggered the IRQ (i.e., TX
> FIFO has space) may no longer be true: the lock protects pl011_int()
> from the race that would otherwise exist between the check on TXIS
> and the actual writes to the FIFO.
>
> Only in special cases (sysrq and oopses) might the lock not be
> held. But this problem exists independently of my changes AFAICT.
> Correct output seems to be best-effort in these cases, which is
> probably the right thing to do.
Actually, yes, you're right, I'm wrong. The console won't drop the
spinlock until the last character has left the TX FIFO and has been
sent on the wire, so at the point I was originally thinking of, we
guarantee that the FIFO is empty (if it weren't like that, it'd
almost certainly be full.)
> > If hardware CTS flow control is enabled, the problem gets worse - in
> > that mode, the TX FIFO can remain fully occupied for an indeterminant
> > amount of time.
>
> Not really? Regardless of why the FIFO is full, pl011_int() won't
> call pl011_tx_chars() because a full FIFO means TXIS is deasserted.
>
> In the start_tx/softirq case TXFF will be checked, as I try to
> explain above.
Yes, in that case, pl011_int() won't be called for transmit, but what
about the softirq case.
Looking at the patch again, I don't see pl011_tx_chars() ever returning
true - so the while (pl011_tx_chars()); in the softirq looks strange
(if it always returns false, we won't ever iterate.)
> Unless you have any other ideas I will try to simplify the code
> along the above lines.
I forget whether we have problems with UARTs which don't generate any TX
irqs - I hope we don't.
What about doing this instead, which should result in fewer changes to
the driver (especially to the fast paths):
- On startup, set a flag to indicate that the TX state is unknown (so we
can detect the first start_tx().)
- When we get our first start_tx(), check the flag.
- read the FIFO flags and interrupt status. If the FIFO flags indicate
that the TX FIFO is empty, but there is no transmit interrupt, read
and load the first character into the FIFO. Instead of just loading
the first character, as we've checked the FIFO status, we could
alternatively load the first half of the FIFO via a call to
pl011_tx_chars().
- clear the flag.
This should mean that we keep the driver structure we have today.
The only concern I have about that is (from what I remember) we don't
expect a call to ->start_tx to cause a tx stop event, though I don't
see anything in serial_core which would get upset if we did.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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
