From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37195) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTBne-00055V-DY for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:24:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TTBnX-0008LL-MU for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:24:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57877) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TTBnX-0008Kw-Df for qemu-devel@nongnu.org; Tue, 30 Oct 2012 09:24:03 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9UDO25I010144 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 30 Oct 2012 09:24:02 -0400 Message-ID: <508FD55C.30008@redhat.com> Date: Tue, 30 Oct 2012 14:25:48 +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. > > 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. > p.s. If we stick with handle_combined_data taking a USBPacket as argument, then I would like to do the same for cancel_combined_packet for consistency! Regards, Hans