Re: [Qemu-devel] [PATCH 15/22] usb: Add packet combining functions

[Hans de Goede <hdegoede@redhat.com>]

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