From owner-freebsd-usb@FreeBSD.ORG Thu Jan 29 19:59:20 2015 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BBAD34DA; Thu, 29 Jan 2015 19:59:20 +0000 (UTC) Received: from mail.turbocat.net (heidi.turbocat.net [88.198.202.214]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 5E57BFEC; Thu, 29 Jan 2015 19:59:19 +0000 (UTC) Received: from laptop015.home.selasky.org (cm-176.74.213.204.customer.telag.net [176.74.213.204]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 36B111FE023; Thu, 29 Jan 2015 20:59:14 +0100 (CET) Message-ID: <54CA9144.4030601@selasky.org> Date: Thu, 29 Jan 2015 21:00:04 +0100 From: Hans Petter Selasky User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Chagin Dmitry Subject: Re: [Bug?] Control Transfers in xHCI References: <20150129.212550.434561541001871867.okuno.kohji@jp.panasonic.com> <54CA4BE3.2090706@selasky.org> <20150129195714.GA3683@dchagin.static.corbina.net> In-Reply-To: <20150129195714.GA3683@dchagin.static.corbina.net> Content-Type: multipart/mixed; boundary="------------020607040003040609030404" Cc: freebsd-usb@freebsd.org X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Jan 2015 19:59:20 -0000 This is a multi-part message in MIME format. --------------020607040003040609030404 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 01/29/15 20:57, Chagin Dmitry wrote: > On Thu, Jan 29, 2015 at 04:04:03PM +0100, Hans Petter Selasky wrote: >> On 01/29/15 13:25, Kohji Okuno wrote: >>> Hi HPS, >>> >>> I found a bug in xHCI device driver. >>> >>> Acording to extensible-host-controler-interface-usb-xhci.pdf:"3.2.9 >>> Control Transfers"... >>> >>> A Data Stage TD consists of a Data Stage TRB followed by zero or more >>> Normal TRBs. If the data is not physically contiguous, Normal TRBs may >>> be chained to the Data Stage TRB. >>> >>> >>> But, in the current imprementation, when two or more TRBs are needed, >>> the device driver set XHCI_TRB_TYPE_DATA_STAGE to all TRBs. >>> This is the violation of the spec. >>> >>> In my minor xHCI, I encountered strange bubble error in a control >>> transfer. After I changed as the following, I succeeded its control >>> transfer. >>> >>> Would you check the following (****)? >>> >> >> Hi Kohji, >> >> You are correct there is a bug, but your patch is not correct. >> >> In FreeBSD we allow SETUP and DATA stages to be done as separate jobs. >> That means at the entry of creating a new DATA chain, we need to check >> if it is there first DATA packet or not. >> >> Can you test the attached patch and see if it works for you? >> > patch is lost somewhere, Hans. > Trying again. I think Kohji got it. --HPS --------------020607040003040609030404 Content-Type: text/x-patch; name="xhci.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xhci.patch" Index: sys/dev/usb/controller/xhci.c =================================================================== --- sys/dev/usb/controller/xhci.c (revision 277724) +++ sys/dev/usb/controller/xhci.c (working copy) @@ -1866,6 +1866,15 @@ XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_DATA_STAGE); if (temp->direction == UE_DIR_IN) dword |= XHCI_TRB_3_DIR_IN | XHCI_TRB_3_ISP_BIT; + /* + * Section 3.2.9 in the XHCI + * specification about control + * transfers says that we should use a + * normal-TRB if there are more TRBs + * extending the data-stage + * TRB. Update the "trb_type". + */ + temp->trb_type = XHCI_TRB_TYPE_NORMAL; break; case XHCI_TRB_TYPE_STATUS_STAGE: dword = XHCI_TRB_3_CHAIN_BIT | XHCI_TRB_3_CYCLE_BIT | @@ -2106,7 +2115,8 @@ mult = 1; temp.isoc_delta = 0; temp.isoc_frame = 0; - temp.trb_type = XHCI_TRB_TYPE_DATA_STAGE; + temp.trb_type = usbd_control_transfer_did_data(xfer) ? + XHCI_TRB_TYPE_NORMAL : XHCI_TRB_TYPE_DATA_STAGE; } else { x = 0; mult = 1; Index: sys/dev/usb/usb_transfer.c =================================================================== --- sys/dev/usb/usb_transfer.c (revision 277724) +++ sys/dev/usb/usb_transfer.c (working copy) @@ -1409,6 +1409,31 @@ } /*------------------------------------------------------------------------* + * usbd_control_transfer_did_data + * + * This function returns non-zero in USB host mode if a control + * endpoint has done the first DATA packet after the SETUP packet. + * Else it return zero. + *------------------------------------------------------------------------*/ +uint8_t +usbd_control_transfer_did_data(struct usb_xfer *xfer) +{ + struct usb_device_request req; + + if (xfer->flags_int.usb_mode == USB_MODE_DEVICE || + xfer->flags_int.control_xfr == 0) + return (0); + + /* copy out the USB request header */ + + usbd_copy_out(xfer->frbuffers, 0, &req, sizeof(req)); + + /* compare remainder to the initial value */ + + return (xfer->flags_int.control_rem != UGETW(req.wLength)); +} + +/*------------------------------------------------------------------------* * usbd_setup_ctrl_transfer * * This function handles initialisation of control transfers. Control Index: sys/dev/usb/usbdi.h =================================================================== --- sys/dev/usb/usbdi.h (revision 277724) +++ sys/dev/usb/usbdi.h (working copy) @@ -529,6 +529,7 @@ void usbd_transfer_stop(struct usb_xfer *xfer); void usbd_transfer_unsetup(struct usb_xfer **pxfer, uint16_t n_setup); void usbd_transfer_poll(struct usb_xfer **ppxfer, uint16_t max); +uint8_t usbd_control_transfer_did_data(struct usb_xfer *xfer); void usbd_set_parent_iface(struct usb_device *udev, uint8_t iface_index, uint8_t parent_index); uint8_t usbd_get_bus_index(struct usb_device *udev); --------------020607040003040609030404--