On Tue, 18 Feb 2025 at 13:54, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Mon, 17 Feb 2025 at 14:55, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >
> > On Sat, 8 Feb 2025 at 16:39, Philippe Mathieu-Daudé <phi...@linaro.org>
> > wrote:
> > >
> > > Hi,
> > >
> > > This series add support for (async) FIFO on the transmit path
> > > of the PL011 UART.
> > >
> >
> > Applied to target-arm.next, thanks (with a couple of minor
> > tweaks to two of the patches).
>
> Unfortunately I seem to get failures in 'make check-functional'
> with the last patch of this series applied.
I had a look at this this morning because I wondered if it
was a mistake in the style fixups I'd applied to the patches
on my end, and I found the bug fairly quickly. The problem is
that pl011_xmit() doesn't update the TXFE and TXFF FIFO empty/full
status flag bits when it removes characters from the FIFO.
So the guest kernel spins forever because TXFF is never unset.
The following patch fixes this for me (and also makes us not
set INT_TX for the case where we couldn't send any bytes to
the chardev, which I noticed reading the code rather than
because it had any visible bad effects):
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -256,6 +256,7 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
int bytes_consumed;
uint8_t buf[PL011_FIFO_DEPTH];
uint32_t count;
+ bool emptied_fifo;
count = fifo8_num_used(&s->xmit_fifo);
trace_pl011_fifo_tx_xmit_used(count);
@@ -280,15 +281,24 @@ static gboolean pl011_xmit(void *do_not_use,
GIOCondition cond, void *opaque)
/* Error in back-end: drain the fifo. */
pl011_drain_tx(s);
return G_SOURCE_REMOVE;
+ } else if (bytes_consumed == 0) {
+ /* Couldn't send anything, try again later */
+ return G_SOURCE_CONTINUE;
}
/* Pop the data we could transmit. */
fifo8_drop(&s->xmit_fifo, bytes_consumed);
s->int_level |= INT_TX;
+ s->flags &= ~PL011_FLAG_TXFF;
+
+ emptied_fifo = fifo8_is_empty(&s->xmit_fifo);
+ if (emptied_fifo) {
+ s->flags |= PL011_FLAG_TXFE;
+ }
pl011_update(s);
- if (!fifo8_is_empty(&s->xmit_fifo)) {
+ if (!emptied_fifo) {
/* Reschedule another transmission if we couldn't transmit all. */
return G_SOURCE_CONTINUE;
}
If you're OK with that as a fix then I'll squash it in
and keep the series in target-arm.next.
thanks
-- PMM
