[Qemu-devel] [PATCH] uas: Fix response iu struct definition

[Qemu-devel] [PATCH] uas: Fix response iu struct definition

[Hans de Goede]

This patch mirrors a patch to the Linux uas kernel driver which I've just
submitted. It looks like the qemu uas struct definitions were taken from
the Linux kernel driver, and have inherited the same mistake.

Besides fixing the response iu struct, the patch also drops the add_info
parameter from the usb_uas_queue_response() function, it is always 0 anyways,
and expressing 3 zero-bytes as a function argument is a bit hard.

Below is the long explanation for this change taken from the kernel commit:

The response iu struct before this patch has a size of 7 bytes, which is weird
since all other iu-s are explictly padded to a multiple of 4 bytes.

Submitting a 7 byte bulk transfer to the status endpoint of a real uasp device
when expecting a response iu results in an USB babble error, as the device
actually sends 8 bytes.

Up on closer reading of the UAS spec:
http://www.t10.org/cgi-bin/ac.pl?t=f&f=uas2r00.pdf

The reason for this becomes clear, the 2 entries in "Table 17 — RESPONSE IU"
are numbered 4 and 6, looking at other iu definitions in the spec, esp.
multi-byte fields, this indicates that the ADDITIONAL RESPONSE INFORMATION
field is not a 2 byte field as one might assume at a first look, but is
a multi-byte field containing 3 bytes.

This also aligns with the SCSI Architecture Model 4 spec, which UAS is based
on which states in paragraph "7.1 Task management function procedure calls"
that the "Additional Response Information" output argument for a Task
management function procedure call is 3 bytes.

Last but not least I've verified this by sending a logical unit reset task
management call with an invalid lun to an actual uasp device, and received
back a response-iu with byte 6 being 0, and byte 7 being 9, which is the
responce code for an invalid iu, which confirms that the response code is
being reported in byte 7 of the response iu rather then in byte 6.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-uas.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 5884035..82a47be 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -76,7 +76,7 @@ typedef struct {
 } QEMU_PACKED  uas_ui_sense;
 
 typedef struct {
-    uint16_t   add_response_info;
+    uint8_t    add_response_info[3];
     uint8_t    response_code;
 } QEMU_PACKED  uas_ui_response;
 
@@ -392,14 +392,12 @@ static void usb_uas_queue_status(UASDevice *uas, UASStatus *st, int length)
     }
 }
 
-static void usb_uas_queue_response(UASDevice *uas, uint16_t tag,
-                                   uint8_t code, uint16_t add_info)
+static void usb_uas_queue_response(UASDevice *uas, uint16_t tag, uint8_t code)
 {
     UASStatus *st = usb_uas_alloc_status(uas, UAS_UI_RESPONSE, tag);
 
     trace_usb_uas_response(uas->dev.addr, tag, code);
     st->status.response.response_code = code;
-    st->status.response.add_response_info = cpu_to_be16(add_info);
     usb_uas_queue_status(uas, st, sizeof(uas_ui_response));
 }
 
@@ -768,32 +766,32 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
         if (req && req->dev == dev) {
             scsi_req_cancel(req->req);
         }
-        usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE, 0);
+        usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE);
         break;
 
     case UAS_TMF_LOGICAL_UNIT_RESET:
         trace_usb_uas_tmf_logical_unit_reset(uas->dev.addr, tag, lun);
         qdev_reset_all(&dev->qdev);
-        usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE, 0);
+        usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE);
         break;
 
     default:
         trace_usb_uas_tmf_unsupported(uas->dev.addr, tag, ui->task.function);
-        usb_uas_queue_response(uas, tag, UAS_RC_TMF_NOT_SUPPORTED, 0);
+        usb_uas_queue_response(uas, tag, UAS_RC_TMF_NOT_SUPPORTED);
         break;
     }
     return;
 
 invalid_tag:
-    usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT, 0);
+    usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT);
     return;
 
 overlapped_tag:
-    usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG, 0);
+    usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG);
     return;
 
 incorrect_lun:
-    usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN, 0);
+    usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN);
 }
 
 static void usb_uas_handle_data(USBDevice *dev, USBPacket *p)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] uas: Fix response iu struct definition

[Gerd Hoffmann]

On Do, 2013-10-31 at 10:35 +0100, Hans de Goede wrote:
> This patch mirrors a patch to the Linux uas kernel driver which I've just
> submitted. It looks like the qemu uas struct definitions were taken from
> the Linux kernel driver, and have inherited the same mistake.
> 
> Besides fixing the response iu struct, the patch also drops the add_info
> parameter from the usb_uas_queue_response() function, it is always 0 anyways,
> and expressing 3 zero-bytes as a function argument is a bit hard.

Added to usb patch queue, together with the other uas patch series.

thanks,
  Gerd