Re: [Qemu-devel] [PATCH 17/18] usb: move cancel callback to USBDeviceInfo

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

Hi,

Gerd, this is more or less a copy of a mail I send you directly earlier
before I saw this pull request.

NACK for this one, rational:

While working on re-basing / re-doing my usb network redirection code for qemu, on top of
your usb.12 I've hit a problem caused by the move of the async cancel callback from
USBPacket to USBDevice, and I think I know now why it used to be in USBPacket
(and we may need to move it back).

The problem is that the USBDevice lifetime may be shorter then the USBPacket lifetime,
USBPackets are created by uhci.c (for example), where as the device is managed
from the monitor (for example), doing a usb_del in the monitor using the guest bus:addr
will call usb_device_delete_addr, which will call qdev_free. At this time the
USBDevice struct is gone, and at a later time the uhci code will cancel any still
outstanding async packets, who's owner pointer will now point to free-ed memory.

Note that doing a usb_del on a (direct or net) redirected device used to work
before.

We could ref-count the USBDevice, but would make the usb_del not do anything
as long as some async requests are active, which seems wrong.

Another solution would be moving the async_cancel function pointer + an opaque
ptr back to the USBPacket, which seems best to me...

Note the above is all theory I've not yet tried this yet (still need to finish
up re-basing my code first).

Regards,

Hans


On 05/23/2011 11:43 AM, Gerd Hoffmann wrote:
> Remove the cancel callback from the USBPacket struct, move it over
> to USBDeviceInfo.  Zap usb_defer_packet() which is obsolete now.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   hw/usb-msd.c |    8 +++-----
>   hw/usb.c     |    2 +-
>   hw/usb.h     |   17 +++++------------
>   usb-linux.c  |    7 +++----
>   4 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index 1064920..141da2c 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -315,9 +315,9 @@ static int usb_msd_handle_control(USBDevice *dev, USBPacket *p,
>       return ret;
>   }
>
> -static void usb_msd_cancel_io(USBPacket *p, void *opaque)
> +static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
>   {
> -    MSDState *s = opaque;
> +    MSDState *s = DO_UPCAST(MSDState, dev, dev);
>       s->scsi_dev->info->cancel_io(s->scsi_dev, s->tag);
>       s->packet = NULL;
>       s->scsi_len = 0;
> @@ -398,7 +398,6 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
>               }
>               if (s->usb_len) {
>                   DPRINTF("Deferring packet %p\n", p);
> -                usb_defer_packet(p, usb_msd_cancel_io, s);
>                   s->packet = p;
>                   ret = USB_RET_ASYNC;
>               } else {
> @@ -421,7 +420,6 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
>               if (s->data_len != 0 || len<  13)
>                   goto fail;
>               /* Waiting for SCSI write to complete.  */
> -            usb_defer_packet(p, usb_msd_cancel_io, s);
>               s->packet = p;
>               ret = USB_RET_ASYNC;
>               break;
> @@ -455,7 +453,6 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
>               }
>               if (s->usb_len) {
>                   DPRINTF("Deferring packet %p\n", p);
> -                usb_defer_packet(p, usb_msd_cancel_io, s);
>                   s->packet = p;
>                   ret = USB_RET_ASYNC;
>               } else {
> @@ -604,6 +601,7 @@ static struct USBDeviceInfo msd_info = {
>       .usb_desc       =&desc,
>       .init           = usb_msd_initfn,
>       .handle_packet  = usb_generic_handle_packet,
> +    .cancel_packet  = usb_msd_cancel_io,
>       .handle_attach  = usb_desc_attach,
>       .handle_reset   = usb_msd_handle_reset,
>       .handle_control = usb_msd_handle_control,
> diff --git a/hw/usb.c b/hw/usb.c
> index 8a9a7fc..4a39cbc 100644
> --- a/hw/usb.c
> +++ b/hw/usb.c
> @@ -345,6 +345,6 @@ void usb_packet_complete(USBDevice *dev, USBPacket *p)
>   void usb_cancel_packet(USBPacket * p)
>   {
>       assert(p->owner != NULL);
> -    p->cancel_cb(p, p->cancel_opaque);
> +    p->owner->info->cancel_packet(p->owner, p);
>       p->owner = NULL;
>   }
> diff --git a/hw/usb.h b/hw/usb.h
> index 80e8e90..9882400 100644
> --- a/hw/usb.h
> +++ b/hw/usb.h
> @@ -194,6 +194,11 @@ struct USBDeviceInfo {
>       int (*handle_packet)(USBDevice *dev, USBPacket *p);
>
>       /*
> +     * Called when a packet is canceled.
> +     */
> +    void (*cancel_packet)(USBDevice *dev, USBPacket *p);
> +
> +    /*
>        * Called when device is destroyed.
>        */
>       void (*handle_destroy)(USBDevice *dev);
> @@ -263,24 +268,12 @@ struct USBPacket {
>       int len;
>       /* Internal use by the USB layer.  */
>       USBDevice *owner;
> -    USBCallback *cancel_cb;
> -    void *cancel_opaque;
>   };
>
>   int usb_handle_packet(USBDevice *dev, USBPacket *p);
>   void usb_packet_complete(USBDevice *dev, USBPacket *p);
>   void usb_cancel_packet(USBPacket * p);
>
> -/* Defer completion of a USB packet.  The hadle_packet routine should then
> -   return USB_RET_ASYNC.  Packets that complete immediately (before
> -   handle_packet returns) should not call this method.  */
> -static inline void usb_defer_packet(USBPacket *p, USBCallback *cancel,
> -                                    void * opaque)
> -{
> -    p->cancel_cb = cancel;
> -    p->cancel_opaque = opaque;
> -}
> -
>   void usb_attach(USBPort *port, USBDevice *dev);
>   void usb_wakeup(USBDevice *dev);
>   int usb_generic_handle_packet(USBDevice *s, USBPacket *p);
> diff --git a/usb-linux.c b/usb-linux.c
> index c7e96c3..baa6574 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -335,9 +335,9 @@ static void async_complete(void *opaque)
>       }
>   }
>
> -static void async_cancel(USBPacket *p, void *opaque)
> +static void usb_host_async_cancel(USBDevice *dev, USBPacket *p)
>   {
> -    USBHostDevice *s = opaque;
> +    USBHostDevice *s = DO_UPCAST(USBHostDevice, dev, dev);
>       AsyncURB *aurb;
>
>       QLIST_FOREACH(aurb,&s->aurbs, next) {
> @@ -736,7 +736,6 @@ static int usb_host_handle_data(USBDevice *dev, USBPacket *p)
>           }
>       }
>
> -    usb_defer_packet(p, async_cancel, s);
>       return USB_RET_ASYNC;
>   }
>
> @@ -868,7 +867,6 @@ static int usb_host_handle_control(USBDevice *dev, USBPacket *p,
>           }
>       }
>
> -    usb_defer_packet(p, async_cancel, s);
>       return USB_RET_ASYNC;
>   }
>
> @@ -1197,6 +1195,7 @@ static struct USBDeviceInfo usb_host_dev_info = {
>       .qdev.size      = sizeof(USBHostDevice),
>       .init           = usb_host_initfn,
>       .handle_packet  = usb_generic_handle_packet,
> +    .cancel_packet  = usb_host_async_cancel,
>       .handle_data    = usb_host_handle_data,
>       .handle_control = usb_host_handle_control,
>       .handle_reset   = usb_host_handle_reset,