From patchwork Thu Oct 31 09:35:31 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 287456 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 3FA432C03B3 for ; Thu, 31 Oct 2013 21:56:25 +1100 (EST) Received: from localhost ([::1]:56126 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbpvI-0007cv-KD for incoming@patchwork.ozlabs.org; Thu, 31 Oct 2013 06:56:20 -0400 Received: from eggs.gnu.org ([208.118.235.92]:54586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vbpuj-0007F8-8q for qemu-devel@nongnu.org; Thu, 31 Oct 2013 06:55:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbofB-0000MY-Hg for qemu-devel@nongnu.org; Thu, 31 Oct 2013 05:39:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30681) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbofB-0000MJ-88 for qemu-devel@nongnu.org; Thu, 31 Oct 2013 05:35:37 -0400 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 r9V9ZYnU010567 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 31 Oct 2013 05:35:35 -0400 Received: from shalem.localdomain.com (vpn1-4-223.ams2.redhat.com [10.36.4.223]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r9V9ZWBD013542; Thu, 31 Oct 2013 05:35:33 -0400 From: Hans de Goede To: Gerd Hoffmann Date: Thu, 31 Oct 2013 10:35:31 +0100 Message-Id: <1383212131-10346-1-git-send-email-hdegoede@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-MIME-Autoconverted: from 8bit to quoted-printable by mx1.redhat.com id r9V9ZYnU010567 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Hans de Goede , qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCH] uas: Fix response iu struct definition X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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)