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.
>
> 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