[Qemu-devel] [PULL 0/6] usb patch queue

[Qemu-devel] [PULL 0/6] usb patch queue

[Gerd Hoffmann]

  Hi,

Here comes the usb patch queue, with a bugfix collection and a
documentation update.

please pull,
  Gerd

The following changes since commit 76d20ea0f1b26ebd5da2f5fb2fdf3250cde887bb:

  Merge remote-tracking branch 'armbru/tags/pull-qapi-2017-05-04-v3' into staging (2017-05-09 15:49:14 -0400)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-usb-20170512-1

for you to fetch changes up to aa612b364ecbe1dc034efcabb04526f24e56c145:

  hw/usb/dev-serial: Do not try to set vendorid or productid properties (2017-05-12 12:30:23 +0200)

----------------------------------------------------------------
usb: bugfixes, doc update

----------------------------------------------------------------
Gerd Hoffmann (1):
      usb-redir: fix stack overflow in usbredir_log_data

Ladi Prosek (3):
      xhci: fix logging
      usb-hub: clear PORT_STAT_SUSPEND on wakeup
      xhci: relax link check

Thomas Huth (2):
      qemu-doc: Update to use the new way of attaching USB devices
      hw/usb/dev-serial: Do not try to set vendorid or productid properties

 docs/qdev-device-use.txt |  6 ++--
 hw/usb/dev-hub.c         |  1 +
 hw/usb/dev-serial.c      | 24 ++++---------
 hw/usb/hcd-xhci.c        |  9 ++---
 hw/usb/redirect.c        | 13 +------
 qemu-doc.texi            | 91 +++++++++++++++++++++++++++---------------------
 6 files changed, 64 insertions(+), 80 deletions(-)

[Qemu-devel] [PULL 1/6] qemu-doc: Update to use the new way of attaching USB devices

[Gerd Hoffmann]

From: Thomas Huth <thuth@redhat.com>

The preferred way of adding USB devices is via "-device" and
"device_add" nowadays, so let's start to get rid of "-usbdevice"
and "usb_add" in the documentation. While we're at it, also
add the new USB devices there which have been added to QEMU
during the last years, and get rid of the old "vendorid" and
"productid" parameters of "-usbdevice serial" which have been
removed in QEMU version 0.14.0 already.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 1494256429-31720-1-git-send-email-thuth@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 docs/qdev-device-use.txt |  6 ++--
 qemu-doc.texi            | 91 +++++++++++++++++++++++++++---------------------
 2 files changed, 53 insertions(+), 44 deletions(-)

diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt
index b059405e0e..4274fe9f25 100644
--- a/docs/qdev-device-use.txt
+++ b/docs/qdev-device-use.txt
@@ -182,15 +182,13 @@ The appropriate DEVNAME depends on the machine type.  For type "pc":
 
   This lets you control I/O ports and IRQs.
 
-* -usbdevice serial:vendorid=VID,productid=PRID becomes
-  -device usb-serial,vendorid=VID,productid=PRID
+* -usbdevice serial::chardev becomes -device usb-serial,chardev=dev.
 
 * -usbdevice braille doesn't support LEGACY-CHARDEV syntax.  It always
   uses "braille".  With -device, this useful default is gone, so you
   have to use something like
 
-  -device usb-braille,chardev=braille,vendorid=VID,productid=PRID
-  -chardev braille,id=braille
+  -device usb-braille,chardev=braille -chardev braille,id=braille
 
 * -virtioconsole becomes
   -device virtio-serial-pci,class=C,vectors=V,ioeventfd=IOEVENTFD,max_ports=N
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 794ab4a080..59d0ccb582 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -182,7 +182,7 @@ Gravis Ultrasound GF1 sound card
 @item
 CS4231A compatible sound card
 @item
-PCI UHCI USB controller and a virtual USB hub.
+PCI UHCI, OHCI, EHCI or XHCI USB controller and a virtual USB-1.1 hub.
 @end itemize
 
 SMP is supported with up to 255 CPUs.
@@ -1357,10 +1357,10 @@ monitor (@pxref{pcsys_keys}).
 @node pcsys_usb
 @section USB emulation
 
-QEMU emulates a PCI UHCI USB controller. You can virtually plug
-virtual USB devices or real host USB devices (experimental, works only
-on Linux hosts).  QEMU will automatically create and connect virtual USB hubs
-as necessary to connect multiple USB devices.
+QEMU can emulate a PCI UHCI, OHCI, EHCI or XHCI USB controller. You can
+plug virtual USB devices or real host USB devices (only works with certain
+host operating systems). QEMU will automatically create and connect virtual
+USB hubs as necessary to connect multiple USB devices.
 
 @menu
 * usb_devices::
@@ -1369,53 +1369,64 @@ as necessary to connect multiple USB devices.
 @node usb_devices
 @subsection Connecting USB devices
 
-USB devices can be connected with the @option{-usbdevice} commandline option
-or the @code{usb_add} monitor command.  Available devices are:
+USB devices can be connected with the @option{-device usb-...} command line
+option or the @code{device_add} monitor command. Available devices are:
 
 @table @code
-@item mouse
+@item usb-mouse
 Virtual Mouse.  This will override the PS/2 mouse emulation when activated.
-@item tablet
+@item usb-tablet
 Pointer device that uses absolute coordinates (like a touchscreen).
 This means QEMU is able to report the mouse position without having
 to grab the mouse.  Also overrides the PS/2 mouse emulation when activated.
-@item disk:@var{file}
-Mass storage device based on @var{file} (@pxref{disk_images})
-@item host:@var{bus.addr}
-Pass through the host device identified by @var{bus.addr}
-(Linux only)
-@item host:@var{vendor_id:product_id}
-Pass through the host device identified by @var{vendor_id:product_id}
-(Linux only)
-@item wacom-tablet
+@item usb-storage,drive=@var{drive_id}
+Mass storage device backed by @var{drive_id} (@pxref{disk_images})
+@item usb-uas
+USB attached SCSI device, see
+@url{http://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/usb-storage.txt,usb-storage.txt}
+for details
+@item usb-bot
+Bulk-only transport storage device, see
+@url{http://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/usb-storage.txt,usb-storage.txt}
+for details here, too
+@item usb-mtp,x-root=@var{dir}
+Media transfer protocol device, using @var{dir} as root of the file tree
+that is presented to the guest.
+@item usb-host,hostbus=@var{bus},hostaddr=@var{addr}
+Pass through the host device identified by @var{bus} and @var{addr}
+@item usb-host,vendorid=@var{vendor},productid=@var{product}
+Pass through the host device identified by @var{vendor} and @var{product} ID
+@item usb-wacom-tablet
 Virtual Wacom PenPartner tablet.  This device is similar to the @code{tablet}
 above but it can be used with the tslib library because in addition to touch
 coordinates it reports touch pressure.
-@item keyboard
+@item usb-kbd
 Standard USB keyboard.  Will override the PS/2 keyboard (if present).
-@item serial:[vendorid=@var{vendor_id}][,product_id=@var{product_id}]:@var{dev}
+@item usb-serial,chardev=@var{id}
 Serial converter. This emulates an FTDI FT232BM chip connected to host character
-device @var{dev}. The available character devices are the same as for the
-@code{-serial} option. The @code{vendorid} and @code{productid} options can be
-used to override the default 0403:6001. For instance,
-@example
-usb_add serial:productid=FA00:tcp:192.168.0.2:4444
-@end example
-will connect to tcp port 4444 of ip 192.168.0.2, and plug that to the virtual
-serial converter, faking a Matrix Orbital LCD Display (USB ID 0403:FA00).
-@item braille
+device @var{id}.
+@item usb-braille,chardev=@var{id}
 Braille device.  This will use BrlAPI to display the braille output on a real
-or fake device.
-@item net:@var{options}
-Network adapter that supports CDC ethernet and RNDIS protocols.  @var{options}
-specifies NIC options as with @code{-net nic,}@var{options} (see description).
+or fake device referenced by @var{id}.
+@item usb-net[,netdev=@var{id}]
+Network adapter that supports CDC ethernet and RNDIS protocols.  @var{id}
+specifies a netdev defined with @code{-netdev @dots{},id=@var{id}}.
 For instance, user-mode networking can be used with
 @example
-qemu-system-i386 [...OPTIONS...] -net user,vlan=0 -usbdevice net:vlan=0
+qemu-system-i386 [...] -netdev user,id=net0 -device usb-net,netdev=net0
 @end example
-Currently this cannot be used in machines that support PCI NICs.
-@item bt[:@var{hci-type}]
-Bluetooth dongle whose type is specified in the same format as with
+@item usb-ccid
+Smartcard reader device
+@item usb-audio
+USB audio device
+@item usb-bt-dongle
+Bluetooth dongle for the transport layer of HCI. It is connected to HCI
+scatternet 0 by default (corresponds to @code{-bt hci,vlan=0}).
+Note that the syntax for the @code{-device usb-bt-dongle} option is not as
+useful yet as it was with the legacy @code{-usbdevice} option. So to
+configure an USB bluetooth device, you might need to use
+"@code{-usbdevice bt}[:@var{hci-type}]" instead. This configures a
+bluetooth dongle whose type is specified in the same format as with
 the @option{-bt hci} option, @pxref{bt-hcis,,allowed HCI types}.  If
 no type is given, the HCI logic corresponds to @code{-bt hci,vlan=0}.
 This USB device implements the USB Transport Layer of HCI.  Example
@@ -1460,11 +1471,11 @@ hubs, it won't work).
 
 @item Add the device in QEMU by using:
 @example
-usb_add host:1234:5678
+device_add usb-host,vendorid=0x1234,productid=0x5678
 @end example
 
-Normally the guest OS should report that a new USB device is
-plugged. You can use the option @option{-usbdevice} to do the same.
+Normally the guest OS should report that a new USB device is plugged.
+You can use the option @option{-device usb-host,...} to do the same.
 
 @item Now you can try to use the host USB device in QEMU.
 
-- 
2.9.3

[Qemu-devel] [PULL 2/6] usb-redir: fix stack overflow in usbredir_log_data

[Gerd Hoffmann]

Don't reinvent a broken wheel, just use the hexdump function we have.

Impact: low, broken code doesn't run unless you have debug logging
enabled.

Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-id: 20170509110128.27261-1-kraxel@redhat.com
---
 hw/usb/redirect.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index b001a27f05..ad5ef783a6 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -229,21 +229,10 @@ static void usbredir_log(void *priv, int level, const char *msg)
 static void usbredir_log_data(USBRedirDevice *dev, const char *desc,
     const uint8_t *data, int len)
 {
-    int i, j, n;
-
     if (dev->debug < usbredirparser_debug_data) {
         return;
     }
-
-    for (i = 0; i < len; i += j) {
-        char buf[128];
-
-        n = sprintf(buf, "%s", desc);
-        for (j = 0; j < 8 && i + j < len; j++) {
-            n += sprintf(buf + n, " %02X", data[i + j]);
-        }
-        error_report("%s", buf);
-    }
+    qemu_hexdump((char *)data, stderr, desc, len);
 }
 
 /*
-- 
2.9.3

[Qemu-devel] [PULL 3/6] xhci: fix logging

[Gerd Hoffmann]

From: Ladi Prosek <lprosek@redhat.com>

slotid and epid were deleted from XHCITransfer in commit d6fcb29.
Also deleting one unused forward declaration.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Message-id: 20170511125314.24549-2-lprosek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index a2d3143bf4..d3d47bf925 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -1790,9 +1790,6 @@ static void xhci_stall_ep(XHCITransfer *xfer)
     }
 }
 
-static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer,
-                       XHCIEPContext *epctx);
-
 static int xhci_setup_packet(XHCITransfer *xfer)
 {
     USBEndpoint *ep;
@@ -1806,7 +1803,7 @@ static int xhci_setup_packet(XHCITransfer *xfer)
         ep = xhci_epid_to_usbep(xfer->epctx);
         if (!ep) {
             DPRINTF("xhci: slot %d has no device\n",
-                    xfer->slotid);
+                    xfer->epctx->slotid);
             return -1;
         }
     }
@@ -1980,7 +1977,7 @@ static int xhci_submit(XHCIState *xhci, XHCITransfer *xfer, XHCIEPContext *epctx
 {
     uint64_t mfindex;
 
-    DPRINTF("xhci_submit(slotid=%d,epid=%d)\n", xfer->slotid, xfer->epid);
+    DPRINTF("xhci_submit(slotid=%d,epid=%d)\n", epctx->slotid, epctx->epid);
 
     xfer->in_xfer = epctx->type>>2;
 
-- 
2.9.3

[Qemu-devel] [PULL 4/6] usb-hub: clear PORT_STAT_SUSPEND on wakeup

[Gerd Hoffmann]

From: Ladi Prosek <lprosek@redhat.com>

The spec says:

  Suspend: (PORT_SUSPEND) This field indicates whether or not the device
  on this port is suspended. Setting this field causes the device to
  suspend by not propagating bus traffic downstream. This field may be
  reset by a request or by resume signaling from the device attached to
  the port.

I can't find any specific statement like "the PORT_SUSPEND field is reset
automatically on remote wakeup", but without this patch, the only way to
reset it is via the ClearPortFeature request so the ".. or by resume
signaling from the device" clause is clearly not implemented on the remote
wakeup path.

The default xhci Windows driver does not issue the ClearPortFeature request
and suspended devices attached to a hub don't properly get out of the
suspended state. Interestingly, the default uhci Windows driver *does*
issue the ClearPortFeature request and does not exhibit this problem.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Message-id: 20170511125314.24549-3-lprosek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-hub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
index 9fe7333946..47b7519910 100644
--- a/hw/usb/dev-hub.c
+++ b/hw/usb/dev-hub.c
@@ -208,6 +208,7 @@ static void usb_hub_wakeup(USBPort *port1)
     USBHubPort *port = &s->ports[port1->index];
 
     if (port->wPortStatus & PORT_STAT_SUSPEND) {
+        port->wPortStatus &= ~PORT_STAT_SUSPEND;
         port->wPortChange |= PORT_STAT_C_SUSPEND;
         usb_wakeup(s->intr, 0);
     }
-- 
2.9.3

[Qemu-devel] [PULL 5/6] xhci: relax link check

[Gerd Hoffmann]

From: Ladi Prosek <lprosek@redhat.com>

The strict td link limit added by commit "05f43d4 xhci: limit the
number of link trbs we are willing to process" causes problems with
Windows guests. Let's raise the limit.

This change is analogous to:

  commit ab6b1105a2259c7072905887f71caa850ce63190
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Tue Mar 7 09:40:18 2017 +0100

      ohci: relax link check

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Message-id: 20170512102100.22675-1-lprosek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index d3d47bf925..77d8e1137a 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -50,7 +50,7 @@
 /* Very pessimistic, let's hope it's enough for all cases */
 #define EV_QUEUE (((3 * 24) + 16) * MAXSLOTS)
 
-#define TRB_LINK_LIMIT  4
+#define TRB_LINK_LIMIT  32
 #define COMMAND_LIMIT   256
 #define TRANSFER_LIMIT  256
 
-- 
2.9.3

[Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Gerd Hoffmann]

From: Thomas Huth <thuth@redhat.com>

When starting QEMU with the legacy USB serial device like this:

 qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio

it currently aborts since the vendorid property does not exist
anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):

 Unexpected error in object_property_find() at qemu/qom/object.c:1008:
 qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
                     '.vendorid' not found
 Aborted (core dumped)

Fix this crash by issuing a more friendly error message instead
(and simplify the code also a little bit this way).

Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-serial.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 6d5137383b..83a4f0e6fb 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
 {
     USBDevice *dev;
     Chardev *cdrv;
-    uint32_t vendorid = 0, productid = 0;
     char label[32];
     static int index;
 
     while (*filename && *filename != ':') {
         const char *p;
-        char *e;
+
         if (strstart(filename, "vendorid=", &p)) {
-            vendorid = strtol(p, &e, 16);
-            if (e == p || (*e && *e != ',' && *e != ':')) {
-                error_report("bogus vendor ID %s", p);
-                return NULL;
-            }
-            filename = e;
+            error_report("vendorid is not supported anymore");
+            return NULL;
         } else if (strstart(filename, "productid=", &p)) {
-            productid = strtol(p, &e, 16);
-            if (e == p || (*e && *e != ',' && *e != ':')) {
-                error_report("bogus product ID %s", p);
-                return NULL;
-            }
-            filename = e;
+            error_report("productid is not supported anymore");
+            return NULL;
         } else {
             error_report("unrecognized serial USB option %s", filename);
             return NULL;
@@ -554,10 +545,7 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
 
     dev = usb_create(bus, "usb-serial");
     qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
-    if (vendorid)
-        qdev_prop_set_uint16(&dev->qdev, "vendorid", vendorid);
-    if (productid)
-        qdev_prop_set_uint16(&dev->qdev, "productid", productid);
+
     return dev;
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Paolo Bonzini]

On 12/05/2017 14:21, Gerd Hoffmann wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> When starting QEMU with the legacy USB serial device like this:
> 
>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
> 
> it currently aborts since the vendorid property does not exist
> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
> 
>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>                      '.vendorid' not found
>  Aborted (core dumped)
> 
> Fix this crash by issuing a more friendly error message instead
> (and simplify the code also a little bit this way).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-serial.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 6d5137383b..83a4f0e6fb 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>  {
>      USBDevice *dev;
>      Chardev *cdrv;
> -    uint32_t vendorid = 0, productid = 0;
>      char label[32];
>      static int index;
>  
>      while (*filename && *filename != ':') {
>          const char *p;
> -        char *e;
> +
>          if (strstart(filename, "vendorid=", &p)) {
> -            vendorid = strtol(p, &e, 16);
> -            if (e == p || (*e && *e != ',' && *e != ':')) {
> -                error_report("bogus vendor ID %s", p);
> -                return NULL;
> -            }
> -            filename = e;
> +            error_report("vendorid is not supported anymore");
> +            return NULL;
>          } else if (strstart(filename, "productid=", &p)) {
> -            productid = strtol(p, &e, 16);
> -            if (e == p || (*e && *e != ',' && *e != ':')) {
> -                error_report("bogus product ID %s", p);
> -                return NULL;
> -            }
> -            filename = e;
> +            error_report("productid is not supported anymore");
> +            return NULL;
>          } else {
>              error_report("unrecognized serial USB option %s", filename);
>              return NULL;

All breanches of the "if" now return NULL, so the "while" loop in turn
can become an

    if (*filename && *filename != ':') {
    }

and the "while (*filename == ',')" subloop can go away, replaced by just
"return NULL".

Even better, the "if (!*filename)" if just below can be moved first.

Paolo
> @@ -554,10 +545,7 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>  
>      dev = usb_create(bus, "usb-serial");
>      qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
> -    if (vendorid)
> -        qdev_prop_set_uint16(&dev->qdev, "vendorid", vendorid);
> -    if (productid)
> -        qdev_prop_set_uint16(&dev->qdev, "productid", productid);
> +
>      return dev;
>  }
>  
> 

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Thomas Huth]

On 18.05.2017 14:00, Paolo Bonzini wrote:
> 
> 
> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> When starting QEMU with the legacy USB serial device like this:
>>
>>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
>>
>> it currently aborts since the vendorid property does not exist
>> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
>>
>>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>>                      '.vendorid' not found
>>  Aborted (core dumped)
>>
>> Fix this crash by issuing a more friendly error message instead
>> (and simplify the code also a little bit this way).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  hw/usb/dev-serial.c | 24 ++++++------------------
>>  1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>> index 6d5137383b..83a4f0e6fb 100644
>> --- a/hw/usb/dev-serial.c
>> +++ b/hw/usb/dev-serial.c
>> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>>  {
>>      USBDevice *dev;
>>      Chardev *cdrv;
>> -    uint32_t vendorid = 0, productid = 0;
>>      char label[32];
>>      static int index;
>>  
>>      while (*filename && *filename != ':') {
>>          const char *p;
>> -        char *e;
>> +
>>          if (strstart(filename, "vendorid=", &p)) {
>> -            vendorid = strtol(p, &e, 16);
>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>> -                error_report("bogus vendor ID %s", p);
>> -                return NULL;
>> -            }
>> -            filename = e;
>> +            error_report("vendorid is not supported anymore");
>> +            return NULL;
>>          } else if (strstart(filename, "productid=", &p)) {
>> -            productid = strtol(p, &e, 16);
>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>> -                error_report("bogus product ID %s", p);
>> -                return NULL;
>> -            }
>> -            filename = e;
>> +            error_report("productid is not supported anymore");
>> +            return NULL;
>>          } else {
>>              error_report("unrecognized serial USB option %s", filename);
>>              return NULL;
> 
> All breanches of the "if" now return NULL, so the "while" loop in turn
> can become an
> 
>     if (*filename && *filename != ':') {
>     }
> 
> and the "while (*filename == ',')" subloop can go away, replaced by just
> "return NULL".
> 
> Even better, the "if (!*filename)" if just below can be moved first.

Feel free to send an additional cleanup patch ... otherwise, I'd say let
it bitrot for another year and we then remove it completely together
with all the other "-usbdevice" functions...

 Thomas

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Paolo Bonzini]

On 18/05/2017 15:22, Thomas Huth wrote:
> On 18.05.2017 14:00, Paolo Bonzini wrote:
>>
>>
>> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>>> From: Thomas Huth <thuth@redhat.com>
>>>
>>> When starting QEMU with the legacy USB serial device like this:
>>>
>>>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
>>>
>>> it currently aborts since the vendorid property does not exist
>>> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
>>>
>>>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>>>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>>>                      '.vendorid' not found
>>>  Aborted (core dumped)
>>>
>>> Fix this crash by issuing a more friendly error message instead
>>> (and simplify the code also a little bit this way).
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  hw/usb/dev-serial.c | 24 ++++++------------------
>>>  1 file changed, 6 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>>> index 6d5137383b..83a4f0e6fb 100644
>>> --- a/hw/usb/dev-serial.c
>>> +++ b/hw/usb/dev-serial.c
>>> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>>>  {
>>>      USBDevice *dev;
>>>      Chardev *cdrv;
>>> -    uint32_t vendorid = 0, productid = 0;
>>>      char label[32];
>>>      static int index;
>>>  
>>>      while (*filename && *filename != ':') {
>>>          const char *p;
>>> -        char *e;
>>> +
>>>          if (strstart(filename, "vendorid=", &p)) {
>>> -            vendorid = strtol(p, &e, 16);
>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>> -                error_report("bogus vendor ID %s", p);
>>> -                return NULL;
>>> -            }
>>> -            filename = e;
>>> +            error_report("vendorid is not supported anymore");
>>> +            return NULL;
>>>          } else if (strstart(filename, "productid=", &p)) {
>>> -            productid = strtol(p, &e, 16);
>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>> -                error_report("bogus product ID %s", p);
>>> -                return NULL;
>>> -            }
>>> -            filename = e;
>>> +            error_report("productid is not supported anymore");
>>> +            return NULL;
>>>          } else {
>>>              error_report("unrecognized serial USB option %s", filename);
>>>              return NULL;
>>
>> All breanches of the "if" now return NULL, so the "while" loop in turn
>> can become an
>>
>>     if (*filename && *filename != ':') {
>>     }
>>
>> and the "while (*filename == ',')" subloop can go away, replaced by just
>> "return NULL".
>>
>> Even better, the "if (!*filename)" if just below can be moved first.
> 
> Feel free to send an additional cleanup patch ... otherwise, I'd say let
> it bitrot for another year and we then remove it completely together
> with all the other "-usbdevice" functions...

Well, Coverity reports it so I'd rather keep it clean...

Paolo

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Thomas Huth]

On 18.05.2017 15:35, Paolo Bonzini wrote:
> 
> 
> On 18/05/2017 15:22, Thomas Huth wrote:
>> On 18.05.2017 14:00, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>>>> From: Thomas Huth <thuth@redhat.com>
>>>>
>>>> When starting QEMU with the legacy USB serial device like this:
>>>>
>>>>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
>>>>
>>>> it currently aborts since the vendorid property does not exist
>>>> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
>>>>
>>>>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>>>>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>>>>                      '.vendorid' not found
>>>>  Aborted (core dumped)
>>>>
>>>> Fix this crash by issuing a more friendly error message instead
>>>> (and simplify the code also a little bit this way).
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>>  hw/usb/dev-serial.c | 24 ++++++------------------
>>>>  1 file changed, 6 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>>>> index 6d5137383b..83a4f0e6fb 100644
>>>> --- a/hw/usb/dev-serial.c
>>>> +++ b/hw/usb/dev-serial.c
>>>> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>>>>  {
>>>>      USBDevice *dev;
>>>>      Chardev *cdrv;
>>>> -    uint32_t vendorid = 0, productid = 0;
>>>>      char label[32];
>>>>      static int index;
>>>>  
>>>>      while (*filename && *filename != ':') {
>>>>          const char *p;
>>>> -        char *e;
>>>> +
>>>>          if (strstart(filename, "vendorid=", &p)) {
>>>> -            vendorid = strtol(p, &e, 16);
>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>> -                error_report("bogus vendor ID %s", p);
>>>> -                return NULL;
>>>> -            }
>>>> -            filename = e;
>>>> +            error_report("vendorid is not supported anymore");
>>>> +            return NULL;
>>>>          } else if (strstart(filename, "productid=", &p)) {
>>>> -            productid = strtol(p, &e, 16);
>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>> -                error_report("bogus product ID %s", p);
>>>> -                return NULL;
>>>> -            }
>>>> -            filename = e;
>>>> +            error_report("productid is not supported anymore");
>>>> +            return NULL;
>>>>          } else {
>>>>              error_report("unrecognized serial USB option %s", filename);
>>>>              return NULL;
>>>
>>> All breanches of the "if" now return NULL, so the "while" loop in turn
>>> can become an
>>>
>>>     if (*filename && *filename != ':') {
>>>     }
>>>
>>> and the "while (*filename == ',')" subloop can go away, replaced by just
>>> "return NULL".
>>>
>>> Even better, the "if (!*filename)" if just below can be moved first.
>>
>> Feel free to send an additional cleanup patch ... otherwise, I'd say let
>> it bitrot for another year and we then remove it completely together
>> with all the other "-usbdevice" functions...
> 
> Well, Coverity reports it so I'd rather keep it clean...

Hmm, maybe we should simply remove "-usbdevice serial" right now already
... ? The vendorid/productid parameter handling has been broken since
QEMU v0.14 already and nobody ever complained, so I guess hardly anybody
is using "-usbdevice serial" anymore ... so I tend to simply remove it
directly instead of going through the typical "mark-as-deprecated ->
wait-two-release-cycles -> finally-remove-it" process here...

Paolo, Gerd, what do you think?

 Thomas

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Gerd Hoffmann]

  Hi,

> >> Feel free to send an additional cleanup patch ... otherwise, I'd say let
> >> it bitrot for another year and we then remove it completely together
> >> with all the other "-usbdevice" functions...
> > 
> > Well, Coverity reports it so I'd rather keep it clean...

It will be clean once removed, I likewise wouldn't put too much effort
into code which is scheduled for removal anyway, especially for more or
less cosmetic issues like unreachable code.

> Hmm, maybe we should simply remove "-usbdevice serial" right now already
> ... ? The vendorid/productid parameter handling has been broken since
> QEMU v0.14 already and nobody ever complained, so I guess hardly anybody
> is using "-usbdevice serial" anymore ... so I tend to simply remove it
> directly instead of going through the typical "mark-as-deprecated ->
> wait-two-release-cycles -> finally-remove-it" process here...

Hmm, there is little reason to set vendorid + productid, so that alone
is no strong indication that -usbdevice serial is unused.  But
nevertheless I think it usb-serial is one of the rarely used devices, so
we could try ...

Order is probably along these lines:

usb hid devices are used alot (tablet on x86, all on !x86).
usb-storage comes next I guess.
usb-host (aka pass-through) is important too of course.

Everything else (serial, net, bluetooth) is largely unused I guess.

ccid, uas and mtp are new enough to not carry around -usbdevice
compatibility support.

cheers,
  Gerd

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18.05.2017 15:35, Paolo Bonzini wrote:
>> 
>> 
>> On 18/05/2017 15:22, Thomas Huth wrote:
>>> On 18.05.2017 14:00, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>
>>>>> When starting QEMU with the legacy USB serial device like this:
>>>>>
>>>>>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
>>>>>
>>>>> it currently aborts since the vendorid property does not exist
>>>>> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
>>>>>
>>>>>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>>>>>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>>>>>                      '.vendorid' not found
>>>>>  Aborted (core dumped)
>>>>>
>>>>> Fix this crash by issuing a more friendly error message instead
>>>>> (and simplify the code also a little bit this way).
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>> ---
>>>>>  hw/usb/dev-serial.c | 24 ++++++------------------
>>>>>  1 file changed, 6 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>>>>> index 6d5137383b..83a4f0e6fb 100644
>>>>> --- a/hw/usb/dev-serial.c
>>>>> +++ b/hw/usb/dev-serial.c
>>>>> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>>>>>  {
>>>>>      USBDevice *dev;
>>>>>      Chardev *cdrv;
>>>>> -    uint32_t vendorid = 0, productid = 0;
>>>>>      char label[32];
>>>>>      static int index;
>>>>>  
>>>>>      while (*filename && *filename != ':') {
>>>>>          const char *p;
>>>>> -        char *e;
>>>>> +
>>>>>          if (strstart(filename, "vendorid=", &p)) {
>>>>> -            vendorid = strtol(p, &e, 16);
>>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>>> -                error_report("bogus vendor ID %s", p);
>>>>> -                return NULL;
>>>>> -            }
>>>>> -            filename = e;
>>>>> +            error_report("vendorid is not supported anymore");
>>>>> +            return NULL;
>>>>>          } else if (strstart(filename, "productid=", &p)) {
>>>>> -            productid = strtol(p, &e, 16);
>>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>>> -                error_report("bogus product ID %s", p);
>>>>> -                return NULL;
>>>>> -            }
>>>>> -            filename = e;
>>>>> +            error_report("productid is not supported anymore");
>>>>> +            return NULL;
>>>>>          } else {
>>>>>              error_report("unrecognized serial USB option %s", filename);
>>>>>              return NULL;
>>>>
>>>> All breanches of the "if" now return NULL, so the "while" loop in turn
>>>> can become an
>>>>
>>>>     if (*filename && *filename != ':') {
>>>>     }
>>>>
>>>> and the "while (*filename == ',')" subloop can go away, replaced by just
>>>> "return NULL".
>>>>
>>>> Even better, the "if (!*filename)" if just below can be moved first.
>>>
>>> Feel free to send an additional cleanup patch ... otherwise, I'd say let
>>> it bitrot for another year and we then remove it completely together
>>> with all the other "-usbdevice" functions...
>> 
>> Well, Coverity reports it so I'd rather keep it clean...
>
> Hmm, maybe we should simply remove "-usbdevice serial" right now already
> ... ? The vendorid/productid parameter handling has been broken since
> QEMU v0.14 already and nobody ever complained, so I guess hardly anybody
> is using "-usbdevice serial" anymore ... so I tend to simply remove it
> directly instead of going through the typical "mark-as-deprecated ->
> wait-two-release-cycles -> finally-remove-it" process here...
>
> Paolo, Gerd, what do you think?

Being broken counts as being deprecated, I'd say.

But was -usbdevice serial broken?  Or just its two optional (and
somewhat exotic) parameters?

Re: [Qemu-devel] [PULL 6/6] hw/usb/dev-serial: Do not try to set vendorid or productid properties

[Thomas Huth]

On 19.05.2017 07:54, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 18.05.2017 15:35, Paolo Bonzini wrote:
>>>
>>>
>>> On 18/05/2017 15:22, Thomas Huth wrote:
>>>> On 18.05.2017 14:00, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 12/05/2017 14:21, Gerd Hoffmann wrote:
>>>>>> From: Thomas Huth <thuth@redhat.com>
>>>>>>
>>>>>> When starting QEMU with the legacy USB serial device like this:
>>>>>>
>>>>>>  qemu-system-x86_64 -usbdevice serial:vendorid=0x1234:stdio
>>>>>>
>>>>>> it currently aborts since the vendorid property does not exist
>>>>>> anymore (it has been removed by commit f29783f72ea77dfbd7ea0c9):
>>>>>>
>>>>>>  Unexpected error in object_property_find() at qemu/qom/object.c:1008:
>>>>>>  qemu-system-x86_64: -usbdevice serial:vendorid=0x1234:stdio: Property
>>>>>>                      '.vendorid' not found
>>>>>>  Aborted (core dumped)
>>>>>>
>>>>>> Fix this crash by issuing a more friendly error message instead
>>>>>> (and simplify the code also a little bit this way).
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> Message-id: 1493883704-27604-1-git-send-email-thuth@redhat.com
>>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>>> ---
>>>>>>  hw/usb/dev-serial.c | 24 ++++++------------------
>>>>>>  1 file changed, 6 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
>>>>>> index 6d5137383b..83a4f0e6fb 100644
>>>>>> --- a/hw/usb/dev-serial.c
>>>>>> +++ b/hw/usb/dev-serial.c
>>>>>> @@ -513,27 +513,18 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
>>>>>>  {
>>>>>>      USBDevice *dev;
>>>>>>      Chardev *cdrv;
>>>>>> -    uint32_t vendorid = 0, productid = 0;
>>>>>>      char label[32];
>>>>>>      static int index;
>>>>>>  
>>>>>>      while (*filename && *filename != ':') {
>>>>>>          const char *p;
>>>>>> -        char *e;
>>>>>> +
>>>>>>          if (strstart(filename, "vendorid=", &p)) {
>>>>>> -            vendorid = strtol(p, &e, 16);
>>>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>>>> -                error_report("bogus vendor ID %s", p);
>>>>>> -                return NULL;
>>>>>> -            }
>>>>>> -            filename = e;
>>>>>> +            error_report("vendorid is not supported anymore");
>>>>>> +            return NULL;
>>>>>>          } else if (strstart(filename, "productid=", &p)) {
>>>>>> -            productid = strtol(p, &e, 16);
>>>>>> -            if (e == p || (*e && *e != ',' && *e != ':')) {
>>>>>> -                error_report("bogus product ID %s", p);
>>>>>> -                return NULL;
>>>>>> -            }
>>>>>> -            filename = e;
>>>>>> +            error_report("productid is not supported anymore");
>>>>>> +            return NULL;
>>>>>>          } else {
>>>>>>              error_report("unrecognized serial USB option %s", filename);
>>>>>>              return NULL;
>>>>>
>>>>> All breanches of the "if" now return NULL, so the "while" loop in turn
>>>>> can become an
>>>>>
>>>>>     if (*filename && *filename != ':') {
>>>>>     }
>>>>>
>>>>> and the "while (*filename == ',')" subloop can go away, replaced by just
>>>>> "return NULL".
>>>>>
>>>>> Even better, the "if (!*filename)" if just below can be moved first.
>>>>
>>>> Feel free to send an additional cleanup patch ... otherwise, I'd say let
>>>> it bitrot for another year and we then remove it completely together
>>>> with all the other "-usbdevice" functions...
>>>
>>> Well, Coverity reports it so I'd rather keep it clean...
>>
>> Hmm, maybe we should simply remove "-usbdevice serial" right now already
>> ... ? The vendorid/productid parameter handling has been broken since
>> QEMU v0.14 already and nobody ever complained, so I guess hardly anybody
>> is using "-usbdevice serial" anymore ... so I tend to simply remove it
>> directly instead of going through the typical "mark-as-deprecated ->
>> wait-two-release-cycles -> finally-remove-it" process here...
>>
>> Paolo, Gerd, what do you think?
> 
> Being broken counts as being deprecated, I'd say.
> 
> But was -usbdevice serial broken?  Or just its two optional (and
> somewhat exotic) parameters?

Just the two exotic parameters. But I agree with Gerd: If you google for
the option to see how people are using the usbdevice option, you almost
only find pages that describe "-usbdevice mouse/keyboard/tablet" or host
passthrough. Sometimes also "storage". But hardly anything about
"serial". However, I've now also found a bug where someone complains
about the vendorid/productid problem:

 https://bugs.launchpad.net/qemu/+bug/1180924

So that's an indication that at least some few people used "-usbdevice
serial" in the past ... so let's fix that coverity warning now instead,
then add some proper deprecation warnings to the "-usbdevice" parameter
and remove all that legacy stuff next year...

 Thomas

Re: [Qemu-devel] [PULL 0/6] usb patch queue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

On Fri, May 12, 2017 at 02:21:52PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> Here comes the usb patch queue, with a bugfix collection and a
> documentation update.
> 
> please pull,
>   Gerd
> 
> The following changes since commit 76d20ea0f1b26ebd5da2f5fb2fdf3250cde887bb:
> 
>   Merge remote-tracking branch 'armbru/tags/pull-qapi-2017-05-04-v3' into staging (2017-05-09 15:49:14 -0400)
> 
> are available in the git repository at:
> 
>   git://git.kraxel.org/qemu tags/pull-usb-20170512-1
> 
> for you to fetch changes up to aa612b364ecbe1dc034efcabb04526f24e56c145:
> 
>   hw/usb/dev-serial: Do not try to set vendorid or productid properties (2017-05-12 12:30:23 +0200)
> 
> ----------------------------------------------------------------
> usb: bugfixes, doc update
> 
> ----------------------------------------------------------------
> Gerd Hoffmann (1):
>       usb-redir: fix stack overflow in usbredir_log_data
> 
> Ladi Prosek (3):
>       xhci: fix logging
>       usb-hub: clear PORT_STAT_SUSPEND on wakeup
>       xhci: relax link check
> 
> Thomas Huth (2):
>       qemu-doc: Update to use the new way of attaching USB devices
>       hw/usb/dev-serial: Do not try to set vendorid or productid properties
> 
>  docs/qdev-device-use.txt |  6 ++--
>  hw/usb/dev-hub.c         |  1 +
>  hw/usb/dev-serial.c      | 24 ++++---------
>  hw/usb/hcd-xhci.c        |  9 ++---
>  hw/usb/redirect.c        | 13 +------
>  qemu-doc.texi            | 91 +++++++++++++++++++++++++++---------------------
>  6 files changed, 64 insertions(+), 80 deletions(-)
> 

Thanks, applied to my staging tree:
https://github.com/stefanha/qemu/commits/staging

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]