From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36305) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTBlZ-00044e-TP for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:22:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTBlT-0007bD-VE for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:22:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57820) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTBlT-0007b0-Mb for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:21:55 -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 q9UDLs2w028056 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 30 Oct 2012 09:21:54 -0400 Message-ID: <508FD4DB.7000900@redhat.com> Date: Tue, 30 Oct 2012 14:23:39 +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> In-Reply-To: <5088E259.1080206@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/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. This is why handle_combined_data takes a USBPacket, and then checks USBPacket->combined to see what to do, rather then taking a USBCombinedPacket, as usb_ep_combine_input_packets simply does not always have a combined packet to pass. Alternatives to allow handle_combined_data to take a USBCombinedPacket, would be: 1) Some flag to the device's handle_data method to indicate it should *really* process the packet and not return USB_RET_ADD_TO_QUEUE 2) Always make Uc allocate a USBCombinedPacket, even when the entire transfer consists of only a single packet, note that this in essence means an unnecessary malloc + free call per such packet, and for example with (redirected) usb-mass-storage one can expect each scsi sense phase transfer to be only a single packet large, and for smaller reads the data phase packets as well! IMHO either alternative is worse then simply passing a USBPacket to handle_combined_data, and let the device's handle_combined_data figure out what to do based on USBPacket->combined. Note that if it were to take a USBCombinedPacket, it would end up getting back to the USBPacket itself through USBCombinedPacket->first anyways to get info from there such as the packet id. Regards, Hans