Date: Thu, 29 Jan 2015 21:00:04 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Chagin Dmitry <dchagin@freebsd.org> Cc: freebsd-usb@freebsd.org Subject: Re: [Bug?] Control Transfers in xHCI Message-ID: <54CA9144.4030601@selasky.org> In-Reply-To: <20150129195714.GA3683@dchagin.static.corbina.net> References: <20150129.212550.434561541001871867.okuno.kohji@jp.panasonic.com> <54CA4BE3.2090706@selasky.org> <20150129195714.GA3683@dchagin.static.corbina.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54CA9144.4030601>