From patchwork Thu Nov 1 13:59:06 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 196205 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1F0832C034B for ; Fri, 2 Nov 2012 00:57:35 +1100 (EST) Received: from localhost ([::1]:35436 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTvH3-0003nN-Cs for incoming@patchwork.ozlabs.org; Thu, 01 Nov 2012 09:57:33 -0400 Received: from eggs.gnu.org ([208.118.235.92]:35901) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTvGs-0003m8-2j for qemu-devel@nongnu.org; Thu, 01 Nov 2012 09:57:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTvGq-0005UP-FD for qemu-devel@nongnu.org; Thu, 01 Nov 2012 09:57:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53249) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTvGq-0005UC-62 for qemu-devel@nongnu.org; Thu, 01 Nov 2012 09:57:20 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qA1DvJxE031181 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 1 Nov 2012 09:57:19 -0400 Received: from shalem.localdomain (vpn1-7-7.ams2.redhat.com [10.36.7.7]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qA1DvH13021992 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Thu, 1 Nov 2012 09:57:18 -0400 Message-ID: <5092802A.7000000@redhat.com> Date: Thu, 01 Nov 2012 14:59:06 +0100 From: Hans de Goede User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 MIME-Version: 1.0 To: Gerd Hoffmann References: <1351687636-14253-1-git-send-email-hdegoede@redhat.com> <1351687636-14253-2-git-send-email-hdegoede@redhat.com> <50924A2F.7090706@redhat.com> In-Reply-To: <50924A2F.7090706@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 From fbcfdf12cbf28c2d1b0e11e290701376aaea554c Mon Sep 17 00:00:00 2001 From: Hans de Goede 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 --- 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