Patchwork [1/8] usb: Add packet combining functions

login
register
mail settings
Submitter Hans de Goede
Date Nov. 1, 2012, 1:59 p.m.
Message ID <5092802A.7000000@redhat.com>
Download mbox | patch
Permalink /patch/196205/
State New
Headers show

Comments

Hans de Goede - Nov. 1, 2012, 1:59 p.m.
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
Gerd Hoffmann - Nov. 1, 2012, 2:23 p.m.
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.
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.
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.
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

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