[usb] Support composite devices with multiple functions

This change removes special casing for composite devices and in the
process fixes support for composite devices having multiple functions by
adding support for holding multiple file handles open at once.

Bug: 637404
Change-Id: I25b1e1c648e1d11c08231765b0843f0121525ebd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153888
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Auto-Submit: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760222}

services/device/usb/usb_device_handle_win.cc

@@ -180,10 +180,12 @@ void UsbDeviceHandleWin::Close() {
     hub_handle_.Close();
   }
 
-  if (function_handle_.IsValid()) {
-    CancelIo(function_handle_.Get());
-    function_handle_.Close();
-    first_interface_handle_ = INVALID_HANDLE_VALUE;
+  for (auto& map_entry : interfaces_) {
+    Interface* interface = &map_entry.second;
+    if (interface->function_handle.IsValid()) {
+      CancelIo(interface->function_handle.Get());
+      interface->function_handle.Close();
+    }
   }
 
   // Aborting requests may run or destroy callbacks holding the last reference
@@ -318,7 +320,7 @@ void UsbDeviceHandleWin::ControlTransfer(
         auto* node_connection_info = new USB_NODE_CONNECTION_INFORMATION_EX;
         node_connection_info->ConnectionIndex = device_->port_number();
 
-        Request* request = MakeRequest(false /* winusb_handle */);
+        Request* request = MakeRequest(/*interface=*/nullptr);
         BOOL result = DeviceIoControl(
             hub_handle_.Get(), IOCTL_USB_GET_NODE_CONNECTION_INFORMATION_EX,
             node_connection_info, sizeof(*node_connection_info),
@@ -345,7 +347,7 @@ void UsbDeviceHandleWin::ControlTransfer(
         descriptor_request->SetupPacket.wIndex = index;
         descriptor_request->SetupPacket.wLength = buffer->size();
 
-        Request* request = MakeRequest(false /* winusb_handle */);
+        Request* request = MakeRequest(/*interface=*/nullptr);
         BOOL result = DeviceIoControl(
             hub_handle_.Get(), IOCTL_USB_GET_DESCRIPTOR_FROM_NODE_CONNECTION,
             request_buffer->front(), size, request_buffer->front(), size,
@@ -369,9 +371,8 @@ void UsbDeviceHandleWin::ControlTransfer(
   }
 
   // Submit a normal control transfer.
-  WINUSB_INTERFACE_HANDLE handle =
-      GetInterfaceForControlTransfer(recipient, index);
-  if (handle == INVALID_HANDLE_VALUE) {
+  Interface* interface = GetInterfaceForControlTransfer(recipient, index);
+  if (!interface) {
     USB_LOG(ERROR) << "Interface handle not available for control transfer.";
     task_runner_->PostTask(
         FROM_HERE,
@@ -387,10 +388,10 @@ void UsbDeviceHandleWin::ControlTransfer(
   setup.Index = index;
   setup.Length = buffer->size();
 
-  Request* control_request = MakeRequest(true /* winusb_handle */);
-  BOOL result =
-      WinUsb_ControlTransfer(handle, setup, buffer->front(), buffer->size(),
-                             nullptr, control_request->overlapped());
+  Request* control_request = MakeRequest(interface);
+  BOOL result = WinUsb_ControlTransfer(
+      interface->handle.Get(), setup, buffer->front(), buffer->size(),
+      /*LengthTransferred=*/nullptr, control_request->overlapped());
   DWORD last_error = GetLastError();
   control_request->MaybeStartWatching(
       result, last_error,
@@ -456,7 +457,7 @@ void UsbDeviceHandleWin::GenericTransfer(
   }
 
   DCHECK(interface->handle.IsValid());
-  Request* request = MakeRequest(true /* winusb_handle */);
+  Request* request = MakeRequest(interface);
   BOOL result;
   if (direction == UsbTransferDirection::INBOUND) {
     result = WinUsb_ReadPipe(interface->handle.Get(), endpoint_address,
@@ -485,10 +486,8 @@ const mojom::UsbInterfaceInfo* UsbDeviceHandleWin::FindInterfaceByEndpoint(
   return nullptr;
 }
 
-UsbDeviceHandleWin::UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device,
-                                       bool composite)
+UsbDeviceHandleWin::UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device)
     : device_(std::move(device)),
-      composite_(composite),
       task_runner_(base::SequencedTaskRunnerHandle::Get()),
       blocking_task_runner_(UsbService::CreateBlockingTaskRunner()) {
   // Windows only supports configuration 1, which therefore must be active.
@@ -505,8 +504,7 @@ UsbDeviceHandleWin::UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device,
       RegisterEndpoints(
           CombinedInterfaceInfo(interface.get(), alternate.get()));
 
-      if (composite_ &&
-          interface->interface_number == interface->first_interface) {
+      if (interface->interface_number == interface->first_interface) {
         auto it = device_->function_paths().find(interface->interface_number);
         if (it != device_->function_paths().end())
           interface_info.function_path = it->second;
@@ -518,7 +516,6 @@ UsbDeviceHandleWin::UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device,
 UsbDeviceHandleWin::UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device,
                                        base::win::ScopedHandle handle)
     : device_(std::move(device)),
-      composite_(false),
       hub_handle_(std::move(handle)),
       task_runner_(base::SequencedTaskRunnerHandle::Get()),
       blocking_task_runner_(UsbService::CreateBlockingTaskRunner()) {}
@@ -541,36 +538,29 @@ bool UsbDeviceHandleWin::OpenInterfaceHandle(Interface* interface) {
 
   WINUSB_INTERFACE_HANDLE handle;
   if (interface->first_interface == interface->interface_number) {
-    if (!function_handle_.IsValid()) {
+    if (!interface->function_handle.IsValid()) {
       const base::string16* function_path;
-      if (composite_) {
-        if (interface->function_path.empty()) {
-          USB_LOG(ERROR) << "No WinUSB interface for interface "
-                         << static_cast<int>(interface->interface_number)
-                         << ".";
-          return false;
-        }
-        function_path = &interface->function_path;
-      } else {
-        function_path = &device_->device_path();
+      if (interface->function_path.empty()) {
+        USB_LOG(ERROR) << "No WinUSB interface for interface "
+                       << static_cast<int>(interface->interface_number) << ".";
+        return false;
       }
+      function_path = &interface->function_path;
 
-      function_handle_.Set(CreateFile(
+      interface->function_handle.Set(CreateFile(
           function_path->c_str(), GENERIC_READ | GENERIC_WRITE,
           FILE_SHARE_READ | FILE_SHARE_WRITE, /*lpSecurityAttributes=*/nullptr,
           OPEN_EXISTING, FILE_FLAG_OVERLAPPED, /*hTemplateFile=*/nullptr));
-      if (!function_handle_.IsValid()) {
+      if (!interface->function_handle.IsValid()) {
         USB_PLOG(ERROR) << "Failed to open " << *function_path;
         return false;
       }
     }
 
-    if (!WinUsb_Initialize(function_handle_.Get(), &handle)) {
+    if (!WinUsb_Initialize(interface->function_handle.Get(), &handle)) {
       USB_PLOG(ERROR) << "Failed to initialize WinUSB handle";
       return false;
     }
-
-    first_interface_handle_ = handle;
   } else {
     auto first_interface_it = interfaces_.find(interface->first_interface);
     DCHECK(first_interface_it != interfaces_.end());
@@ -610,7 +600,8 @@ void UsbDeviceHandleWin::UnregisterEndpoints(
     endpoints_.erase(ConvertEndpointNumberToAddress(*endpoint));
 }
 
-WINUSB_INTERFACE_HANDLE UsbDeviceHandleWin::GetInterfaceForControlTransfer(
+UsbDeviceHandleWin::Interface*
+UsbDeviceHandleWin::GetInterfaceForControlTransfer(
     UsbControlTransferRecipient recipient,
     uint16_t index) {
   if (recipient == UsbControlTransferRecipient::ENDPOINT) {
@@ -618,7 +609,7 @@ WINUSB_INTERFACE_HANDLE UsbDeviceHandleWin::GetInterfaceForControlTransfer(
     // endpoint.
     auto endpoint_it = endpoints_.find(index & 0xff);
     if (endpoint_it == endpoints_.end())
-      return INVALID_HANDLE_VALUE;
+      return nullptr;
 
     // "Fall through" to the interface case.
     recipient = UsbControlTransferRecipient::INTERFACE;
@@ -631,35 +622,45 @@ WINUSB_INTERFACE_HANDLE UsbDeviceHandleWin::GetInterfaceForControlTransfer(
     // interface.
     auto interface_it = interfaces_.find(index & 0xff);
     if (interface_it == interfaces_.end())
-      return INVALID_HANDLE_VALUE;
+      return nullptr;
 
     interface = &interface_it->second;
-  } else if (composite_) {
-    // For all other recipients any interface can be used but if the device
-    // is composite, then a function with the WinUSB driver loaded must be
-    // found.
+  } else {
+    // For all other recipients any interface can be used as long as a
+    // function with the WinUSB driver loaded can be found.
     for (auto& map_entry : interfaces_) {
       if (!map_entry.second.function_path.empty())
         interface = &map_entry.second;
     }
-  } else if (!interfaces_.empty()) {
-    // For a non-composite device there is only a single device path to
-    // choose from so just pick the first interface.
-    interface = &interfaces_.begin()->second;
   }
 
-  if (!interface)
-    return INVALID_HANDLE_VALUE;
-
-  OpenInterfaceHandle(interface);
-  return interface->handle.Get();
+  if (interface)
+    OpenInterfaceHandle(interface);
+  return interface;
 }
 
 UsbDeviceHandleWin::Request* UsbDeviceHandleWin::MakeRequest(
-    bool winusb_handle) {
-  auto request = std::make_unique<Request>(
-      winusb_handle ? first_interface_handle_ : hub_handle_.Get(),
-      winusb_handle);
+    Interface* interface) {
+  // The HANDLE used to get the overlapped result must be the
+  // WINUSB_INTERFACE_HANDLE of the first interface in the function.
+  //
+  // https://docs.microsoft.com/en-us/windows/win32/api/winusb/nf-winusb-winusb_getoverlappedresult
+  HANDLE handle;
+  bool is_winusb_handle;
+  if (!interface) {
+    handle = hub_handle_.Get();
+    is_winusb_handle = false;
+  } else if (interface->interface_number == interface->first_interface) {
+    handle = interface->handle.Get();
+    is_winusb_handle = true;
+  } else {
+    auto it = interfaces_.find(interface->first_interface);
+    DCHECK(it != interfaces_.end());
+    handle = it->second.handle.Get();
+    is_winusb_handle = true;
+  }
+
+  auto request = std::make_unique<Request>(handle, is_winusb_handle);
   Request* request_ptr = request.get();
   requests_[request_ptr] = std::move(request);
   return request_ptr;

services/device/usb/usb_device_handle_win.h

@@ -75,7 +75,7 @@ class UsbDeviceHandleWin : public UsbDeviceHandle {
   friend class UsbDeviceWin;
 
   // Constructor used to build a connection to the device.
-  UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device, bool composite);
+  UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device);
 
   // Constructor used to build a connection to the device's parent hub.
   UsbDeviceHandleWin(scoped_refptr<UsbDeviceWin> device,
@@ -102,6 +102,7 @@ class UsbDeviceHandleWin : public UsbDeviceHandle {
 
     // In a composite device each function has its own driver and path to open.
     base::string16 function_path;
+    base::win::ScopedHandle function_handle;
 
     ScopedWinUsbHandle handle;
     bool claimed = false;
@@ -118,7 +119,7 @@ class UsbDeviceHandleWin : public UsbDeviceHandle {
   bool OpenInterfaceHandle(Interface* interface);
   void RegisterEndpoints(const CombinedInterfaceInfo& interface);
   void UnregisterEndpoints(const CombinedInterfaceInfo& interface);
-  WINUSB_INTERFACE_HANDLE GetInterfaceForControlTransfer(
+  Interface* GetInterfaceForControlTransfer(
       mojom::UsbControlTransferRecipient recipient,
       uint16_t index);
   void SetInterfaceAlternateSettingBlocking(uint8_t interface_number,
@@ -127,7 +128,7 @@ class UsbDeviceHandleWin : public UsbDeviceHandle {
   void SetInterfaceAlternateSettingComplete(uint8_t interface_number,
                                             uint8_t alternate_setting,
                                             const ResultCallback& callback);
-  Request* MakeRequest(bool winusb_handle);
+  Request* MakeRequest(Interface* interface);
   std::unique_ptr<Request> UnlinkRequest(Request* request);
   void GotNodeConnectionInformation(TransferCallback callback,
                                     void* node_connection_info,
@@ -154,17 +155,12 @@ class UsbDeviceHandleWin : public UsbDeviceHandle {
   SEQUENCE_CHECKER(sequence_checker_);
 
   scoped_refptr<UsbDeviceWin> device_;
-  const bool composite_;
 
   // |hub_handle_| or all the handles for claimed interfaces in |interfaces_|
   // must outlive their associated |requests_| because individual Request
   // objects hold on to the raw handles for the purpose of calling
   // GetOverlappedResult().
   base::win::ScopedHandle hub_handle_;
-  base::win::ScopedHandle function_handle_;
-
-  // The handle returned by WinUsb_Initialize is special.
-  WINUSB_INTERFACE_HANDLE first_interface_handle_ = INVALID_HANDLE_VALUE;
 
   std::map<uint8_t, Interface> interfaces_;
   std::map<uint8_t, Endpoint> endpoints_;

services/device/usb/usb_device_win.cc

@@ -43,10 +43,10 @@ void UsbDeviceWin::Open(OpenCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   scoped_refptr<UsbDeviceHandle> device_handle;
-  if (base::EqualsCaseInsensitiveASCII(driver_name_, L"winusb"))
-    device_handle = new UsbDeviceHandleWin(this, /*composite=*/false);
-  else if (base::EqualsCaseInsensitiveASCII(driver_name_, L"usbccgp"))
-    device_handle = new UsbDeviceHandleWin(this, /*composite=*/true);
+  if (base::EqualsCaseInsensitiveASCII(driver_name_, L"winusb") ||
+      base::EqualsCaseInsensitiveASCII(driver_name_, L"usbccgp")) {
+    device_handle = new UsbDeviceHandleWin(this);
+  }
 
   if (device_handle)
     handles().push_back(device_handle.get());

services/device/usb/usb_service_win.cc

@@ -427,12 +427,12 @@ class UsbServiceWin::BlockingTaskRunnerHelper {
       return;
     }
 
-    // For composite devices Windows loads the usbccgp driver, which creates
-    // child device notes for each of the device functions. It is these device
-    // paths for these children which must be opened in order to communicate
-    // with the WinUSB driver.
     std::vector<std::pair<int, base::string16>> function_paths;
     if (base::EqualsCaseInsensitiveASCII(service_name, L"usbccgp")) {
+      // For composite devices Windows loads the usbccgp driver, which creates
+      // child device nodes for each of the device functions. It is these device
+      // paths for these children which must be opened in order to communicate
+      // with the WinUSB driver.
       for (const base::string16& instance_id : child_instance_ids) {
         int interface_number = GetInterfaceNumber(instance_id);
         if (interface_number != -1) {
@@ -440,6 +440,10 @@ class UsbServiceWin::BlockingTaskRunnerHelper {
                                       GetWinUsbDevicePath(instance_id));
         }
       }
+    } else if (base::EqualsCaseInsensitiveASCII(service_name, L"winusb")) {
+      // A non-composite device has a single device node for all interfaces as
+      // it only has a single function.
+      function_paths.emplace_back(0, device_path);
     }
 
     base::string16& hub_path = hub_paths_[parent_instance_id];