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

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

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