[Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v2)

[Qemu-devel] [PATCH 0/3] add usb_detach and usb_attach (v2)

[Alon Levy]

This patchset uses id like device_del for attaching/detaching usb
devices. The first two patches ready the way:
 1. makes qdev_find_recursive non static and in qdev.h
 2. adds a usb_device_by_id which goes over the usb buses calling
  qdev_find_recursive
 3. adds the commands that use usb_device_by_id

Alon Levy (3):
  qdev: make qdev_find_recursive public
  usb: add public usb_device_by_id
  monitor: add usb_attach and usb_detach

 hmp-commands.hx |   34 ++++++++++++++++++++++++++++++++++
 hw/qdev.c       |    2 +-
 hw/qdev.h       |    1 +
 hw/usb-bus.c    |   16 ++++++++++++++++
 hw/usb.h        |    1 +
 sysemu.h        |    2 ++
 vl.c            |   31 +++++++++++++++++++++++++++++++
 7 files changed, 86 insertions(+), 1 deletions(-)

-- 
1.7.3.1

[Qemu-devel] [PATCH 1/3] qdev: make qdev_find_recursive public

[Alon Levy]

---
 hw/qdev.c |    2 +-
 hw/qdev.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..d669a9d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -477,7 +477,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static DeviceState *qdev_find_recursive(BusState *bus, const char *id)
+DeviceState *qdev_find_recursive(BusState *bus, const char *id)
 {
     DeviceState *dev, *ret;
     BusState *child;
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..214066e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -177,6 +177,7 @@ void qbus_create_inplace(BusState *bus, BusInfo *info,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(BusInfo *info, DeviceState *parent, const char *name);
 void qbus_free(BusState *bus);
+DeviceState *qdev_find_recursive(BusState *bus, const char *id);
 
 #define FROM_QBUS(type, dev) DO_UPCAST(type, qbus, dev)
 
-- 
1.7.3.1

[Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Alon Levy]

---
 hw/usb-bus.c |   16 ++++++++++++++++
 hw/usb.h     |    1 +
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index b692503..d732bd3 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -189,6 +189,22 @@ int usb_device_detach(USBDevice *dev)
     return 0;
 }
 
+USBDevice *usb_device_by_id(const char* id)
+{
+    USBBus *bus;
+    DeviceState *qdev;
+    USBDevice *dev;
+
+    QTAILQ_FOREACH(bus, &busses, next) {
+        qdev = qdev_find_recursive(&bus->qbus, id);
+        if (qdev != NULL) {
+            dev = DO_UPCAST(USBDevice, qdev, qdev);
+            return dev;
+        }
+    }
+    return NULL;
+}
+
 int usb_device_delete_addr(int busnr, int addr)
 {
     USBBus *bus;
diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..e70fccd 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -317,6 +317,7 @@ void usb_unregister_port(USBBus *bus, USBPort *port);
 int usb_device_attach(USBDevice *dev);
 int usb_device_detach(USBDevice *dev);
 int usb_device_delete_addr(int busnr, int addr);
+USBDevice *usb_device_by_id(const char* id);
 
 static inline USBBus *usb_bus_from_device(USBDevice *d)
 {
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Gerd Hoffmann]

> +USBDevice *usb_device_by_id(const char* id)
> +{
> +    USBBus *bus;
> +    DeviceState *qdev;
> +    USBDevice *dev;
> +
> +    QTAILQ_FOREACH(bus,&busses, next) {
> +        qdev = qdev_find_recursive(&bus->qbus, id);
> +        if (qdev != NULL) {
> +            dev = DO_UPCAST(USBDevice, qdev, qdev);
> +            return dev;
> +        }
> +    }

You don't need qdev_find_recursive here.  Have a look at the usb_info() 
code to see how to loop over all usb devices.  Then compare id with 
USBDevice->qdev.id.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Alon Levy]

----- "Gerd Hoffmann" <kraxel@redhat.com> wrote:

> > +USBDevice *usb_device_by_id(const char* id)
> > +{
> > +    USBBus *bus;
> > +    DeviceState *qdev;
> > +    USBDevice *dev;
> > +
> > +    QTAILQ_FOREACH(bus,&busses, next) {
> > +        qdev = qdev_find_recursive(&bus->qbus, id);
> > +        if (qdev != NULL) {
> > +            dev = DO_UPCAST(USBDevice, qdev, qdev);
> > +            return dev;
> > +        }
> > +    }
> 
> You don't need qdev_find_recursive here.  Have a look at the
> usb_info() 
> code to see how to loop over all usb devices.  Then compare id with 
> USBDevice->qdev.id.
> 
> cheers,
>    Gerd

There is no problem to loop over all usb devices. But first of all I don't want to loop on used, since then I miss any detached devices, so I actually do want the same behavior of qdev_find_recursive, and since it's already available, why rewrite it in a different compilation unit?

Alon

Re: [Qemu-devel] [PATCH 2/3] usb: add public usb_device_by_id

[Gerd Hoffmann]

   Hi,

> There is no problem to loop over all usb devices. But first of all I
> don't want to loop on used, since then I miss any detached devices,
> so I actually do want the same behavior of qdev_find_recursive, and
> since it's already available, why rewrite it in a different
> compilation unit?

Point.  ACK then.

cheers,
   Gerd

[Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Alon Levy]

---
 hmp-commands.hx |   34 ++++++++++++++++++++++++++++++++++
 sysemu.h        |    2 ++
 vl.c            |   31 +++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..660205c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -517,6 +517,40 @@ command @code{info usb} to see the devices you can remove.
 ETEXI
 
     {
+        .name       = "usb_attach",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "attach USB device 'bus.addr'",
+        .mhandler.cmd = do_usb_attach,
+    },
+
+STEXI
+@item usb_attach @var{devname}
+@findex usb_attach
+
+Attach the USB device @var{devname} to the QEMU virtual USB
+hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
+command @code{info usb} to see the devices you can attach.
+ETEXI
+
+    {
+        .name       = "usb_detach",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "remove USB device 'bus.addr'",
+        .mhandler.cmd = do_usb_detach,
+    },
+
+STEXI
+@item usb_detach @var{devname}
+@findex usb_detach
+
+Detach the USB device @var{devname} from the QEMU virtual USB
+hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
+command @code{info usb} to see the devices you can detach.
+ETEXI
+
+    {
         .name       = "device_add",
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
diff --git a/sysemu.h b/sysemu.h
index b81a70e..1dc0e58 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -182,6 +182,8 @@ extern struct soundhw soundhw[];
 
 void do_usb_add(Monitor *mon, const QDict *qdict);
 void do_usb_del(Monitor *mon, const QDict *qdict);
+void do_usb_attach(Monitor *mon, const QDict *qdict);
+void do_usb_detach(Monitor *mon, const QDict *qdict);
 void usb_info(Monitor *mon);
 
 void rtc_change_mon_event(struct tm *tm);
diff --git a/vl.c b/vl.c
index df414ef..35db6c8 100644
--- a/vl.c
+++ b/vl.c
@@ -894,6 +894,37 @@ void do_usb_del(Monitor *mon, const QDict *qdict)
     }
 }
 
+void do_usb_attach(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    USBDevice *dev;
+
+    dev = usb_device_by_id(id);
+
+    if (dev == NULL) {
+        error_report("no such USB device '%s'", id);
+        return;
+    }
+    if (usb_device_attach(dev) < 0) {
+        error_report("could not attach USB device '%s'", id);
+    }
+}
+
+void do_usb_detach(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    USBDevice *dev;
+
+    dev = usb_device_by_id(id);
+    if (dev == NULL) {
+        error_report("no such USB device '%s'", id);
+        return;
+    }
+    if (usb_device_detach(dev) < 0) {
+        error_report("could not detach USB device '%s'", id);
+    }
+}
+
 /***********************************************************/
 /* PCMCIA/Cardbus */
 
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Gerd Hoffmann]

   Hi,

> +        .help       = "attach USB device 'bus.addr'",

> +@item usb_attach @var{devname}

/me sees a mismatch here.

There is still the use case question.  Also note that this might have 
unwanted side effects when drivers automagically attach/detach devices 
like usb-host.

Having this purely for debugging/troubleshooting purposes would be fine 
with me, but the documentation should clearly say so.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Luiz Capitulino]

On Tue, 19 Oct 2010 15:35:01 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>    Hi,
> 
> > +        .help       = "attach USB device 'bus.addr'",
> 
> > +@item usb_attach @var{devname}
> 
> /me sees a mismatch here.
> 
> There is still the use case question.  Also note that this might have 
> unwanted side effects when drivers automagically attach/detach devices 
> like usb-host.

Alon, did you check this? I hope it doesn't blowup.

> Having this purely for debugging/troubleshooting purposes would be fine 
> with me, but the documentation should clearly say so.

Is this useful in production or only in the development of new devices? If
it's the letter, then I would even prefer enabling them only when DEBUG
is enabled.

Re: [Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Gerd Hoffmann]

On 10/20/10 18:24, Luiz Capitulino wrote:
> On Tue, 19 Oct 2010 15:35:01 +0200
> Gerd Hoffmann<kraxel@redhat.com>  wrote:
>
>>     Hi,
>>
>>> +        .help       = "attach USB device 'bus.addr'",
>>
>>> +@item usb_attach @var{devname}
>>
>> /me sees a mismatch here.
>>
>> There is still the use case question.  Also note that this might have
>> unwanted side effects when drivers automagically attach/detach devices
>> like usb-host.
>
> Alon, did you check this? I hope it doesn't blowup.
>
>> Having this purely for debugging/troubleshooting purposes would be fine
>> with me, but the documentation should clearly say so.
>
> Is this useful in production or only in the development of new devices?

I'd say more a development tool.

> If
> it's the letter, then I would even prefer enabling them only when DEBUG
> is enabled.

Makes sense indeed.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Alon Levy]

On Thu, Oct 21, 2010 at 12:00:12PM +0200, Gerd Hoffmann wrote:
> On 10/20/10 18:24, Luiz Capitulino wrote:
> >On Tue, 19 Oct 2010 15:35:01 +0200
> >Gerd Hoffmann<kraxel@redhat.com>  wrote:
> >
> >>    Hi,
> >>
> >>>+        .help       = "attach USB device 'bus.addr'",
> >>
> >>>+@item usb_attach @var{devname}
> >>
> >>/me sees a mismatch here.
> >>
> >>There is still the use case question.  Also note that this might have
> >>unwanted side effects when drivers automagically attach/detach devices
> >>like usb-host.
> >
> >Alon, did you check this? I hope it doesn't blowup.
> >
> >>Having this purely for debugging/troubleshooting purposes would be fine
> >>with me, but the documentation should clearly say so.
> >
> >Is this useful in production or only in the development of new devices?
> 
> I'd say more a development tool.
> 
> >If
> >it's the letter, then I would even prefer enabling them only when DEBUG
> >is enabled.
> 
> Makes sense indeed.

I've sent a third version with fixes for the args_type you pointed out and the suggested default disabled behavior (I also added a flag to enable just this command, but made sure --enable-debug enables it too).

> 
> cheers,
>   Gerd

Re: [Qemu-devel] [PATCH 3/3] monitor: add usb_attach and usb_detach

[Alon Levy]

On Wed, Oct 20, 2010 at 02:24:20PM -0200, Luiz Capitulino wrote:
> On Tue, 19 Oct 2010 15:35:01 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >    Hi,
> > 
> > > +        .help       = "attach USB device 'bus.addr'",
> > 
> > > +@item usb_attach @var{devname}
> > 
> > /me sees a mismatch here.
> > 
> > There is still the use case question.  Also note that this might have 
> > unwanted side effects when drivers automagically attach/detach devices 
> > like usb-host.
> 
> Alon, did you check this? I hope it doesn't blowup.

I just did, I created
bus
 usb-serial
 usb-hub id=hub1
  usb-serial
  .. (6 more)
  usb-hub id=hub2
   usb-serial
   (6 more)

(verified the guest sees this tree using lsusb -t)

I did usb_detach hub1, and as expected only the first usb-serial remained.
I then did a usb_attach hub1, and no devices appeared. A second usb_attach
gets the expected warning (you tried to attach a device twice).

So It doesn't blow up, but an info qtree showed it's because the rest of
the devices (those below hub1) were still attached. So I think the detach
should take care to detach anything below. If after that attach still won't
cause attach then hub attaches need fixing as well (it should be just causing
attach for devices under it, since the whole protocol for an attach is already
correctly implemented).

> 
> > Having this purely for debugging/troubleshooting purposes would be fine 
> > with me, but the documentation should clearly say so.
> 
> Is this useful in production or only in the development of new devices? If
> it's the letter, then I would even prefer enabling them only when DEBUG
> is enabled.
>