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

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

Hi,

On 10/17/2012 01:29 PM, Gerd Hoffmann wrote:
>    Hi,
>
>> +    /*
>> +     * 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);
>
> Do we really need these?

For handle_combined_data, yes, as usb_ep_combine_input_packets can
cause multiple packets to get submitted, since if a combined packet
ends with a packet, which does not have its short_not_ok flag set,
another packet can be safely pipelined after it. This is not
useful for usb mass storage, but very usefull for usb serial
port converters.

I actually first did not have cancel_combined_packet :) Instead
I had usb_combined_packet_cancel() return a boolean indicating
if it had handled the cancel, or if the caller (which would be
a device's cancel method) needs to handle the cancel itself.

I personally find the cancel_combined_packet callback somewhat
cleaner, esp. since with the return boolean method,
after calling usb_combined_packet_cancel(p) p->combined will be
NULL, so if the device's cancel method needs access to p->combined
to deal with combined packets, things get more difficult.

But if you prefer getting rid of the callback we can do that.

> I think it isn't much work for the callers to
> do that themself.  Saves them providing a callback.  And makes the code
> flow easier to follow by removing a pointless indirection.
>
> For handle_combined_data we probably must make
> usb_ep_combine_input_packets return a status code.

Regards,

Hans