Windows: Deal with failed synchronous control transfers

[tormodvolden]

This merge request includes the patch in #1016, which is a prerequisite EDIT: which has been merged to master.

[tormodvolden]

@mcuee thanks for testing. Clearly this needs more work.

[tormodvolden]

Here is another try (branch repushed).

[mcuee]

Hopefully someone can review this and then it can be merged.

[mcuee]

@tormodvolden I think this can be merged and it will at least NOT make things worse.

[tormodvolden]

I still feel this needs review from someone familiar with overlapped->internal and friends. But I agree that the test result in #1017 looks promising and it seems to work better than before.

[mcuee]

@Youw and @Novakov,

Just wondering if you can take a look at this change. Thanks.

To me it is pretty safe to merge this PR.

[tormodvolden]

I believe the the first commit is well understood, but it needs a better commit message explaining which issues it solves.

The second commit also needs a better description, the current one is more a cry for help rather than helping anyone.

[tormodvolden]

I started on a longer explanation and background for this fix, my assumptions should also be reviewed:

Our transfer handling always follows an asynchronous model, so synchronous driver calls (libusb0) must be shoehorned into this.

When submitting a control transfer there are 4 outcomes to consider from the ControlTransfer call:

1. I/O pending, a asynchronous transfer was initiated
2. Success in case of a synchronous driver transfer (libusb0)
3. Failure, the transfer could not be initiated
4. Failure, a synchronous transfer (libusb0) finished with error

Case 1 works fine, a completion event will later fire and be picked up by windows_handle_transfer_completion() and the transfer result will be passed to the caller.

In case 2 there is a forced sync completion, thus firing a completion event so that the caller is notified like in case 1. In the current code, a hard-coded Success status code is written to overlapped->Internal, which gets picked up in windows_handle_transfer_completion(). This is obviously fine for this success case.

Case 3 simply returns LIBUSB_IO_ERROR which is rather fine, this is a rare case to fall into, and typically means the device got lost.

However, case 4 can be caused e.g. by the device stalling the pipe, and it is currently treated same as case 3, returning LIBUSB_IO_ERROR even if a more detailed transfer status could be read by GetLastError().

The first commit "Deal with failed synchronous control transfers" instead passes the value from GetLastError to windows_force_sync_completion() which also has been changed to report the error (and not a hard-coded success) via overlapped->Internal to windows_handle_transfer_completion().

The second commit "Carry over error code from synchronous transfer" makes windows_handle_transfer_completion() actually read the error from transfer_priv->overlapped.Internal and pass it to the caller.

Now the caller can detect e.g. a pipe stall also for the synchronous driver call (libusb0).

Note this also makes any immediate error from ControlTransfer() for asynchronous driver calls also propagate to windows_handle_transfer_completion() and our winusbx_submit_control_transfer() will always return success. Thus it changes case 3 above for both asynchronous and synchronous driver calls.

Users of libusb synchronous API will probably not see any difference since it does the asynchronous submit and reap under the hood. Users of libusb asynchronous API will now only see a failure as a transfer completion, and never on submission.

Inside windows_handle_transfer_completion() the GetOverlappedResult() call returns true or false, and false means a failure to get the result and as usual the cause can be diagnosed with GetLastError. If it returns true, the result of the asynchronous operation is hiding in transfer_priv->overlapped.Internal, either written by the driver* for asynchronous calls, or by us for synchronous calls.

Previously we were always using NO_ERROR (I believe it should have been ERROR_SUCCESS, although they have the same numeric value) in the "true" case. I don't know if there is any situation for asynchronous driver calls where GetOverlappedResult() returns true and the overlapped.Internal is not ERROR_SUCCESS, and we therefore will be doing something different?

*) The Overlapped docs says:

Internal: The status code for the I/O request. When the request is issued, the system sets this member to STATUS_PENDING to indicate that the operation has not yet started. When the request is completed, the system sets this member to the status code for the completed request.

Note this last change (in the second commit) affects all transfers, not just control transfers.

So I think there are reasons for testing this carefully for different kind of transfers, on different drivers, and for various failure modes (device stalls etc).

I also think any changes here is for the good, yielding more precise return values. But any change might upset some application.

[mcuee]

I do not have good test cases for async transfer, but at least basic tests with xusb seems to be fine with libusb0.sys.

[mcuee]

From here.

• https://github.com/libusb/libusb/issues/1019#issuecomment-1435960798

@tormodvolden BTW, pr #1018 does help with the libusb-1.0.27.3 filter driver usage.

libusb git master branch

PS C:\work\libusb\libusb_pr1018> .\examples\xusb.exe 0403:6001
Using libusb v1.0.26.11784

Opening device 0403:6001...
libusb: error [winusbx_open] could not open device \\?\USB#VID_0403&PID_6001#A50285BI#{A5DCBF10-6530-11D2-901F-00C04FB951ED} (interface 0): [87] The parameter is incorrect.
  Failed.
PS C:\work\libusb\libusb_pr1018> cd ..
PS C:\work\libusb> cd libusb
PS C:\work\libusb\libusb> .\examples\xusb.exe 0403:6001
Using libusb v1.0.26.11784

Opening device 0403:6001...

Reading device descriptor:
            length: 18
      device class: 0
               S/N: 3
           VID:PID: 0403:6001
         bcdDevice: 0600
   iMan:iProd:iSer: 1:2:3
          nb confs: 1

Reading BOS descriptor: libusb: warning [winusbx_submit_control_transfer] ControlTransfer failed: [31] A device attached to the system is not functioning.
libusb: error [libusb_get_bos_descriptor] failed to read BOS (-1)
no descriptor

Reading first configuration descriptor:
              total length: 32
         descriptor length: 9
             nb interfaces: 1
              interface[0]: id = 0
interface[0].altsetting[0]: num endpoints = 2
   Class.SubClass.Protocol: FF.FF.FF
       endpoint[0].address: 81
           max packet size: 0040
          polling interval: 00
       endpoint[1].address: 02
           max packet size: 0040
          polling interval: 00

Kernel driver attached for interface 0: (not supported)

Claiming interface 0...

Reading string descriptors:
   String (0x01): "FTDI"
   String (0x02): "FT232R USB UART"
   String (0x03): "A50285BI"

Reading OS string descriptor:libusb: warning [winusbx_submit_control_transfer] ControlTransfer failed: [31] A device attached to the system is not functioning.
 no descriptor

Reading interface association descriptors (IADs) for first configuration:
    nb IADs: 0

Releasing interface 0...
Closing device...

No more warning messages with PR #1018 integrated into git master.

PS C:\work\libusb\libusb_pr1018> .\examples\xusb.exe 0403:6001
Using libusb v1.0.26.11784

Opening device 0403:6001...

Reading device descriptor:
            length: 18
      device class: 0
               S/N: 3
           VID:PID: 0403:6001
         bcdDevice: 0600
   iMan:iProd:iSer: 1:2:3
          nb confs: 1

Reading BOS descriptor: no descriptor

Reading first configuration descriptor:
              total length: 32
         descriptor length: 9
             nb interfaces: 1
              interface[0]: id = 0
interface[0].altsetting[0]: num endpoints = 2
   Class.SubClass.Protocol: FF.FF.FF
       endpoint[0].address: 81
           max packet size: 0040
          polling interval: 00
       endpoint[1].address: 02
           max packet size: 0040
          polling interval: 00

Kernel driver attached for interface 0: (not supported)

Claiming interface 0...

Reading string descriptors:
   String (0x01): "FTDI"
   String (0x02): "FT232R USB UART"
   String (0x03): "A50285BI"

Reading OS string descriptor: no descriptor

Reading interface association descriptors (IADs) for first configuration:
    nb IADs: 0

Releasing interface 0...
Closing device...

[mcuee]

Same results with libusb-win32 1.2.6.0 device filter driver. This PR does help.

PS C:\work\libusb\libusb> .\examples\xusb.exe 0403:6001
Using libusb v1.0.26.11784

Opening device 0403:6001...

Reading device descriptor:
            length: 18
      device class: 0
               S/N: 3
           VID:PID: 0403:6001
         bcdDevice: 0600
   iMan:iProd:iSer: 1:2:3
          nb confs: 1

Reading BOS descriptor: libusb: warning [winusbx_submit_control_transfer] ControlTransfer failed: [31] A device attached to the system is not functioning.
libusb: error [libusb_get_bos_descriptor] failed to read BOS (-1)
no descriptor

Reading first configuration descriptor:
              total length: 32
         descriptor length: 9
             nb interfaces: 1
              interface[0]: id = 0
interface[0].altsetting[0]: num endpoints = 2
   Class.SubClass.Protocol: FF.FF.FF
       endpoint[0].address: 81
           max packet size: 0040
          polling interval: 00
       endpoint[1].address: 02
           max packet size: 0040
          polling interval: 00

Kernel driver attached for interface 0: (not supported)

Claiming interface 0...

Reading string descriptors:
   String (0x01): "FTDI"
   String (0x02): "US232R"
   String (0x03): "FTDCRPIE"

Reading OS string descriptor:libusb: warning [winusbx_submit_control_transfer] ControlTransfer failed: [31] A device attached to the system is not functioning.
 no descriptor

Reading interface association descriptors (IADs) for first configuration:
    nb IADs: 0

Releasing interface 0...
Closing device...

PS C:\work\libusb\libusb_pr1018> .\examples\xusb.exe 0403:6001
Using libusb v1.0.26.11784

Opening device 0403:6001...

Reading device descriptor:
            length: 18
      device class: 0
               S/N: 3
           VID:PID: 0403:6001
         bcdDevice: 0600
   iMan:iProd:iSer: 1:2:3
          nb confs: 1

Reading BOS descriptor: no descriptor

Reading first configuration descriptor:
              total length: 32
         descriptor length: 9
             nb interfaces: 1
              interface[0]: id = 0
interface[0].altsetting[0]: num endpoints = 2
   Class.SubClass.Protocol: FF.FF.FF
       endpoint[0].address: 81
           max packet size: 0040
          polling interval: 00
       endpoint[1].address: 02
           max packet size: 0040
          polling interval: 00

Kernel driver attached for interface 0: (not supported)

Claiming interface 0...

Reading string descriptors:
   String (0x01): "FTDI"
   String (0x02): "US232R"
   String (0x03): "FTDCRPIE"

Reading OS string descriptor: no descriptor

Reading interface association descriptors (IADs) for first configuration:
    nb IADs: 0

Releasing interface 0...
Closing device...

[tormodvolden]

(just rebased to latest master. EDIT: and actually pushed the rebase branch)

[tormodvolden]

Eventually the two commits should be merged, since the first doesn't make any real change without the second. They were originally split because the first one seems straightforward, but the second is more delicate.

[mcuee]

@tormodvolden

Not so sure why but I have problems building the PR under VS2022. git HEAD is okay,

PS C:\work\libusb\libusb_pr1018\msvc> msbuild -m -v:m -p:PlatformToolset=v143,Platform=x64,Configuration=Release .\libusb.sln
Microsoft (R) Build Engine version 17.1.0+ae57d105c for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

  getopt.vcxproj -> C:\work\libusb\libusb_pr1018\build\v143\x64\Release\getopt.lib
  core.c
  descriptor.c
  events_windows.c
  hotplug.c
  io.c
  strerror.c
  sync.c
  threads_windows.c
  windows_common.c
  windows_usbdk.c
  core.c
  windows_winusb.c
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220: the following warning is treated as an er
ror [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220:           result = transfer_priv->overlap
ped.Internal; [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220:
   ^ [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
  descriptor.c
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244: '=': conversion from 'ULONG_PTR' to 'DW
ORD', possible loss of data [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244:                 result = transfer_priv-
>overlapped.Internal; [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244:
           ^ [C:\work\libusb\libusb_pr1018\msvc\libusb_dll.vcxproj]
  events_windows.c
  hotplug.c
  io.c
  strerror.c
  sync.c
  threads_windows.c
  windows_common.c
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220: the following warning is treated as an er
ror [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220:           result = transfer_priv->overlap
ped.Internal; [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): error C2220:
   ^ [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244: '=': conversion from 'ULONG_PTR' to 'DW
ORD', possible loss of data [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244:                 result = transfer_priv-
>overlapped.Internal; [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
C:\work\libusb\libusb_pr1018\libusb\os\windows_common.c(805,37): warning C4244:
           ^ [C:\work\libusb\libusb_pr1018\msvc\libusb_static.vcxproj]
  windows_usbdk.c
  windows_winusb.c

[tormodvolden]

Not so sure why but I have problems building the PR under VS2022. git HEAD is ok

Thanks for the heads-up. It was a fair warning, fixed up now. It was also seen on Appveyor x64 builds.

[mcuee]

@sonatique

Just wondering if you can give this PR a review as well. Thanks.

[sonatique]

@mcuee : I carefully read the code and everything looks fine to me.

[mcuee]

@tormodvolden

I think this PR is good to go now.