diff mbox

[1/8] usb: Add packet combining functions

Message ID 5092802A.7000000@redhat.com
State New
Headers show

Commit Message

Hans de Goede Nov. 1, 2012, 1:59 p.m. UTC 
Hi,

On 11/01/2012 11:08 AM, Gerd Hoffmann wrote:
> On 10/31/12 13:47, Hans de Goede wrote:
>> +    /*
>> +     * If we had leftover packets the hcd driver will have cancelled them
>> +     * and usb_combined_packet_cancel has already freed combined!
>> +     */
>> +    if (state != leftover) {
>> +        g_free(combined);
>> +    }
>
> This calls for reference-counting USBCombinedPacket IMHO.

Here is a patch changing the way this is handled to something which
hopefully will be more to your liking. Feel free to squash this
into the original commit.

Regards,

Hans

Comments

Gerd Hoffmann Nov. 1, 2012, 2:23 p.m. UTC | #1 
Hi,

> Here is a patch changing the way this is handled to something which
> hopefully will be more to your liking. Feel free to squash this
> into the original commit.

Looks good but doesn't apply.  Likewise the "split packet result into
actual_length + status" patch series.

Pushed current usb patch queue to
http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next, please rebase
and resend.

thanks,
  Gerd
Hans de Goede Nov. 1, 2012, 4:17 p.m. UTC | #2 
Hi,

On 11/01/2012 03:23 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Here is a patch changing the way this is handled to something which
>> hopefully will be more to your liking. Feel free to squash this
>> into the original commit.
>
> Looks good but doesn't apply.  Likewise the "split packet result into
> actual_length + status" patch series.
>
> Pushed current usb patch queue to
> http://www.kraxel.org/cgit/qemu/log/?h=rebase/usb-next, please rebase
> and resend.

Rebased and re-rested (a bit), new version send.

I see you've just done a pull-req for what was in usb-next already,
I really hope you can do another one with the status + length splitting
patches in there, so that those can go into 1.3, this will make it
a lot easier to cherry pick any later bugfixes from master to 1.3

Thanks & Regards,

Hans
Gerd Hoffmann Nov. 1, 2012, 4:42 p.m. UTC | #3 
Hi,

> Rebased and re-rested (a bit), new version send.

Added to the queue.

> I see you've just done a pull-req for what was in usb-next already,
> I really hope you can do another one with the status + length splitting
> patches in there, so that those can go into 1.3, this will make it
> a lot easier to cherry pick any later bugfixes from master to 1.3

It's soft freeze only, not hard freeze yet (see
http://wiki.qemu.org/Planning/1.3).  It's a bit invasive (although local
to usb), but it isn't a major change.  So merging this a bit later
should be no problem.  I'll try to get a incremental pull out of the
door before kvm forum nevertheless.

cheers,
  Gerd
Hans de Goede Nov. 1, 2012, 7:07 p.m. UTC | #4 
Hi,

On 11/01/2012 05:42 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> Rebased and re-rested (a bit), new version send.
>
> Added to the queue.
>
>> I see you've just done a pull-req for what was in usb-next already,
>> I really hope you can do another one with the status + length splitting
>> patches in there, so that those can go into 1.3, this will make it
>> a lot easier to cherry pick any later bugfixes from master to 1.3
>
> It's soft freeze only, not hard freeze yet (see
> http://wiki.qemu.org/Planning/1.3).  It's a bit invasive (although local
> to usb), but it isn't a major change.  So merging this a bit later
> should be no problem.  I'll try to get a incremental pull out of the
> door before kvm forum nevertheless.

Ok, thanks!

Regards,

Hans
diff mbox

Patch

From fbcfdf12cbf28c2d1b0e11e290701376aaea554c Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 1 Nov 2012 14:46:31 +0100
Subject: [PATCH] usb/combined-packet: Move freeing of combined to
 usb_combined_packet_remove()

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/combined-packet.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index e722198..4a0c299 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -31,12 +31,16 @@  static void usb_combined_packet_add(USBCombinedPacket *combined, USBPacket *p)
     p->combined = combined;
 }
 
+/* Note will free combined when the last packet gets removed */
 static void usb_combined_packet_remove(USBCombinedPacket *combined,
                                        USBPacket *p)
 {
     assert(p->combined == combined);
     p->combined = NULL;
     QTAILQ_REMOVE(&combined->packets, p, combined_entry);
+    if (QTAILQ_EMPTY(&combined->packets)) {
+        g_free(combined);
+    }
 }
 
 /* Also handles completion of non combined packets for pipelined input eps */
@@ -45,9 +49,8 @@  void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
     USBCombinedPacket *combined = p->combined;
     USBEndpoint *ep = p->ep;
     USBPacket *next;
-    enum { completing, complete, leftover };
-    int status, actual_length, state = completing;
-    bool short_not_ok;
+    int status, actual_length;
+    bool short_not_ok, done = false;
 
     if (combined == NULL) {
         usb_packet_complete_one(dev, p);
@@ -61,39 +64,34 @@  void usb_combined_input_packet_complete(USBDevice *dev, USBPacket *p)
     short_not_ok = QTAILQ_LAST(&combined->packets, packets_head)->short_not_ok;
 
     QTAILQ_FOREACH_SAFE(p, &combined->packets, combined_entry, next) {
-        if (state == completing) {
+        if (!done) {
             /* Distribute data over uncombined packets */
             if (actual_length >= p->iov.size) {
                 p->actual_length = p->iov.size;
             } else {
                 /* Send short or error packet to complete the transfer */
                 p->actual_length = actual_length;
-                state = complete;
+                done = true;
             }
             /* Report status on the last packet */
-            if (state == complete || next == NULL) {
+            if (done || next == NULL) {
                 p->status = status;
             } else {
                 p->status = USB_RET_SUCCESS;
             }
             p->short_not_ok = short_not_ok;
+            /* Note will free combined when the last packet gets removed! */
             usb_combined_packet_remove(combined, p);
             usb_packet_complete_one(dev, p);
             actual_length -= p->actual_length;
         } else {
             /* Remove any leftover packets from the queue */
-            state = leftover;
             p->status = USB_RET_REMOVE_FROM_QUEUE;
+            /* Note will free combined on the last packet! */
             dev->port->ops->complete(dev->port, p);
         }
     }
-    /*
-     * If we had leftover packets the hcd driver will have cancelled them
-     * and usb_combined_packet_cancel has already freed combined!
-     */
-    if (state != leftover) {
-        g_free(combined);
-    }
+    /* Do not use combined here, it has been freed! */
 leave:
     /* Check if there are packets in the queue waiting for our completion */
     usb_ep_combine_input_packets(ep);
@@ -104,14 +102,13 @@  void usb_combined_packet_cancel(USBDevice *dev, USBPacket *p)
 {
     USBCombinedPacket *combined = p->combined;
     assert(combined != NULL);
+    USBPacket *first = p->combined->first;
 
+    /* Note will free combined on the last packet! */
     usb_combined_packet_remove(combined, p);
-    if (p == combined->first) {
+    if (p == first) {
         usb_device_cancel_packet(dev, p);
     }
-    if (QTAILQ_EMPTY(&combined->packets)) {
-        g_free(combined);
-    }
 }
 
 /*
-- 
1.7.12.1