| Message ID | 20101005164029.92f21cb3.alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 5 Oct 2010 16:40:29 +0200 Alon Levy <alevy@redhat.com> wrote: > Signed-off-by: Alon Levy <alevy@redhat.com> This needs to be rebased as it now conflicts with my last series merged on master. I have some minor comments (below), but in general seems ok to me. However, I'd like to get an ACK from someone familiar with QEMU's USB before applying to the monitor's queue. Anthony? Gerd? > > --- > qemu-monitor.hx | 17 +++++++++++++++++ > sysemu.h | 1 + > vl.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 0 deletions(-) > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > index 49bcd8d..9c792a9 100644 > --- a/qemu-monitor.hx > +++ b/qemu-monitor.hx > @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove. > ETEXI > > { > + .name = "usb_detach", > + .args_type = "devname:s", > + .params = "device", > + .help = "detach 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 a1f6466..ac68863 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -183,6 +183,7 @@ 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_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 d352d18..6cfa009 100644 > --- a/vl.c > +++ b/vl.c > @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict) > } > } > > +static USBDevice *usb_device_from_bus_dot_addr(const char *devname) > +{ > + int bus_num, addr; > + const char *p; > + USBBus *bus; > + USBPort *port; > + > + if (!usb_enabled) { > + return NULL; > + } > + p = strchr(devname, '.'); > + if (!p) { > + return NULL; > + } > + bus_num = strtoul(devname, NULL, 0); > + addr = strtoul(p + 1, NULL, 0); Checking stroul()'s return would avoid nasty surprises. > + bus = usb_bus_find(bus_num); > + if (!bus) { > + return NULL; > + } > + QTAILQ_FOREACH(port, &bus->used, next) { > + if (port->dev->addr == addr) { > + break; > + } > + } > + if (!port) { > + return NULL; > + } > + return port->dev; > +} > + > +void do_usb_detach(Monitor *mon, const QDict *qdict) > +{ > + const char *devname = qdict_get_str(qdict, "devname"); > + USBDevice *dev = usb_device_from_bus_dot_addr(devname); > + > + if (dev == NULL || usb_device_detach(dev) < 0) { > + error_report("could not detach USB device '%s'", devname); > + } You can improve the error message if you check for dev and detach separately. Minor. > +} > + > /***********************************************************/ > /* PCMCIA/Cardbus */ >
How is this different than usb_del? Is it that it detaches it but does not delete the device? Regards, Anthony Liguori On 10/05/2010 09:40 AM, Alon Levy wrote: > Signed-off-by: Alon Levy<alevy@redhat.com> > > --- > qemu-monitor.hx | 17 +++++++++++++++++ > sysemu.h | 1 + > vl.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+), 0 deletions(-) > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > index 49bcd8d..9c792a9 100644 > --- a/qemu-monitor.hx > +++ b/qemu-monitor.hx > @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove. > ETEXI > > { > + .name = "usb_detach", > + .args_type = "devname:s", > + .params = "device", > + .help = "detach 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 a1f6466..ac68863 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -183,6 +183,7 @@ 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_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 d352d18..6cfa009 100644 > --- a/vl.c > +++ b/vl.c > @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict) > } > } > > +static USBDevice *usb_device_from_bus_dot_addr(const char *devname) > +{ > + int bus_num, addr; > + const char *p; > + USBBus *bus; > + USBPort *port; > + > + if (!usb_enabled) { > + return NULL; > + } > + p = strchr(devname, '.'); > + if (!p) { > + return NULL; > + } > + bus_num = strtoul(devname, NULL, 0); > + addr = strtoul(p + 1, NULL, 0); > + bus = usb_bus_find(bus_num); > + if (!bus) { > + return NULL; > + } > + QTAILQ_FOREACH(port,&bus->used, next) { > + if (port->dev->addr == addr) { > + break; > + } > + } > + if (!port) { > + return NULL; > + } > + return port->dev; > +} > + > +void do_usb_detach(Monitor *mon, const QDict *qdict) > +{ > + const char *devname = qdict_get_str(qdict, "devname"); > + USBDevice *dev = usb_device_from_bus_dot_addr(devname); > + > + if (dev == NULL || usb_device_detach(dev)< 0) { > + error_report("could not detach USB device '%s'", devname); > + } > +} > + > /***********************************************************/ > /* PCMCIA/Cardbus */ > >
----- "Anthony Liguori" <anthony@codemonkey.ws> wrote: > How is this different than usb_del? Is it that it detaches it but > does > not delete the device? > yes. There is no usb_attach command because it was harder to write (can't use the bus.addr since a detached device doesn't have them) and I didn't need it right now, my device attaches itself based on a external event. > Regards, > > Anthony Liguori > > On 10/05/2010 09:40 AM, Alon Levy wrote: > > Signed-off-by: Alon Levy<alevy@redhat.com> > > > > --- > > qemu-monitor.hx | 17 +++++++++++++++++ > > sysemu.h | 1 + > > vl.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 59 insertions(+), 0 deletions(-) > > > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > > index 49bcd8d..9c792a9 100644 > > --- a/qemu-monitor.hx > > +++ b/qemu-monitor.hx > > @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you > can remove. > > ETEXI > > > > { > > + .name = "usb_detach", > > + .args_type = "devname:s", > > + .params = "device", > > + .help = "detach 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 a1f6466..ac68863 100644 > > --- a/sysemu.h > > +++ b/sysemu.h > > @@ -183,6 +183,7 @@ 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_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 d352d18..6cfa009 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict > *qdict) > > } > > } > > > > +static USBDevice *usb_device_from_bus_dot_addr(const char > *devname) > > +{ > > + int bus_num, addr; > > + const char *p; > > + USBBus *bus; > > + USBPort *port; > > + > > + if (!usb_enabled) { > > + return NULL; > > + } > > + p = strchr(devname, '.'); > > + if (!p) { > > + return NULL; > > + } > > + bus_num = strtoul(devname, NULL, 0); > > + addr = strtoul(p + 1, NULL, 0); > > + bus = usb_bus_find(bus_num); > > + if (!bus) { > > + return NULL; > > + } > > + QTAILQ_FOREACH(port,&bus->used, next) { > > + if (port->dev->addr == addr) { > > + break; > > + } > > + } > > + if (!port) { > > + return NULL; > > + } > > + return port->dev; > > +} > > + > > +void do_usb_detach(Monitor *mon, const QDict *qdict) > > +{ > > + const char *devname = qdict_get_str(qdict, "devname"); > > + USBDevice *dev = usb_device_from_bus_dot_addr(devname); > > + > > + if (dev == NULL || usb_device_detach(dev)< 0) { > > + error_report("could not detach USB device '%s'", devname); > > + } > > +} > > + > > /***********************************************************/ > > /* PCMCIA/Cardbus */ > > > >
On 10/10/10 13:12, Alon Levy wrote: > > ----- "Anthony Liguori"<anthony@codemonkey.ws> wrote: > >> How is this different than usb_del? Is it that it detaches it but >> does >> not delete the device? > > yes. There is no usb_attach command because it was harder to write (can't > use the bus.addr since a detached device doesn't have them) and I didn't > need it right now, my device attaches itself based on a external event. Which points out a problem with this patch: It should better not use bus.addr. addr isn't fixed and even can be uninitialized. Yes, usb_del uses it (for historical reasons). But we better should not use it in new code. Better use the device id (like device_del). Which will work for usb_attach too. Next question: What is the use case? attach/detach is used by devices internally. usb-host does attach/detach when devices get plugged-in and -out on the host. The ccid device does simliar things on vsclient connect/disconnect. So toggeling the attach state via monitor easily could have unwanted side effects ... cheers, Gerd
----- "Gerd Hoffmann" <kraxel@redhat.com> wrote: > On 10/10/10 13:12, Alon Levy wrote: > > > > ----- "Anthony Liguori"<anthony@codemonkey.ws> wrote: > > > >> How is this different than usb_del? Is it that it detaches it but > >> does > >> not delete the device? > > > > yes. There is no usb_attach command because it was harder to write > (can't > > use the bus.addr since a detached device doesn't have them) and I > didn't > > need it right now, my device attaches itself based on a external > event. > > Which points out a problem with this patch: It should better not use > > bus.addr. addr isn't fixed and even can be uninitialized. Yes, > usb_del > uses it (for historical reasons). But we better should not use it in > > new code. Better use the device id (like device_del). Which will > work > for usb_attach too. > > Next question: What is the use case? attach/detach is used by > devices > internally. usb-host does attach/detach when devices get plugged-in > and > -out on the host. The ccid device does simliar things on vsclient > connect/disconnect. So toggeling the attach state via monitor easily > debugging. naturally when developing the ccid I had cases where I'd rather detach the device then bring down qemu. since there is no way currently to add/remove chardev's from monitor, removing/adding a device is not enough to reset a device state to the state right after start. > could have unwanted side effects ... > > cheers, > Gerd
diff --git a/qemu-monitor.hx b/qemu-monitor.hx index 49bcd8d..9c792a9 100644 --- a/qemu-monitor.hx +++ b/qemu-monitor.hx @@ -711,6 +711,23 @@ command @code{info usb} to see the devices you can remove. ETEXI { + .name = "usb_detach", + .args_type = "devname:s", + .params = "device", + .help = "detach 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 a1f6466..ac68863 100644 --- a/sysemu.h +++ b/sysemu.h @@ -183,6 +183,7 @@ 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_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 d352d18..6cfa009 100644 --- a/vl.c +++ b/vl.c @@ -891,6 +891,47 @@ void do_usb_del(Monitor *mon, const QDict *qdict) } } +static USBDevice *usb_device_from_bus_dot_addr(const char *devname) +{ + int bus_num, addr; + const char *p; + USBBus *bus; + USBPort *port; + + if (!usb_enabled) { + return NULL; + } + p = strchr(devname, '.'); + if (!p) { + return NULL; + } + bus_num = strtoul(devname, NULL, 0); + addr = strtoul(p + 1, NULL, 0); + bus = usb_bus_find(bus_num); + if (!bus) { + return NULL; + } + QTAILQ_FOREACH(port, &bus->used, next) { + if (port->dev->addr == addr) { + break; + } + } + if (!port) { + return NULL; + } + return port->dev; +} + +void do_usb_detach(Monitor *mon, const QDict *qdict) +{ + const char *devname = qdict_get_str(qdict, "devname"); + USBDevice *dev = usb_device_from_bus_dot_addr(devname); + + if (dev == NULL || usb_device_detach(dev) < 0) { + error_report("could not detach USB device '%s'", devname); + } +} + /***********************************************************/ /* PCMCIA/Cardbus */
Signed-off-by: Alon Levy <alevy@redhat.com> --- qemu-monitor.hx | 17 +++++++++++++++++ sysemu.h | 1 + vl.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 0 deletions(-)