[17/18] usb: move cancel callback to USBDeviceInfo

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(-)

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,

Hi,

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

Good spotting, this is indeed a issue which needs fixing.  It isn't 
introduced by this patch though, it exists even without the usb patch queue.

usb-msd.c passes a USBDevice pointer directly as opaque.  The 
usb-linux.c callback function assumes it can dereference aurb->hdev just 
fine.  Both will hit free'ed memory in case the device is unplugged 
while a async packet is in flight.

cheers,
   Gerd

Hi,

On 05/23/2011 04:34 PM, Gerd Hoffmann wrote:
> Hi,
>
>> 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.
>
> Good spotting, this is indeed a issue which needs fixing. It isn't introduced by this patch though, it exists even without the usb patch queue.
>
> usb-msd.c passes a USBDevice pointer directly as opaque. The usb-linux.c callback function assumes it can dereference aurb->hdev just fine.

Ah, that is no good, my usb network redir device code uses aurb's similar too linux.c, but on
device-destroy walks its list of pending aurbs, sends a cancel to the host-os, and
sets aurb->hdev to null, and the async cancel checks for aurb->hdev being NULL and
in that case only frees the aurb and does nothing else.

 > Both will hit free'ed memory in case the device is unplugged while a async packet is in flight.

Yep, linux.c could be fixed the same way as my usb net redir device code. But I like
the patch you just send better. It looks incomplete though, I'll give more details
in a reply to the patch it self.

Regards,

Hans