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

[Hans de Goede]

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