New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 640725 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Problem with receiving data from input USB transfers with a timeout

Project Member Reported by emaxx@chromium.org, Aug 24 2016

Issue description

Version: 54
OS: Chrome OS, Linux

What steps will reproduce the problem?
(1) Write an App that has a loop with chrome.usb.interruptTransfer calls requesting for input transfers with a timeout X.
(2) Run the App and connect the expected USB device.
(3) Operate the device in a way that the interrupt notifications happen at some time T, where X < T < X*2.

What is the expected output?
The first interruptTransfer call finishes with a timeout error, and the second interruptTransfer call finishes with the received notification.

What do you see instead?
Both the first and the second interruptTransfer calls finish with a timeout error. The subsequent interruptTransfer calls also don't receive the expected data.
The only way to make the subsequent interruptTransfer calls receive the new notifications from the interrupt endpoint seems to operate the USB device in a way that it makes several new interrupt notifications.


Reilly, I'm not actually sure whether the description above is correct - this is based on my investigation of a problem occurred in a complex application. I didn't try to build a minimized sample app for a repro case.

But, unless I'm missing something, I think the problem is that the UsbDeviceHandleUsbfs class just discards the data received for the transfers that it considers as canceled (including due to time out):
https://cs.chromium.org/chromium/src/device/usb/usb_device_handle_usbfs.cc?l=758
It means that in case of two input transfers reading from the same endpoint, where the first transfer is canceled from the UsbDeviceHandleUsbfs point of view, the data would be still directed by udev to the first transfer, and the data would be effectively lost due to the logic in UsbDeviceHandleUsbfs.


Please correct me if I'm wrong in the analysis above, and I should search for an error in my application instead.
 
That analysis seems correct. We should be using USBDEVFS_DISCARDURB to tell the kernel to cancel the request when it times out in userspace.

Comment 2 by emaxx@chromium.org, Aug 25 2016

Cc: dskaram@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f91bd7e250f649c06e1059033943d56dbf0a79f

commit 6f91bd7e250f649c06e1059033943d56dbf0a79f
Author: reillyg <reillyg@chromium.org>
Date: Fri Aug 26 02:42:03 2016

Tell the kernel to discard USB requests when they time out.

This patch adds a call to USBDEVFS_DISCARDURB when we time out a USB
transfer. This causes the host controller to stop polling the device for
data and means that a later transfer will receive the next bytes that
are sent from the device. Otherwise those bytes will be claimed by the
cancelled transfer and discarded.

BUG= 640725 

Review-Url: https://codereview.chromium.org/2274343002
Cr-Commit-Position: refs/heads/master@{#414635}

[modify] https://crrev.com/6f91bd7e250f649c06e1059033943d56dbf0a79f/device/usb/usb_device_handle_usbfs.cc
[modify] https://crrev.com/6f91bd7e250f649c06e1059033943d56dbf0a79f/device/usb/usb_device_handle_usbfs.h

Comment 4 by emaxx@chromium.org, Aug 26 2016

Confirming that the patch fixes the problem with our app (Smart Card Connector for Chrome OS). Thanks for the quick fix!

Into which Chrome versions would it be possible to get this fix backported? (I guess that all versions >52.0.2713 may be affected, due to the change crrev.com/2f3b6285b45d43141dfc72131dd81702e4342f85 that turned on the usbdevfs implementation.)
Labels: -M-54 Merge-Request-53 M-53
Let's get this merged into M-53 at least.

Comment 6 by gov...@chromium.org, Aug 26 2016

Cc: keta...@chromium.org
Seems like this change is already verified per #4 and that app is on Chrome OS (Smart Card Connector for Chrome OS). So I will let ketakid@ (Chrome OS TPM) to make do merge review for this.

Comment 7 by dimu@chromium.org, Aug 27 2016

Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M53, manual review required.
Labels: -Merge-Review-53 Merge-Approved-53
Approving merge to M53 cros. emaxx@ Please merge this in asap.

Comment 9 by emaxx@chromium.org, Sep 2 2016

Owner: reillyg@chromium.org
Status: Assigned (was: Untriaged)
It was Reilly who made the fix, I only reported the bug.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 2 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bc5ad0f6a48a2a136e5fe2510d83de6df8258ef

commit 6bc5ad0f6a48a2a136e5fe2510d83de6df8258ef
Author: Reilly Grant <reillyg@chromium.org>
Date: Fri Sep 02 19:45:26 2016

Tell the kernel to discard USB requests when they time out.

This patch adds a call to USBDEVFS_DISCARDURB when we time out a USB
transfer. This causes the host controller to stop polling the device for
data and means that a later transfer will receive the next bytes that
are sent from the device. Otherwise those bytes will be claimed by the
cancelled transfer and discarded.

BUG= 640725 

Review-Url: https://codereview.chromium.org/2274343002
Cr-Commit-Position: refs/heads/master@{#414635}
(cherry picked from commit 6f91bd7e250f649c06e1059033943d56dbf0a79f)

Review URL: https://codereview.chromium.org/2308783002 .

Cr-Commit-Position: refs/branch-heads/2785@{#814}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/6bc5ad0f6a48a2a136e5fe2510d83de6df8258ef/device/usb/usb_device_handle_usbfs.cc
[modify] https://crrev.com/6bc5ad0f6a48a2a136e5fe2510d83de6df8258ef/device/usb/usb_device_handle_usbfs.h

Status: Fixed (was: Assigned)

Sign in to add a comment