[Qemu-devel] [PATCH] ehci_free_packet: Discard finished packets when the queue is halted

[Qemu-devel] [PATCH] ehci_free_packet: Discard finished packets when the queue is halted

[Hans de Goede]

With pipelining it is possible to encounter a finished packet when cleaning
the queue due to a halt. This happens when a non stall error happens while
talking to a real device. In this case the queue on the usb-host side will
continue processing packets, and we can have completed packets waiting in
the queue after an error condition packet causing a halt.

There are 2 reasons to discard the completed packets at this point, rather
then trying to writing them back to the guest:

1) The guest expect to be able to cancel and/or change packets after the
packet with the error without doing an unlink, so writing them back may
confuse the guest.

2) Since the queue does not advance when halted, the writing back of these
packets will fail anyways since p->qtdaddr != q->qtdaddr, so the
ehci_verify_qtd call in ehci_writeback_async_complete_packet will fail.

Note that 2) means that then only functional change this patch introduces
is the printing of a warning when this scenario happens.

Note that discarding these packets means that the guest driver and the device
will get out of sync! This is unfortunate, but should not be a problem since
with a non stall error (iow an io-error) the 2 are out of sync already anyways.
Still this patch adds a warning to signal this happening.

Note that sofar this has only been seen with a DVB-T receiver, which gives
of a MPEG-2 stream, which allows for recovering from lost packets, see:
https://bugzilla.redhat.com/show_bug.cgi?id=890320

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 5176251..626a4c5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -586,17 +586,23 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 
 static void ehci_free_packet(EHCIPacket *p)
 {
-    if (p->async == EHCI_ASYNC_FINISHED) {
+    if (p->async == EHCI_ASYNC_FINISHED &&
+            !(p->queue->qh.token & QTD_TOKEN_HALT)) {
         ehci_writeback_async_complete_packet(p);
         return;
     }
     trace_usb_ehci_packet_action(p->queue, p, "free");
-    if (p->async == EHCI_ASYNC_INITIALIZED) {
-        usb_packet_unmap(&p->packet, &p->sgl);
-        qemu_sglist_destroy(&p->sgl);
-    }
     if (p->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&p->packet);
+    }
+    if (p->async == EHCI_ASYNC_FINISHED) {
+        fprintf(stderr,
+                "EHCI: Dropping completed packet from halted %s ep %02X\n",
+                (p->pid == USB_TOKEN_IN) ? "in" : "out",
+                get_field(p->queue->qh.epchar, QH_EPCHAR_EP));
+                
+    }
+    if (p->async != EHCI_ASYNC_NONE) {
         usb_packet_unmap(&p->packet, &p->sgl);
         qemu_sglist_destroy(&p->sgl);
     }
-- 
1.8.2

[Qemu-devel] [PATCH 0/1] ehci_free_packet: Discard finished packets when the queue is halted v2

[Hans de Goede]

Hi Gerd,

Here is a v2 of the patch I send yesterday, changed in v2 is that the warning
is only printed when packets with a status of success are dropped, since on
a regular stall or disconnect chances are all other packets in the queue 
also have completed with an error status when we process the queue.

This is a normal condition, on which we should not warn / log.

Regards,

Hans

[Qemu-devel] [PATCH] ehci_free_packet: Discard finished packets when the queue is halted

[Hans de Goede]

With pipelining it is possible to encounter a finished packet when cleaning
the queue due to a halt. This happens when a non stall error happens while
talking to a real device. In this case the queue on the usb-host side will
continue processing packets, and we can have completed packets waiting in
the queue after an error condition packet causing a halt.

There are 2 reasons to discard the completed packets at this point, rather
then trying to writing them back to the guest:

1) The guest expect to be able to cancel and/or change packets after the
packet with the error without doing an unlink, so writing them back may
confuse the guest.

2) Since the queue does not advance when halted, the writing back of these
packets will fail anyways since p->qtdaddr != q->qtdaddr, so the
ehci_verify_qtd call in ehci_writeback_async_complete_packet will fail.

Note that 2) means that then only functional change this patch introduces
is the printing of a warning when this scenario happens.

Note that discarding these packets means that the guest driver and the device
will get out of sync! This is unfortunate, but should not be a problem since
with a non stall error (iow an io-error) the 2 are out of sync already anyways.
Still this patch adds a warning to signal this happening.

Note that sofar this has only been seen with a DVB-T receiver, which gives
of a MPEG-2 stream, which allows for recovering from lost packets, see:
https://bugzilla.redhat.com/show_bug.cgi?id=890320

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 5176251..6913b42 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -586,17 +586,24 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 
 static void ehci_free_packet(EHCIPacket *p)
 {
-    if (p->async == EHCI_ASYNC_FINISHED) {
+    if (p->async == EHCI_ASYNC_FINISHED &&
+            !(p->queue->qh.token & QTD_TOKEN_HALT)) {
         ehci_writeback_async_complete_packet(p);
         return;
     }
     trace_usb_ehci_packet_action(p->queue, p, "free");
-    if (p->async == EHCI_ASYNC_INITIALIZED) {
-        usb_packet_unmap(&p->packet, &p->sgl);
-        qemu_sglist_destroy(&p->sgl);
-    }
     if (p->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&p->packet);
+    }
+    if (p->async == EHCI_ASYNC_FINISHED &&
+            p->packet.status == USB_RET_SUCCESS) {
+        fprintf(stderr,
+                "EHCI: Dropping completed packet from halted %s ep %02X\n",
+                (p->pid == USB_TOKEN_IN) ? "in" : "out",
+                get_field(p->queue->qh.epchar, QH_EPCHAR_EP));
+                
+    }
+    if (p->async != EHCI_ASYNC_NONE) {
         usb_packet_unmap(&p->packet, &p->sgl);
         qemu_sglist_destroy(&p->sgl);
     }
-- 
1.8.2

[Qemu-devel] [PATCH 0/1] ehci_free_packet: Discard finished packets when the queue is halted v3

[Hans de Goede]

Hi Gerd,

And here is a v3 fixing a whitespace issue in v2, sorry about the noise.

Regards,

Hans

[Qemu-devel] [PATCH] ehci_free_packet: Discard finished packets when the queue is halted

[Hans de Goede]

With pipelining it is possible to encounter a finished packet when cleaning
the queue due to a halt. This happens when a non stall error happens while
talking to a real device. In this case the queue on the usb-host side will
continue processing packets, and we can have completed packets waiting in
the queue after an error condition packet causing a halt.

There are 2 reasons to discard the completed packets at this point, rather
then trying to writing them back to the guest:

1) The guest expect to be able to cancel and/or change packets after the
packet with the error without doing an unlink, so writing them back may
confuse the guest.

2) Since the queue does not advance when halted, the writing back of these
packets will fail anyways since p->qtdaddr != q->qtdaddr, so the
ehci_verify_qtd call in ehci_writeback_async_complete_packet will fail.

Note that 2) means that then only functional change this patch introduces
is the printing of a warning when this scenario happens.

Note that discarding these packets means that the guest driver and the device
will get out of sync! This is unfortunate, but should not be a problem since
with a non stall error (iow an io-error) the 2 are out of sync already anyways.
Still this patch adds a warning to signal this happening.

Note that sofar this has only been seen with a DVB-T receiver, which gives
of a MPEG-2 stream, which allows for recovering from lost packets, see:
https://bugzilla.redhat.com/show_bug.cgi?id=890320

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/hcd-ehci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 5176251..0d3799d 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -586,17 +586,23 @@ static EHCIPacket *ehci_alloc_packet(EHCIQueue *q)
 
 static void ehci_free_packet(EHCIPacket *p)
 {
-    if (p->async == EHCI_ASYNC_FINISHED) {
+    if (p->async == EHCI_ASYNC_FINISHED &&
+            !(p->queue->qh.token & QTD_TOKEN_HALT)) {
         ehci_writeback_async_complete_packet(p);
         return;
     }
     trace_usb_ehci_packet_action(p->queue, p, "free");
-    if (p->async == EHCI_ASYNC_INITIALIZED) {
-        usb_packet_unmap(&p->packet, &p->sgl);
-        qemu_sglist_destroy(&p->sgl);
-    }
     if (p->async == EHCI_ASYNC_INFLIGHT) {
         usb_cancel_packet(&p->packet);
+    }
+    if (p->async == EHCI_ASYNC_FINISHED &&
+            p->packet.status == USB_RET_SUCCESS) {
+        fprintf(stderr,
+                "EHCI: Dropping completed packet from halted %s ep %02X\n",
+                (p->pid == USB_TOKEN_IN) ? "in" : "out",
+                get_field(p->queue->qh.epchar, QH_EPCHAR_EP));
+    }
+    if (p->async != EHCI_ASYNC_NONE) {
         usb_packet_unmap(&p->packet, &p->sgl);
         qemu_sglist_destroy(&p->sgl);
     }
-- 
1.8.2