From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTC0Q-0000Z8-Mi for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:37:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTC0E-00050t-Ei for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:37:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63196) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTC0E-00050m-6D for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:37:10 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9UDb8g6032086 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 30 Oct 2012 09:37:09 -0400 Message-ID: <508FD86E.3030205@redhat.com> Date: Tue, 30 Oct 2012 14:38:54 +0100 From: Hans de Goede MIME-Version: 1.0 References: <1351095258-5579-1-git-send-email-hdegoede@redhat.com> <1351095258-5579-16-git-send-email-hdegoede@redhat.com> <5088E259.1080206@redhat.com> <508FD4DB.7000900@redhat.com> In-Reply-To: <508FD4DB.7000900@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 15/22] usb: Add packet combining functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org Hi, On 10/30/2012 02:23 PM, Hans de Goede wrote: > Hi, > > On 10/25/2012 08:55 AM, Gerd Hoffmann wrote: >> On 10/24/12 18:14, Hans de Goede wrote: >>> + /* >>> + * Process / cancel combined packets, called from >>> + * usb_ep_combine_input_packets() / usb_combined_packet_cancel(). >>> + * Only called for devices which call these functions themselves. >>> + */ >>> + int (*handle_combined_data)(USBDevice *dev, USBPacket *p); >>> + void (*cancel_combined_packet)(USBDevice *dev, USBPacket *p); >> >> I still think these should get a USBCombinedPacket not a USBPacket. > > I just rebased my tree's USB bits to your usb.68 pull req, and then > tried to make this change, and then I realized again why at least > handle_combined_data is not getting a USBCombinedPacket as argument. > > The call sequence goes like this: > > 1) hcd calls usb_handle_packet > 2) usb_handle_packet calls devices handle_data (through process_one) > 3) device's handle_data sees this is for a input ep on which it is > doing input pipelining, returns USB_RET_ADD_TO_QUEUE > 4) hcd calls usb_device_flush_ep_queue > 5) usb_device_flush_ep_queue calls usb_ep_combine_input_packets > 6) usb_ep_combine_input_packets either ends up with a combined > packet, or with a single regular packet to send to > the device > > Currently usb_ep_combine_input_packets calls the device's > handle_combined_data method in both cases, and that can distinguish > between the 2 scenarios by checking the passed in USBPacket's > combined field. > > I did things this way, even though it may seem more logical for > usb_ep_combine_input_packets to call the device's "regular" > handle_data method in case no combining is done for a packet, > so it is submitting a single regular packet, but in that case > we would end up at step 3) again, and the device's handle_data > will again return USB_RET_ADD_TO_QUEUE which is not what we want. Oh wait, thinking more about this, the device's handle_data method can see the difference between the 1st and 2nd call, as the packets state will have changed from USB_PACKET_SETUP to USB_PACKET_QUEUED. Using that, we can do even better and completely get rid of the handle_combined_data and cancel_combined_packet methods. Instead have the device code check for USBPacket->combined where appropriate, this also makes the handle_data and cancel_packet paths more alike, as the device already needed to check for USBPacket->combined in its cancel_packet method. New version coming up! Regards, Hans