Re: [Qemu-devel] [PATCH 1/8] usb: Add packet combining functions

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

Hi,

On 11/01/2012 02:14 PM, Hans de Goede wrote:
> Hi,
>
> On 11/01/2012 11:08 AM, Gerd Hoffmann wrote:
>> On 10/31/12 13:47, Hans de Goede wrote:
>>> +    /*
>>> +     * If we had leftover packets the hcd driver will have cancelled them
>>> +     * and usb_combined_packet_cancel has already freed combined!
>>> +     */
>>> +    if (state != leftover) {
>>> +        g_free(combined);
>>> +    }
>>
>> This calls for reference-counting USBCombinedPacket IMHO.
>
> Why? We call packet_complete with a status if USB_RET_REMOVE_FROM_QUEUE
> if we've left-over packets, the hcd code will cancel these, and
> usb_combined_packet_cancel will free the combined packet when the
> last packet of it gets cancelled, which *will* happen as we're
> always processing *all* packets in combined here. There is no
> scenario here where one or the other party wants to keep the
> combined packet around any longer...
>
> The only reason this is a bit non straightforward is that
> normally packets get freed either on completion or cancellation,
> but here we've a partial completion and a partial cancellation.

Also note that reference counting will not make the special case
go away, as for combined packets without any leftover packets the
packet_complete (status == USB_RET_REMOVE_FROM_QUEUE) -> cancel
-> free/unref will never happen.

So simply taking a ref at the beginning of usb_combined_input_packet_complete
and then doing unref at the end will not help. Because for combined-packets
where all packets where used we then would need to do unref twice, once
to drop the local ref, and once to drop the final ref.

Regards,

Hans