Vhci ude rebase spog

[spog]

@cezanne - i hope you looked for this kind of rebas-ed pull-request:)

[cezanne]

@spog: Thanks. I have tested your work and confirmed that it worked very well. I felt speed-up for the patched version. Maybe spinlock?

[spog]

@cezanne - This is good to hear, but i am not sure about the speed, except that spinlocks for sure wait in the fastest possible way:)

[cezanne]

@spog : Can you describe a reproducible test case that current master branch fails without this PR ?

[spog]

@cezanne - One test case could be that just connecting and disconnecting the USB audio interface (FIIO - K3) via the usbip-win client using the vhci_ude driver, leaves the driver in a state that it can not be unloaded any more (and after quite some time the system reboots itself).

[cezanne]

@spog : I've just created a spog branch. I'll update some more commits on that branch including a PagedPool case.

[spog]

@cezanne - i rebuilt the 'spog' branch and successfully loaded, connected, disconnected and unloaded the driver for both storage device and audio interface. And regarding operation of devices, the storage device operated OK (for test i only modified a file on the USB key) and the audio interface shows similar behavior as before (also unloading of the driver is not possible after i.e. volume change and if i manage to interrupt the connection).

[cezanne]

@spog : Please retry with 29798a28aa126f88376400df484d3b9a22f44a01.

[spog]

@cezanne - i retried the https://github.com/cezanne/usbip-win/commit/29798a28aa126f88376400df484d3b9a22f44a01 and it is ok for the storage device but the audio interface usage is worse. The driver fails to unload immediately after disconnect without any other action upon the usb device (i.e. volume setting). After a while a system reset ocurred.

Here is the trace log: vhci_ude-fiio.txt

[spog]

@cezanne - The following patch on top of https://github.com/cezanne/usbip-win/commit/29798a28aa126f88376400df484d3b9a22f44a01 helped to unload the driver upon audio interface disconnection (even if volume has been changed before disconnection). I am not sure, if this is the best solution however:

diff --git a/driver/vhci_ude/vhci_ep.c b/driver/vhci_ude/vhci_ep.c
index d28f1a3..f7a857e 100644
--- a/driver/vhci_ude/vhci_ep.c
+++ b/driver/vhci_ude/vhci_ep.c
@@ -33,6 +33,9 @@ ep_purge(_In_ UDECXUSBENDPOINT ude_ep)
        /* WdfIoQueuePurgeSynchronously would suffer from blocking */
        WdfIoQueuePurge(ep->queue, purge_complete, NULL);

+       if (ep == ep->vusb->ep_default)
+               ep->vusb->ep_default = NULL;
+
        TRD(VUSB, "Leave");
 }

@@ -216,7 +219,7 @@ ep_configure(_In_ UDECXUSBDEVICE udev, _In_ WDFREQUEST req, _In_ PUDECX_ENDPOINT
        TRD(VUSB, "Enter: %!epconf!", params->ConfigureType);

        release_ep(params);
-       if ((params->ConfigureType == UdecxEndpointsConfigureTypeEndpointsReleasedOnly) || (vusb->invalid == TRUE)) {
+       if ((params->ConfigureType == UdecxEndpointsConfigureTypeEndpointsReleasedOnly) || (vusb->invalid == TRUE) || (vusb->ep_default == NULL)) {
                WdfRequestComplete(req, status);
                TRD(VUSB, "Leave: %!STATUS!", status);
                return;
@@ -227,14 +230,23 @@ ep_configure(_In_ UDECXUSBDEVICE udev, _In_ WDFREQUEST req, _In_ PUDECX_ENDPOINT
                /* FIXME: UDE framework seems to not call SET CONFIGURATION if a USB has multiple interfaces.
                 * This enforces the device to be set with the first configuration.
                 */
-               status = submit_req_select(vusb->ep_default, req, 1, vusb->default_conf_value, 0, 0);
-               TRD(VUSB, "trying to SET CONFIGURATION: %u", (ULONG)vusb->default_conf_value);
-               break;
+               if (vusb->ep_default != NULL) {
+                       status = submit_req_select(vusb->ep_default, req, 1, vusb->default_conf_value, 0, 0);
+                       TRD(VUSB, "trying to SET CONFIGURATION: %u", (ULONG)vusb->default_conf_value);
+               } else
+                       status = STATUS_UNSUCCESSFUL;
+                       break;
        case UdecxEndpointsConfigureTypeDeviceConfigurationChange:
-               status = submit_req_select(vusb->ep_default, req, 1, params->NewConfigurationValue, 0, 0);
-               break;
+               if (vusb->ep_default != NULL)
+                       status = submit_req_select(vusb->ep_default, req, 1, params->NewConfigurationValue, 0, 0);
+               else
+                       status = STATUS_UNSUCCESSFUL;
+       break;
        case UdecxEndpointsConfigureTypeInterfaceSettingChange:
-               status = set_intf_for_ep(vusb, req, params);
+               if (vusb->ep_default != NULL)
+                       status = set_intf_for_ep(vusb, req, params);
+               else
+                       status = STATUS_UNSUCCESSFUL;
                break;
        default:
                TRE(VUSB, "unhandled configure type: %!epconf!", params->ConfigureType);

[spog]

@cezanne - Please forget about the previous patch. The following patch on top of 29798a2 helped to unload the driver upon audio interface disconnection (even if volume has been changed before disconnection). I am not sure, if this kind of change is the best solution however:

diff --git a/driver/vhci_ude/vhci_ep.c b/driver/vhci_ude/vhci_ep.c
index d28f1a3..005bc26 100644
--- a/driver/vhci_ude/vhci_ep.c
+++ b/driver/vhci_ude/vhci_ep.c
@@ -167,6 +167,8 @@ release_ep(PUDECX_ENDPOINTS_CONFIGURE_PARAMS params)
        for (ULONG i = 0; i < params->ReleasedEndpointsCount; i++) {
                pctx_ep_t       ep = TO_EP(params->ReleasedEndpoints[i]);
                WdfIoQueuePurgeSynchronously(ep->queue);
+               if (ep == ep->vusb->ep_default)
+                       ep->vusb->ep_default = NULL;
                TRD(VUSB, "Released ep->addr=0x%x!", ep->addr);
        }
 }
@@ -216,7 +218,7 @@ ep_configure(_In_ UDECXUSBDEVICE udev, _In_ WDFREQUEST req, _In_ PUDECX_ENDPOINT
        TRD(VUSB, "Enter: %!epconf!", params->ConfigureType);

        release_ep(params);
-       if ((params->ConfigureType == UdecxEndpointsConfigureTypeEndpointsReleasedOnly) || (vusb->invalid == TRUE)) {
+       if ((params->ConfigureType == UdecxEndpointsConfigureTypeEndpointsReleasedOnly) || (vusb->invalid == TRUE) || (vusb->ep_default == NULL)) {
                WdfRequestComplete(req, status);
                TRD(VUSB, "Leave: %!STATUS!", status);
                return;
@@ -227,14 +229,23 @@ ep_configure(_In_ UDECXUSBDEVICE udev, _In_ WDFREQUEST req, _In_ PUDECX_ENDPOINT
                /* FIXME: UDE framework seems to not call SET CONFIGURATION if a USB has multiple interfaces.
                 * This enforces the device to be set with the first configuration.
                 */
-               status = submit_req_select(vusb->ep_default, req, 1, vusb->default_conf_value, 0, 0);
-               TRD(VUSB, "trying to SET CONFIGURATION: %u", (ULONG)vusb->default_conf_value);
-               break;
+               if (vusb->ep_default != NULL) {
+                       status = submit_req_select(vusb->ep_default, req, 1, vusb->default_conf_value, 0, 0);
+                       TRD(VUSB, "trying to SET CONFIGURATION: %u", (ULONG)vusb->default_conf_value);
+               } else
+                       status = STATUS_UNSUCCESSFUL;
+                       break;
        case UdecxEndpointsConfigureTypeDeviceConfigurationChange:
-               status = submit_req_select(vusb->ep_default, req, 1, params->NewConfigurationValue, 0, 0);
-               break;
+               if (vusb->ep_default != NULL)
+                       status = submit_req_select(vusb->ep_default, req, 1, params->NewConfigurationValue, 0, 0);
+               else
+                       status = STATUS_UNSUCCESSFUL;
+       break;
        case UdecxEndpointsConfigureTypeInterfaceSettingChange:
-               status = set_intf_for_ep(vusb, req, params);
+               if (vusb->ep_default != NULL)
+                       status = set_intf_for_ep(vusb, req, params);
+               else
+                       status = STATUS_UNSUCCESSFUL;
                break;
        default:
                TRE(VUSB, "unhandled configure type: %!epconf!", params->ConfigureType);

[cezanne]

@spog : I haven't tried your recent patch. Instead, b1d980a0102adef243ee90235124302d85d93a38 is newly added on the spog branch. This commit resolves my web cam freeze when detaching. I have checked that a vusb cleanup is called but it takes 15~30secs.

I'll try your patch also. But it was not cleanly patched. Maybe there's some differences between our codes. Of course, I can manually do patch.

[spog]

@cezanne - i have checked that the spog branch from my fork is the same as yours. Here is the log of disconnection/unloading my audio interface with my patch enabled. i'd try your latest commit but after viewing the code i am not sure. Namely, i found out that requests for removing endpoints often come implicitly via another request like setting the interface or configuration.

vhci_ude-fiio_test.txt

[spog]

@cezanne - And here is another trace, where i also change volume (which leads to error) before the disconnect and the module is successfully unloaded afterwards.

vhci_ude-fiio_test1.txt

[spog]

@cezanne - i sketched a high level view of the project used as a client (picture attached). Would you please comment, if there is something fundamentally wrong about the picture or whatever you would like say about it.

[spog]

@cezanne - Here is also the trace of your latest commit with unsuccessful disconnection/unload.

vhci_ude-fiio_b1d980a.txt

[cezanne]

@spog : Your sketch looks great even though I have not looked closely. Such an architecture diagram for usbip-win would be good for gihub wiki. I'll check your sketch later.

Anyway, your patch is pretty better than mine. At least, it helps to detach faster. I got a hint from your patch and will upload another commit.

[cezanne]

@spog : When I retried, your patch did not work for disconnecting or detaching an actively running webcam. I uploaded a temporary branch for your verification.

[spog]

@cezanne: i've tested the temporary branch and it behaves the same as my spog_test branch. It is interesting (for both branches), that the first test failes and then after reboot it behaves as expected (see log): vhci_ude-fiio_temp.txt

[cezanne]

@spog : My logitech webcam c170 works good with the spog branch. It was not properly displayed over usbip-win with the current master. So I will merge the spog branch into the master after some more tests.

[cezanne]

@spog : Thanks for your work. Any unresolved problem or something will be discussed in a new issue or PR.