From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmJ6B-0002U4-B4 for qemu-devel@nongnu.org; Fri, 29 Nov 2013 03:06:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmJ61-0002CI-8T for qemu-devel@nongnu.org; Fri, 29 Nov 2013 03:06:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmJ60-0002Bj-Vd for qemu-devel@nongnu.org; Fri, 29 Nov 2013 03:06:41 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAT86eJl017737 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 29 Nov 2013 03:06:40 -0500 From: Gerd Hoffmann Date: Fri, 29 Nov 2013 09:06:12 +0100 Message-Id: <1385712381-30918-9-git-send-email-kraxel@redhat.com> In-Reply-To: <1385712381-30918-1-git-send-email-kraxel@redhat.com> References: <1385712381-30918-1-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] [PATCH 08/17] uas: Fix response iu struct definition List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Hans de Goede , Gerd Hoffmann From: 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 anyw= ays, 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 commi= t: 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 d= evice when expecting a response iu results in an USB babble error, as the devic= e actually sends 8 bytes. Up on closer reading of the UAS spec: http://www.t10.org/cgi-bin/ac.pl?t=3Df&f=3Duas2r00.pdf The reason for this becomes clear, the 2 entries in "Table 17 =E2=80=94 R= ESPONSE IU" are numbered 4 and 6, looking at other iu definitions in the spec, esp. multi-byte fields, this indicates that the ADDITIONAL RESPONSE INFORMATIO= N 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 ba= sed on which states in paragraph "7.1 Task management function procedure call= s" 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 tas= k management call with an invalid lun to an actual uasp device, and receive= d 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 Signed-off-by: Gerd Hoffmann --- 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; =20 typedef struct { - uint16_t add_response_info; + uint8_t add_response_info[3]; uint8_t response_code; } QEMU_PACKED uas_ui_response; =20 @@ -392,14 +392,12 @@ static void usb_uas_queue_status(UASDevice *uas, UA= SStatus *st, int length) } } =20 -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 =3D usb_uas_alloc_status(uas, UAS_UI_RESPONSE, tag); =20 trace_usb_uas_response(uas->dev.addr, tag, code); st->status.response.response_code =3D code; - st->status.response.add_response_info =3D cpu_to_be16(add_info); usb_uas_queue_status(uas, st, sizeof(uas_ui_response)); } =20 @@ -768,32 +766,32 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui= ) if (req && req->dev =3D=3D 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; =20 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; =20 default: trace_usb_uas_tmf_unsupported(uas->dev.addr, tag, ui->task.funct= ion); - 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; =20 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; =20 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; =20 incorrect_lun: - usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN, 0); + usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN); } =20 static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) --=20 1.8.3.1