Replace libusb in the Linux/Chrome OS USB I/O path.

Replace libusb in the Linux/Chrome OS USB I/O path.

As a first step in adding support for connecting to USB devices on
Android this patch replaces the libusb-based implementation of
UsbDeviceHandle used on Linux and Chrome OS with one written directly to
the Linux usbdevfs ioctl interface.

BUG=422562, 601627
Committed: https://crrev.com/fed1de7faf5d91a3282d461fd1c0657f8fe3a533
Cr-Commit-Position: refs/heads/master@{#387315}

[Reilly Grant (use Gerrit), 2016-04-08 23:32:56]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2016-04-09 02:13:12]

reillyg@chromium.org changed reviewers:
+ juncai@chromium.org

[Reilly Grant (use Gerrit), 2016-04-09 02:13:13]

Please take a look. This patch will be landed after the M-51 branch is cut so
that it can bake on dev-channel for as long as possible.

[juncai, 2016-04-13 00:22:33]

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:42: return 0;
Noticed that USB_DIR_OUT has value 0:
Maybe can return other value here?

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:57: return 0;
ditto. USB_TYPE_STANDARD has value 0.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:72: return 0;
ditto. USB_RECIP_DEVICE has value 0.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:85: new net::IOBuffer(length +
sizeof(struct usb_ctrlrequest)));
"struct" may be omitted.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:86: struct usb_ctrlrequest* setup =
ditto.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:87: reinterpret_cast<struct
usb_ctrlrequest*>(new_buffer->data());
ditto.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:95: memcpy(new_buffer->data() +
sizeof(struct usb_ctrlrequest),
ditto.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:112: return 0;
USBDEVFS_URB_TYPE_ISO has value 0.
Maybe can return other value here?

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:141: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
Maybe the above two parameters can just use:
scoped_refptr<...> instead of const scoped_refptr<...>&

Noticed other places use it:
https://code.google.com/p/chromium/codesearch#chromium/src/device/hid/hid_ser...

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:320: // to
rearrange the comments here.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:334: if (ContainsKey(interfaces_,
interface_number)) {
Add
#include "base/stl_util.h" for ContainsKey().

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:441: total_length, packet_lengths,
timeout, callback);
|total_length| is size_t, but the parameter at IsochronousTransferInternal() is
int.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:453:
IsochronousTransferInternal(endpoint_address, buffer, total_length,
ditto.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:473: USB_LOG(USER) << "Endpoint address "
<< (int)endpoint_address
use static_cast<int> instead of (int)?

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:590: struct usbdevfs_setinterface cmd =
{0};
"struct" may be omitted.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:625: if (rc)
use {} here for this if statement.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:645: USB_LOG(USER) << "Endpoint address "
<< (int)endpoint_address
use static_cast<int> instead of (int)?

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.h (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:35: // UsbDeviceHandle implementation.
nit:
s/UsbDeviceHandle implementation./UsbDeviceHandle:

Noticed usb_device_handle_usbfs.cc uses this kind of comment style for virtual
functions.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:114: void ReapedUrbs(std::vector<struct
usbdevfs_urb*> urbs);
The "struct" may be omitted.

Also here the parameter can be:
const std::vector<struct usbdevfs_urb*>&
to avoid copying.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:118: const std::vector<uint32_t>
packet_lengths,
Use const std::vector<uint32_t>& as parameter?

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:133: std::map<uint8_t, InterfaceInfo>
interfaces_;
The type of interface number is int elsewhere. Here it is uint8_t.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:140: // Helper object owned by the blocking
task thread. I will be freed if that
s/I/It/

[juncai, 2016-04-13 00:31:10]

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_i...
File device/usb/usb_device_impl.cc (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_i...
device/usb/usb_device_impl.cc:264: break;
may just use return; here.

[Reilly Grant (use Gerrit), 2016-04-13 20:21:14]

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.cc (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:42: return 0;
On 2016/04/13 at 00:22:32, juncai wrote:
> Noticed that USB_DIR_OUT has value 0:
> Maybe can return other value here?

At this point something terrible has gone wrong so I am not concerned about the
value overlapping.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:85: new net::IOBuffer(length +
sizeof(struct usb_ctrlrequest)));
On 2016/04/13 at 00:22:32, juncai wrote:
> "struct" may be omitted.

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:141: const
scoped_refptr<base::SequencedTaskRunner>& task_runner);
On 2016/04/13 at 00:22:32, juncai wrote:
> Maybe the above two parameters can just use:
> scoped_refptr<...> instead of const scoped_refptr<...>&
> 
> Noticed other places use it:
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/hid/hid_ser...

There's been some discussion of this on the mailing list. The idea is to avoid
extra AddRef and Release calls as the variable is passed through the
constructor. The recommendation at this point is, I believe, not to bother so
I've changed these to regular pass-by-value.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:320: // to
On 2016/04/13 at 00:22:32, juncai wrote:
> rearrange the comments here.

Fixed.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:334: if (ContainsKey(interfaces_,
interface_number)) {
On 2016/04/13 at 00:22:32, juncai wrote:
> Add
> #include "base/stl_util.h" for ContainsKey().

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:441: total_length, packet_lengths,
timeout, callback);
On 2016/04/13 at 00:22:32, juncai wrote:
> |total_length| is size_t, but the parameter at IsochronousTransferInternal()
is int.

Changed the parameter type to size_t.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:473: USB_LOG(USER) << "Endpoint address "
<< (int)endpoint_address
On 2016/04/13 at 00:22:32, juncai wrote:
> use static_cast<int> instead of (int)?

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:590: struct usbdevfs_setinterface cmd =
{0};
On 2016/04/13 at 00:22:32, juncai wrote:
> "struct" may be omitted.

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:625: if (rc)
On 2016/04/13 at 00:22:32, juncai wrote:
> use {} here for this if statement.

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.cc:645: USB_LOG(USER) << "Endpoint address "
<< (int)endpoint_address
On 2016/04/13 at 00:22:32, juncai wrote:
> use static_cast<int> instead of (int)?

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.h (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:35: // UsbDeviceHandle implementation.
On 2016/04/13 at 00:22:32, juncai wrote:
> nit:
> s/UsbDeviceHandle implementation./UsbDeviceHandle:
> 
> Noticed usb_device_handle_usbfs.cc uses this kind of comment style for virtual
functions.

We're not very consistent about this in Chromium. I like the more verbose
version.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:114: void ReapedUrbs(std::vector<struct
usbdevfs_urb*> urbs);
On 2016/04/13 at 00:22:33, juncai wrote:
> The "struct" may be omitted.
> 
> Also here the parameter can be:
> const std::vector<struct usbdevfs_urb*>&
> to avoid copying.

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:118: const std::vector<uint32_t>
packet_lengths,
On 2016/04/13 at 00:22:33, juncai wrote:
> Use const std::vector<uint32_t>& as parameter?

Good catch.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:133: std::map<uint8_t, InterfaceInfo>
interfaces_;
On 2016/04/13 at 00:22:33, juncai wrote:
> The type of interface number is int elsewhere. Here it is uint8_t.

The type is int when a "not found" value (-1) is required. The proper type for
an interface number is uint8_t.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:140: // Helper object owned by the blocking
task thread. I will be freed if that
On 2016/04/13 at 00:22:32, juncai wrote:
> s/I/It/

Done.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:140: // Helper object owned by the blocking
task thread. I will be freed if that
On 2016/04/13 at 00:22:32, juncai wrote:
> s/I/It/

Done.

[juncai, 2016-04-13 23:52:12]

LGTM with one nit.

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.h (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:35: // UsbDeviceHandle implementation.
nit: s/implementation/overrides
To be consistent, noticed usb_device_handle_usbfs.cc uses "overrides" instead of
"implementation".

[Reilly Grant (use Gerrit), 2016-04-14 00:02:45]

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
File device/usb/usb_device_handle_usbfs.h (right):

https://codereview.chromium.org/1877503003/diff/40001/device/usb/usb_device_h...
device/usb/usb_device_handle_usbfs.h:35: // UsbDeviceHandle implementation.
On 2016/04/13 at 23:52:12, juncai wrote:
> nit: s/implementation/overrides
> To be consistent, noticed usb_device_handle_usbfs.cc uses "overrides" instead
of "implementation".

We discussed this offline and I explained the reason for differentiating between
an "implementation" (UsbDeviceHandleUsbfs implements UsbDeviceHandle's abstract
interface) and an "override" (FileThreadHelper overrides some event handlers in
the observers it subclasses).

[Reilly Grant (use Gerrit), 2016-04-14 14:50:38]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-04-14 14:50:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877503003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877503003/60001

[commit-bot: I haz the power, 2016-04-14 14:55:53]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-14 14:57:00]

Description was changed from

==========
Replace libusb in the Linux/Chrome OS USB I/O path.

As a first step in adding support for connecting to USB devices on
Android this patch replaces the libusb-based implementation of
UsbDeviceHandle used on Linux and Chrome OS with one written directly to
the Linux usbdevfs ioctl interface.

BUG=422562,601627
==========

to

==========
Replace libusb in the Linux/Chrome OS USB I/O path.

As a first step in adding support for connecting to USB devices on
Android this patch replaces the libusb-based implementation of
UsbDeviceHandle used on Linux and Chrome OS with one written directly to
the Linux usbdevfs ioctl interface.

BUG=422562,601627

Committed: https://crrev.com/fed1de7faf5d91a3282d461fd1c0657f8fe3a533
Cr-Commit-Position: refs/heads/master@{#387315}
==========

[commit-bot: I haz the power, 2016-04-14 14:57:01]

Patchset 3 (id:??) landed as
https://crrev.com/fed1de7faf5d91a3282d461fd1c0657f8fe3a533
Cr-Commit-Position: refs/heads/master@{#387315}

[Will Harris, 2016-04-15 19:49:44]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/1897483003/ by wfh@chromium.org.

The reason for reverting is: broke u2f see crbug.com/603962.