[Qemu-devel] [PULL v2 1.0] usb patch queue

[Qemu-devel] [PULL v2 1.0] usb patch queue

[Gerd Hoffmann]

  Hi,

This pull fixes SYNCRONIZE_CACHE handling in usb-storage code.

The SYNCRONIZE_CACHE scsi command is different from all other ones:
It doesn't transfer any data, but still doesn't complete instantly.
usb-storage failes to handle this case correctly, resulting in all sorts
of funky assert failures and segfaults when the guest writes to a
virtual usb stick and flushes stuff.

Patches 1-4 are minor cleanups for usb-storage (slighly improved scsi
request tracking).  Patch 5 is the actual bugfix, which depends on the
cleanups.

Patches 6+7 are bonus fixes for Coverity-spotted issues which are
sitting in the usb patch queue for a while already.

The following changes since commit bc4268998d154b9b3cc86a7b6bd932cc974591c9:

  x86: fix pcmpestrm and pcmpistrm (2011-11-19 13:51:27 +0000)

are available in the git repository at:
  git://git.kraxel.org/qemu usb.31

Gerd Hoffmann (7):
      usb-storage: move status debug message to usb_msd_send_status.
      usb-storage: fill status in complete callback.
      usb-storage: drop tag from device state.
      usb-storage: drop result from device state.
      usb-storage: don't try to send the status early.
      ehci: add assert
      usb-linux: fix /proc/bus/usb/devices scan

 hw/usb-ehci.c |    1 +
 hw/usb-msd.c  |   65 ++++++++++++++++++++++++++++++--------------------------
 usb-linux.c   |    7 +++++-
 3 files changed, 42 insertions(+), 31 deletions(-)

[Qemu-devel] [PATCH 1/7] usb-storage: move status debug message to usb_msd_send_status.

[Gerd Hoffmann]

usb_msd_send_status can be called from different code paths, move the
debug message into the function to make sure it is printed
unconditionally.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-msd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index b734177..8dde421 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -194,6 +194,8 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
     struct usb_msd_csw csw;
     int len;
 
+    DPRINTF("Command status %d tag 0x%x, len %zd\n",
+            s->result, s->tag, p->iov.size);
     csw.sig = cpu_to_le32(0x53425355);
     csw.tag = cpu_to_le32(s->tag);
     csw.residue = s->residue;
@@ -432,8 +434,6 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
             break;
 
         case USB_MSDM_CSW:
-            DPRINTF("Command status %d tag 0x%x, len %zd\n",
-                    s->result, s->tag, p->iov.size);
             if (p->iov.size < 13) {
                 goto fail;
             }
-- 
1.7.1

[Qemu-devel] [PATCH 2/7] usb-storage: fill status in complete callback.

[Gerd Hoffmann]

Put status word into device state, fill it in command_complete, have
usb_msd_send_status just send it out.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-msd.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 8dde421..5dc4afc 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -38,6 +38,13 @@ enum USBMSDMode {
     USB_MSDM_CSW /* Command Status.  */
 };
 
+struct usb_msd_csw {
+    uint32_t sig;
+    uint32_t tag;
+    uint32_t residue;
+    uint8_t status;
+};
+
 typedef struct {
     USBDevice dev;
     enum USBMSDMode mode;
@@ -46,6 +53,7 @@ typedef struct {
     uint32_t data_len;
     uint32_t residue;
     uint32_t tag;
+    struct usb_msd_csw csw;
     SCSIRequest *req;
     SCSIBus bus;
     BlockConf conf;
@@ -67,13 +75,6 @@ struct usb_msd_cbw {
     uint8_t cmd[16];
 };
 
-struct usb_msd_csw {
-    uint32_t sig;
-    uint32_t tag;
-    uint32_t residue;
-    uint8_t status;
-};
-
 enum {
     STR_MANUFACTURER = 1,
     STR_PRODUCT,
@@ -191,19 +192,15 @@ static void usb_msd_copy_data(MSDState *s, USBPacket *p)
 
 static void usb_msd_send_status(MSDState *s, USBPacket *p)
 {
-    struct usb_msd_csw csw;
     int len;
 
     DPRINTF("Command status %d tag 0x%x, len %zd\n",
-            s->result, s->tag, p->iov.size);
-    csw.sig = cpu_to_le32(0x53425355);
-    csw.tag = cpu_to_le32(s->tag);
-    csw.residue = s->residue;
-    csw.status = s->result;
-
-    len = MIN(sizeof(csw), p->iov.size);
-    usb_packet_copy(p, &csw, len);
-    p->result = len;
+            s->csw.status, s->csw.tag, p->iov.size);
+
+    assert(s->csw.sig == 0x53425355);
+    len = MIN(sizeof(s->csw), p->iov.size);
+    usb_packet_copy(p, &s->csw, len);
+    memset(&s->csw, 0, sizeof(s->csw));
 }
 
 static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
@@ -236,6 +233,12 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
     DPRINTF("Command complete %d\n", status);
     s->residue = s->data_len;
     s->result = status != 0;
+
+    s->csw.sig = cpu_to_le32(0x53425355);
+    s->csw.tag = cpu_to_le32(s->tag);
+    s->csw.residue = s->residue;
+    s->csw.status = s->result;
+
     if (s->packet) {
         if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
             /* A deferred packet with no write data remaining must be
@@ -257,6 +260,7 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
     } else if (s->data_len == 0) {
         s->mode = USB_MSDM_CSW;
     }
+
     scsi_req_unref(req);
     s->req = NULL;
 }
-- 
1.7.1

[Qemu-devel] [PATCH 3/7] usb-storage: drop tag from device state.

[Gerd Hoffmann]

scsi keeps track of the tag in SCSIRequest,
no need to store a separate copy.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-msd.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 5dc4afc..089c23c 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -52,7 +52,6 @@ typedef struct {
     uint8_t *scsi_buf;
     uint32_t data_len;
     uint32_t residue;
-    uint32_t tag;
     struct usb_msd_csw csw;
     SCSIRequest *req;
     SCSIBus bus;
@@ -230,12 +229,12 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
     USBPacket *p = s->packet;
 
-    DPRINTF("Command complete %d\n", status);
+    DPRINTF("Command complete %d tag 0x%x\n", status, req->tag);
     s->residue = s->data_len;
     s->result = status != 0;
 
     s->csw.sig = cpu_to_le32(0x53425355);
-    s->csw.tag = cpu_to_le32(s->tag);
+    s->csw.tag = cpu_to_le32(req->tag);
     s->csw.residue = s->residue;
     s->csw.status = s->result;
 
@@ -260,7 +259,6 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
     } else if (s->data_len == 0) {
         s->mode = USB_MSDM_CSW;
     }
-
     scsi_req_unref(req);
     s->req = NULL;
 }
@@ -340,6 +338,7 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
 static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
 {
     MSDState *s = (MSDState *)dev;
+    uint32_t tag;
     int ret = 0;
     struct usb_msd_cbw cbw;
     uint8_t devep = p->devep;
@@ -366,7 +365,7 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                 fprintf(stderr, "usb-msd: Bad LUN %d\n", cbw.lun);
                 goto fail;
             }
-            s->tag = le32_to_cpu(cbw.tag);
+            tag = le32_to_cpu(cbw.tag);
             s->data_len = le32_to_cpu(cbw.data_len);
             if (s->data_len == 0) {
                 s->mode = USB_MSDM_CSW;
@@ -376,10 +375,10 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                 s->mode = USB_MSDM_DATAOUT;
             }
             DPRINTF("Command tag 0x%x flags %08x len %d data %d\n",
-                    s->tag, cbw.flags, cbw.cmd_len, s->data_len);
+                    tag, cbw.flags, cbw.cmd_len, s->data_len);
             s->residue = 0;
             s->scsi_len = 0;
-            s->req = scsi_req_new(s->scsi_dev, s->tag, 0, cbw.cmd, NULL);
+            s->req = scsi_req_new(s->scsi_dev, tag, 0, cbw.cmd, NULL);
             scsi_req_enqueue(s->req);
             /* ??? Should check that USB and SCSI data transfer
                directions match.  */
-- 
1.7.1

[Qemu-devel] [PATCH 4/7] usb-storage: drop result from device state.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-msd.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 089c23c..6f32a0e 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -59,7 +59,6 @@ typedef struct {
     char *serial;
     SCSIDevice *scsi_dev;
     uint32_t removable;
-    int result;
     /* For async completion.  */
     USBPacket *packet;
 } MSDState;
@@ -231,12 +230,11 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status)
 
     DPRINTF("Command complete %d tag 0x%x\n", status, req->tag);
     s->residue = s->data_len;
-    s->result = status != 0;
 
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
     s->csw.residue = s->residue;
-    s->csw.status = s->result;
+    s->csw.status = status != 0;
 
     if (s->packet) {
         if (s->data_len == 0 && s->mode == USB_MSDM_DATAOUT) {
-- 
1.7.1

[Qemu-devel] [PATCH 5/7] usb-storage: don't try to send the status early.

[Gerd Hoffmann]

Until recently all scsi commands sent to scsi-disk did either transfer
data or finished instantly.  The correct implementation of
SYNCRONIZE_CACHE changed the picture though, and usb-storage needs
a fix to handle that case correctly.
---
 hw/usb-msd.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 6f32a0e..68e3756 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -378,9 +378,7 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
             s->scsi_len = 0;
             s->req = scsi_req_new(s->scsi_dev, tag, 0, cbw.cmd, NULL);
             scsi_req_enqueue(s->req);
-            /* ??? Should check that USB and SCSI data transfer
-               directions match.  */
-            if (s->mode != USB_MSDM_CSW && s->residue == 0) {
+            if (s->req && s->req->cmd.xfer != SCSI_XFER_NONE) {
                 scsi_req_continue(s->req);
             }
             ret = p->result;
@@ -439,9 +437,15 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
                 goto fail;
             }
 
-            usb_msd_send_status(s, p);
-            s->mode = USB_MSDM_CBW;
-            ret = 13;
+            if (s->req) {
+                /* still in flight */
+                s->packet = p;
+                ret = USB_RET_ASYNC;
+            } else {
+                usb_msd_send_status(s, p);
+                s->mode = USB_MSDM_CBW;
+                ret = 13;
+            }
             break;
 
         case USB_MSDM_DATAIN:
-- 
1.7.1

[Qemu-devel] [PATCH 6/7] ehci: add assert

[Gerd Hoffmann]

Coverity thinks q could be NULL there and warns.
I believe it can't be NULL there.
Add assert to prove it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-ehci.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index cdd5aae..3eea94d 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -2046,6 +2046,7 @@ static void ehci_advance_state(EHCIState *ehci,
             break;
 
         case EST_WRITEBACK:
+            assert(q != NULL);
             again = ehci_state_writeback(q, async);
             break;
 
-- 
1.7.1

[Qemu-devel] [PATCH 7/7] usb-linux: fix /proc/bus/usb/devices scan

[Gerd Hoffmann]

Commit 0c402e5abb8c2755390eee864b43a98280fc2453 is incomplete
and misses one of the two function pointer calls in
usb_host_scan_dev().  Add the additional port handling logic
to the other call too.

Spotted by Coverity.

Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 usb-linux.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index f086d57..d4426ea 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1568,7 +1568,12 @@ static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
         if (line[0] == 'T' && line[1] == ':') {
             if (device_count && (vendor_id || product_id)) {
                 /* New device.  Add the previously discovered device.  */
-                ret = func(opaque, bus_num, addr, 0, class_id, vendor_id,
+                if (port > 0) {
+                    snprintf(buf, sizeof(buf), "%d", port);
+                } else {
+                    snprintf(buf, sizeof(buf), "?");
+                }
+                ret = func(opaque, bus_num, addr, buf, class_id, vendor_id,
                            product_id, product_name, speed);
                 if (ret) {
                     goto the_end;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 7/7] usb-linux: fix /proc/bus/usb/devices scan

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Commit 0c402e5abb8c2755390eee864b43a98280fc2453 is incomplete
> and misses one of the two function pointer calls in
> usb_host_scan_dev().  Add the additional port handling logic
> to the other call too.
>
> Spotted by Coverity.
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PULL v2 1.0] usb patch queue

[Anthony Liguori]

On 11/21/2011 10:15 AM, Gerd Hoffmann wrote:
>    Hi,
>
> This pull fixes SYNCRONIZE_CACHE handling in usb-storage code.
>
> The SYNCRONIZE_CACHE scsi command is different from all other ones:
> It doesn't transfer any data, but still doesn't complete instantly.
> usb-storage failes to handle this case correctly, resulting in all sorts
> of funky assert failures and segfaults when the guest writes to a
> virtual usb stick and flushes stuff.
>
> Patches 1-4 are minor cleanups for usb-storage (slighly improved scsi
> request tracking).  Patch 5 is the actual bugfix, which depends on the
> cleanups.
>
> Patches 6+7 are bonus fixes for Coverity-spotted issues which are
> sitting in the usb patch queue for a while already.

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> The following changes since commit bc4268998d154b9b3cc86a7b6bd932cc974591c9:
>
>    x86: fix pcmpestrm and pcmpistrm (2011-11-19 13:51:27 +0000)
>
> are available in the git repository at:
>    git://git.kraxel.org/qemu usb.31
>
> Gerd Hoffmann (7):
>        usb-storage: move status debug message to usb_msd_send_status.
>        usb-storage: fill status in complete callback.
>        usb-storage: drop tag from device state.
>        usb-storage: drop result from device state.
>        usb-storage: don't try to send the status early.
>        ehci: add assert
>        usb-linux: fix /proc/bus/usb/devices scan
>
>   hw/usb-ehci.c |    1 +
>   hw/usb-msd.c  |   65 ++++++++++++++++++++++++++++++--------------------------
>   usb-linux.c   |    7 +++++-
>   3 files changed, 42 insertions(+), 31 deletions(-)
>
>