On Thu, Oct 03, 2013 at 07:18:42PM -0000, hemantk@xxxxxxxxxxxxxx wrote: > > On Thu, Oct 03, 2013 at 06:00:50PM -0000, hemantk@xxxxxxxxxxxxxx wrote: > >> We have devices which require to send zero length packet in data stage > >> to > >> host in certain cases. We were not seeing any issue when we were using > >> ehci based controller. When we switched to XHCI here is what we are > >> observing:- > >> > >> For IN DATA dir control xfer XHCI sets ISP so xhci updates actual_length > >> for short packet in data stage using event->transfer_len. > >> > >> But for data packets not short xhci directly uses > >> urb->transfer_buffer_length as actual_length.This is done assuming data > >> stage will never have zero length (short packet) data packet. Once > >> Device > >> sends zero length packet in data stage > >> ISP triggers and actual_length is updated with value 0. Then later in > >> status stage (since urb->actual_length is zero) driver incorrectly sets > >> actual_length with urb->transfer_buffer_length. What should urb->actual_length be set to instead? The device sent zero bytes in the data stage and urb->transfer_buffer_length *is* zero, so it makes sense that urb->actual_length would be set to zero. Does EHCI set it to something different? > >> I would like to get insight from linux-usb grp to fix this issue. > > > > What kernel version are you seeing these problems? > > > > thanks, > > > > greg k-h > > > > i am using 3.10 kernel. Also i looked at tip i see same implementation for > process_ctrl_td() > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/xhci-ring.c Yeah, the implementation has always been this way. I've known since the beginning of writing the xHCI driver that it needs to handle zero-length data phases, but hadn't had ever had a report that it was required by a device under Linux. The reason why is implementing zero-length control transfers is somewhat complicated, and would involve more than just a change in the xHCI driver control transfer event handling code. Normally, when the xHCI driver gets a USB transfer request without a transfer buffer, and the urb->transfer_length set to zero, it only queues the setup and status phase TRBs to the ring. It initializes the setup TRB to say there is no data phase, by setting the Transfer Type field to '0'. With the setup and status TRBs queued, the host receives an ACK to the setup phase fine from the device. It moves on to processing the status phase TRB, and expects the device to send an ACK for the status phase. Instead, your device sends a zero-length IN transfer. I think that will result in the host saying there is a short packet on the status phase. I'm not sure what will happen with the extra status phase the device wanted to send. I assume your driver doesn't set the URB_SHORT_NOT_OK transfer_flag? In that case, yes, the xHCI driver will set the urb->status to zero, and set the actual_length to the transfer_buffer_length. But the fault is not just in the control transfer event handling, it's in the actual control TRB initialization. In order to fix your issue, the xHCI driver would need to know if the device is going to send/receive an extra zero-length data phase, so it can change how it sets up the control transfer TRBs. I don't think there's currently an URB transfer_flag to let the host know that. It would have to be added. On top of that, you'll have to fix the call to prepare_transfer() in xhci_queue_ctrl_tx() to allocate one more TRB for the data phase in this case. You'll also have to fix the Transfer Type field initialization, and queue a zero-length Data IN TRB. However, I note that the 2012-08-14 xHCI 1.0 errata changed the Data Stage TRB's transfer length field to only accept values from 1 to 64K, instead of 0 to 64K, so I'm not even sure newer xHCI hosts support zero-length data phases. I'll ping the xHCI spec architect to see why that change was made. Finally, changes will have to be made in the xHCI control transfer event completion code, in order to make the driver handle the extra data phase TRB. There are probably implications in the ring cancellation code as well, but I would have to double check. So, yeah, this is a bigger xHCI architectural change, that would require USB core and driver modifications. Do you want to work to get this fixed? I currently don't have time to do so. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
