Changeset 31cca4f3 in mainline

Timestamp:

2017-10-23T18:37:53Z (7 years ago)

Author:

Petr Manek <petr.manek@…>

Branches:

lfn, master, serial, ticket/834-toolchain-update, topic/msim-upgrade, topic/simplify-dev-export

Children:

e160bfe8

Parents:

81487c4a

Message:

Corrected deadlock in IPC between USB remote interface and usbdev driver. Extracted creation of input context. Now issuing drop endpoint command when the endpoint is removed.

Location:

uspace

Files:

• drv/bus/usb/xhci/bus.c (modified) (1 diff)
• drv/bus/usb/xhci/endpoint.c (modified) (7 diffs)
• drv/bus/usb/xhci/rh.c (modified) (2 diffs)
• lib/drv/generic/remote_usb.c (modified) (1 diff)

uspace/drv/bus/usb/xhci/bus.c

@@ -139 +139 @@
         free32(xhci_dev->dev_ctx);
         hc->dcbaa[xhci_dev->slot_id] = 0;
-        return ENOTSUP;
+        return EOK;
 }
 

uspace/drv/bus/usb/xhci/endpoint.c

@@ -81 +81 @@
 int xhci_endpoint_alloc_transfer_ds(xhci_endpoint_t *xhci_ep)
 {
-        int err;
-
         if (endpoint_uses_streams(xhci_ep)) {
                 /* Set up primary stream context array if needed. */
                 const size_t size = primary_stream_ctx_array_size(xhci_ep);
+                usb_log_debug2("Allocating primary stream context array of size %lu for endpoint %d:%d.",
+                    size, xhci_ep->base.target.address, xhci_ep->base.target.endpoint);
 
                 xhci_ep->primary_stream_ctx_array = malloc32(size * sizeof(xhci_stream_ctx_t));
@@ -94 +94 @@
                 memset(xhci_ep->primary_stream_ctx_array, 0, size * sizeof(xhci_stream_ctx_t));
         } else {
+                usb_log_debug2("Allocating main transfer ring for endpoint %d:%d.",
+                    xhci_ep->base.target.address, xhci_ep->base.target.endpoint);
+
                 xhci_ep->primary_stream_ctx_array = NULL;
+
+                int err;
                 if ((err = xhci_trb_ring_init(&xhci_ep->ring))) {
                         return err;
@@ -105 +110 @@
 int xhci_endpoint_free_transfer_ds(xhci_endpoint_t *xhci_ep)
 {
-        int err;
-
         if (endpoint_uses_streams(xhci_ep)) {
+                usb_log_debug2("Freeing primary stream context array for endpoint %d:%d.",
+                    xhci_ep->base.target.address, xhci_ep->base.target.endpoint);
+
                 // TODO: What about secondaries?
                 free32(xhci_ep->primary_stream_ctx_array);
         } else {
+                usb_log_debug2("Freeing main transfer ring for endpoint %d:%d.",
+                    xhci_ep->base.target.address, xhci_ep->base.target.endpoint);
+
+                int err;
                 if ((err = xhci_trb_ring_fini(&xhci_ep->ring))) {
                         return err;
@@ -232 +242 @@
 };
 
-int xhci_device_add_endpoint(xhci_device_t *dev, xhci_endpoint_t *ep)
-{
-        assert(dev);
-        assert(ep);
-
-        /* Offline devices don't create new endpoints other than EP0. */
-        if (!dev->online) {
-                return EAGAIN;
-        }
-
-        int err = ENOMEM;
-        const usb_endpoint_t ep_num = ep->base.target.endpoint;
-
-        assert(&dev->base == ep->base.device);
-        assert(dev->base.address == ep->base.target.address);
-        assert(!dev->endpoints[ep_num]);
-
-        dev->endpoints[ep_num] = ep;
-        ++dev->active_endpoint_count;
-
-        if (ep_num == 0)
-                /* EP 0 is initialized while setting up the device,
-                 * so we must not issue the command now. */
-                return EOK;
-
-        // FIXME: Set these from usb_superspeed_endpoint_companion_descriptor_t:
-        ep->max_streams = 0;
-        ep->max_burst = 0;
-        ep->mult = 0;
-
-        xhci_endpoint_alloc_transfer_ds(ep);
-
-        // Prepare input context.
+static int create_valid_input_ctx(xhci_input_ctx_t **out_ictx)
+{
         xhci_input_ctx_t *ictx = malloc32(sizeof(xhci_input_ctx_t));
-        if (!ictx)
-                goto err;
+        if (!ictx) {
+                return ENOMEM;
+        }
 
         memset(ictx, 0, sizeof(xhci_input_ctx_t));
@@ -277 +257 @@
         XHCI_INPUT_CTRL_CTX_ADD_SET(ictx->ctrl_ctx, 0);
 
+        if (out_ictx) {
+                *out_ictx = ictx;
+        }
+
+        return EOK;
+}
+
+int xhci_device_add_endpoint(xhci_device_t *dev, xhci_endpoint_t *ep)
+{
+        assert(dev);
+        assert(ep);
+
+        /* Offline devices don't create new endpoints other than EP0. */
+        if (!dev->online) {
+                return EAGAIN;
+        }
+
+        int err = ENOMEM;
+        const usb_endpoint_t ep_num = ep->base.target.endpoint;
+
+        assert(&dev->base == ep->base.device);
+        assert(dev->base.address == ep->base.target.address);
+        assert(!dev->endpoints[ep_num]);
+
+        dev->endpoints[ep_num] = ep;
+        ++dev->active_endpoint_count;
+
+        if (ep_num == 0) {
+                /* EP 0 is initialized while setting up the device,
+                 * so we must not issue the command now. */
+                return EOK;
+        }
+
+        // FIXME: Set these from usb_superspeed_endpoint_companion_descriptor_t:
+        ep->max_streams = 0;
+        ep->max_burst = 0;
+        ep->mult = 0;
+
+        /* Set up TRB ring / PSA. */
+        if ((err = xhci_endpoint_alloc_transfer_ds(ep))) {
+                goto err;
+        }
+
+        /* Issue configure endpoint command (sec 4.3.5). */
+        xhci_input_ctx_t *ictx;
+        if ((err = create_valid_input_ctx(&ictx))) {
+                goto err_ds;
+        }
+
         const unsigned ep_idx = xhci_endpoint_index(ep);
         XHCI_INPUT_CTRL_CTX_ADD_SET(ictx->ctrl_ctx, ep_idx + 1); /* Preceded by slot ctx */
-
-        xhci_ep_ctx_t *ep_ctx = &ictx->endpoint_ctx[ep_idx];
-        setup_ep_ctx_helpers[ep->base.transfer_type](ep, ep_ctx);
-
-        // Issue configure endpoint command (sec 4.3.5).
+        setup_ep_ctx_helpers[ep->base.transfer_type](ep, &ictx->endpoint_ctx[ep_idx]);
+
         xhci_cmd_t cmd;
         xhci_cmd_init(&cmd);
 
         cmd.slot_id = dev->slot_id;
-        xhci_send_configure_endpoint_command(dev->hc, &cmd, ictx);
-        if ((err = xhci_cmd_wait(&cmd, XHCI_DEFAULT_TIMEOUT)) != EOK)
+
+        if ((err = xhci_send_configure_endpoint_command(dev->hc, &cmd, ictx))) {
                 goto err_ictx;
+        }
+
+        if ((err = xhci_cmd_wait(&cmd, XHCI_DEFAULT_TIMEOUT))) {
+                goto err_ictx;
+        }
 
         xhci_cmd_fini(&cmd);
@@ -299 +330 @@
 err_ictx:
         free32(ictx);
+err_ds:
+        xhci_endpoint_free_transfer_ds(ep);
 err:
         dev->endpoints[ep_num] = NULL;
@@ -311 +344 @@
         assert(dev->endpoints[ep->base.target.endpoint]);
 
-        // TODO: Issue configure endpoint command to drop this endpoint.
-
-        // FIXME: Ignoring return code.
-        xhci_endpoint_free_transfer_ds(ep);
+        int err = ENOMEM;
+        const usb_endpoint_t ep_num = ep->base.target.endpoint;
 
         dev->endpoints[ep->base.target.endpoint] = NULL;
         --dev->active_endpoint_count;
-        return EOK;
-}
-
-xhci_endpoint_t * xhci_device_get_endpoint(xhci_device_t *dev, usb_endpoint_t ep)
-{
-        return dev->endpoints[ep];
-}
-
-int xhci_device_configure(xhci_device_t *dev, xhci_hc_t *hc)
-{
-        int err;
-
-        // Prepare input context.
-        xhci_input_ctx_t *ictx = malloc(sizeof(xhci_input_ctx_t));
-        if (!ictx) {
-                return ENOMEM;
-        }
-
-        memset(ictx, 0, sizeof(xhci_input_ctx_t));
-
-        // Quoting sec. 4.6.6: A1, D0, D1 are down, A0 is up.
-        XHCI_INPUT_CTRL_CTX_ADD_CLEAR(ictx->ctrl_ctx, 1);
-        XHCI_INPUT_CTRL_CTX_DROP_CLEAR(ictx->ctrl_ctx, 0);
-        XHCI_INPUT_CTRL_CTX_DROP_CLEAR(ictx->ctrl_ctx, 1);
-        XHCI_INPUT_CTRL_CTX_ADD_SET(ictx->ctrl_ctx, 0);
-
-        // TODO: Set slot context and other flags. (probably forgot a lot of 'em)
-
-        // Issue configure endpoint command (sec 4.3.5).
+
+        if (ep_num == 0) {
+                /* EP 0 is finalized while releasing the device,
+                 * so we must not issue the command now. */
+                return EOK;
+        }
+
+        /* Issue configure endpoint command to drop this endpoint. */
+        xhci_input_ctx_t *ictx;
+        if ((err = create_valid_input_ctx(&ictx))) {
+                goto err;
+        }
+
+        const unsigned ep_idx = xhci_endpoint_index(ep);
+        XHCI_INPUT_CTRL_CTX_DROP_SET(ictx->ctrl_ctx, ep_idx + 1); /* Preceded by slot ctx */
+
         xhci_cmd_t cmd;
         xhci_cmd_init(&cmd);
 
         cmd.slot_id = dev->slot_id;
-        xhci_send_configure_endpoint_command(hc, &cmd, ictx);
-        if ((err = xhci_cmd_wait(&cmd, XHCI_DEFAULT_TIMEOUT)) != EOK)
+
+        if ((err = xhci_send_configure_endpoint_command(dev->hc, &cmd, ictx))) {
+                goto err_ictx;
+        }
+
+        if ((err = xhci_cmd_wait(&cmd, XHCI_DEFAULT_TIMEOUT))) {
+                goto err_ictx;
+        }
+
+        xhci_cmd_fini(&cmd);
+
+        /* Tear down TRB ring / PSA. */
+        /* FIXME: For some reason, this causes crash at xhci_trb_ring_fini.
+        if ((err = xhci_endpoint_free_transfer_ds(ep))) {
                 goto err_cmd;
+        }
+        */
+
+        free32(ictx);
+        return EOK;
+
+err_ictx:
+        free32(ictx);
+err:
+        dev->endpoints[ep_num] = ep;
+        ++dev->active_endpoint_count;
+        return err;
+}
+
+xhci_endpoint_t *xhci_device_get_endpoint(xhci_device_t *dev, usb_endpoint_t ep)
+{
+        return dev->endpoints[ep];
+}
+
+int xhci_device_configure(xhci_device_t *dev, xhci_hc_t *hc)
+{
+        int err;
+
+        /* Issue configure endpoint command (sec 4.3.5). */
+        xhci_input_ctx_t *ictx;
+        if ((err = create_valid_input_ctx(&ictx))) {
+                goto err;
+        }
+
+        // TODO: Set slot context and other flags. (probably forgot a lot of 'em)
+
+        xhci_cmd_t cmd;
+        xhci_cmd_init(&cmd);
+
+        cmd.slot_id = dev->slot_id;
+
+        if ((err = xhci_send_configure_endpoint_command(hc, &cmd, ictx))) {
+                goto err_cmd;
+        }
+
+        if ((err = xhci_cmd_wait(&cmd, XHCI_DEFAULT_TIMEOUT))) {
+                goto err_cmd;
+        }
 
         xhci_cmd_fini(&cmd);
+
+        free32(ictx);
         return EOK;
 
 err_cmd:
-        free(ictx);
+        free32(ictx);
+err:
         return err;
 }

uspace/drv/bus/usb/xhci/rh.c

@@ -315 +315 @@
         }
 
+        /* TODO: Figure out how to handle errors here. So far, they are reported and skipped. */
+
         /* Make DDF (and all drivers) forget about the device. */
         if ((err = ddf_fun_unbind(dev->base.fun))) {
@@ -321 +323 @@
         }
 
-        /* FIXME:
-         * A deadlock happens on the previous line. For some reason, the HID driver
-         * does not fully release its DDF function (tracked it down to release_endpoint
-         * in usb_remote.c so far).
-         *
-         * For that reason as well, the following 3 lines are untested.
-         */
-
-        xhci_bus_remove_device(&rh->hc->bus, rh->hc, &dev->base);
-        hc_disable_slot(rh->hc, dev->slot_id);
+        // TODO: Remove EP0.
+        // TODO: Deconfigure device.
+
+        /* Remove device from XHCI bus. */
+        if ((err = xhci_bus_remove_device(&rh->hc->bus, rh->hc, &dev->base))) {
+                usb_log_warning("Failed to remove device '%s' from XHCI bus: %s",
+                    ddf_fun_get_name(dev->base.fun), str_error(err));
+        }
+
+        /* Disable device slot. */
+        if ((err = hc_disable_slot(rh->hc, dev->slot_id))) {
+                usb_log_warning("Failed to disable slot for device '%s': %s",
+                    ddf_fun_get_name(dev->base.fun), str_error(err));
+        }
+
+        /* Destroy DDF device. */
         hcd_ddf_device_destroy(&dev->base);
 

uspace/lib/drv/generic/remote_usb.c

@@ -510 +510 @@
 
         free(buffer);
-        if (rc != EOK) {
-                async_answer_0(callid, rc);
-        }
+        async_answer_0(callid, rc);
 }