Changeset 4db49344 in mainline for uspace/drv/bus/usb/xhci

Timestamp:

2018-01-23T21:52:28Z (8 years ago)

Author:

Ondřej Hlavatý <aearsis@…>

Branches:

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

Children:

3dd80f8

Parents:

a6afb4c

git-author:

Ondřej Hlavatý <aearsis@…> (2018-01-23 20:49:35)

git-committer:

Ondřej Hlavatý <aearsis@…> (2018-01-23 21:52:28)

Message:

usb: fix wrong design of transfer aborting

Apparently, we didn't do a good job in thinking through the problem.
In older HCs, it was done just wrong - the UHCI implementation commited
a batch that could have been already aborted, and EHCI+OHCI might miss
an interrupt because they commited the batch sooner than they added it
to their checked list.

This commit takes everything from the other end, which is probably the
only right one. Instead of an endpoint having an extra mutex, it
inherits a mutex from the outside. It never locks it though, it just
checks if the mutex is locked and uses it for waiting on condition
variables.

This mutex is supposed to be the one which the HC driver uses for
locking its structures in scheduling. This way, we avoid the ABBA
deadlock completely, while preserving the synchronization on an
endpoint.

The good thing is that this implementation is much easier to extend with
multiple active batches per endpoint.

Location:

uspace/drv/bus/usb/xhci

Files:

• device.c (modified) (1 diff)
• endpoint.c (modified) (4 diffs)
• endpoint.h (modified) (1 diff)
• transfers.c (modified) (5 diffs)

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

@@ -81 +81 @@
         usb_log_debug("Obtained slot ID: %u.", dev->slot_id);
 
-        /* Create and configure control endpoint. */
-        endpoint_t *ep0_base = xhci_endpoint_create(&dev->base, &ep0_initial_desc);
-        if (!ep0_base)
+        endpoint_t *ep0_base;
+        if ((err = bus_endpoint_add(&dev->base, &ep0_initial_desc, &ep0_base)))
                 goto err_slot;
-
-        /* Bus reference */
-        endpoint_add_ref(ep0_base);
-        dev->base.endpoints[0] = ep0_base;
 
         usb_log_debug("Looking up new device initial MPS: %s",

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

@@ -68 +68 @@
         endpoint_init(ep, dev, desc);
 
+        fibril_mutex_initialize(&xhci_ep->guard);
+
         xhci_ep->max_burst = desc->companion.max_burst + 1;
 
@@ -177 +179 @@
         xhci_endpoint_t *ep = xhci_endpoint_get(ep_base);
 
-        if ((err = hc_add_endpoint(ep)))
+        if (ep_base->endpoint != 0 && (err = hc_add_endpoint(ep)))
                 return err;
 
+        endpoint_set_online(ep_base, &ep->guard);
         return EOK;
 }
@@ -186 +189 @@
  * Abort a transfer on an endpoint.
  */
-static int endpoint_abort(endpoint_t *ep)
+static void endpoint_abort(endpoint_t *ep)
 {
         xhci_device_t *dev = xhci_device_get(ep->device);
         xhci_endpoint_t *xhci_ep = xhci_endpoint_get(ep);
 
-        usb_transfer_batch_t *batch = NULL;
-        fibril_mutex_lock(&ep->guard);
-        if (ep->active_batch) {
-                if (dev->slot_id) {
-                        const int err = hc_stop_endpoint(xhci_ep);
-                        if (err) {
-                                usb_log_warning("Failed to stop endpoint %u of device "
-                                    XHCI_DEV_FMT ": %s", ep->endpoint, XHCI_DEV_ARGS(*dev),
-                                    str_error(err));
-                        }
-
-                        endpoint_wait_timeout_locked(ep, 2000);
-                }
-
-                batch = ep->active_batch;
-                if (batch) {
-                        endpoint_deactivate_locked(ep);
-                }
-        }
-        fibril_mutex_unlock(&ep->guard);
-
-        if (batch) {
-                batch->error = EINTR;
-                batch->transferred_size = 0;
-                usb_transfer_batch_finish(batch);
-        }
-        return EOK;
+        /* This function can only abort endpoints without streams. */
+        assert(xhci_ep->primary_stream_data_array == NULL);
+
+        fibril_mutex_lock(&xhci_ep->guard);
+
+        endpoint_set_offline_locked(ep);
+
+        if (!ep->active_batch) {
+                fibril_mutex_unlock(&xhci_ep->guard);
+                return;
+        }
+
+        /* First, offer the batch a short chance to be finished. */
+        endpoint_wait_timeout_locked(ep, 10000);
+
+        if (!ep->active_batch) {
+                fibril_mutex_unlock(&xhci_ep->guard);
+                return;
+        }
+
+        usb_transfer_batch_t * const batch = ep->active_batch;
+
+        const int err = hc_stop_endpoint(xhci_ep);
+        if (err) {
+                usb_log_error("Failed to stop endpoint %u of device "
+                    XHCI_DEV_FMT ": %s", ep->endpoint, XHCI_DEV_ARGS(*dev),
+                    str_error(err));
+        }
+
+        fibril_mutex_unlock(&xhci_ep->guard);
+
+        batch->error = EINTR;
+        batch->transferred_size = 0;
+        usb_transfer_batch_finish(batch);
+        return;
 }
 
@@ -235 +246 @@
 
         /* If device slot is still available, drop the endpoint. */
-        if (dev->slot_id) {
+        if (ep_base->endpoint != 0 && dev->slot_id) {
 
                 if ((err = hc_drop_endpoint(ep))) {

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

@@ -70 +70 @@
         endpoint_t base;        /**< Inheritance. Keep this first. */
 
+        /** Guarding scheduling of this endpoint. */
+        fibril_mutex_t guard;
+
         /** Main transfer ring (unused if streams are enabled) */
         xhci_trb_ring_t ring;

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

@@ -300 +300 @@
 
         if (TRB_EVENT_DATA(*trb)) {
-                assert(ep->base.transfer_type != USB_TRANSFER_ISOCHRONOUS);
+                /* We schedule those only when streams are involved */
+                assert(ep->primary_stream_ctx_array != NULL);
+
                 /* We are received transfer pointer instead - work with that */
                 transfer = (xhci_transfer_t *) addr;
@@ -306 +308 @@
                     transfer->interrupt_trb_phys);
                 batch = &transfer->batch;
-
-                fibril_mutex_lock(&ep->base.guard);
-                endpoint_deactivate_locked(&ep->base);
-                fibril_mutex_unlock(&ep->base.guard);
         }
         else {
@@ -321 +319 @@
                 }
 
-                fibril_mutex_lock(&ep->base.guard);
+                fibril_mutex_lock(&ep->guard);
                 batch = ep->base.active_batch;
+                endpoint_deactivate_locked(&ep->base);
+                fibril_mutex_unlock(&ep->guard);
+
                 if (!batch) {
-                        fibril_mutex_unlock(&ep->base.guard);
                         /* Dropping temporary reference */
                         endpoint_del_ref(&ep->base);
@@ -331 +331 @@
 
                 transfer = xhci_transfer_from_batch(batch);
-
-                endpoint_deactivate_locked(&ep->base);
-                fibril_mutex_unlock(&ep->base.guard);
         }
 
@@ -482 +479 @@
 
 
-        fibril_mutex_lock(&ep->guard);
-        endpoint_activate_locked(ep, batch);
-        const int err = transfer_handlers[batch->ep->transfer_type](hc, transfer);
-
-        if (err) {
+        int err;
+        fibril_mutex_lock(&xhci_ep->guard);
+
+        if ((err = endpoint_activate_locked(ep, batch))) {
+                fibril_mutex_unlock(&xhci_ep->guard);
+                return err;
+        }
+
+        if ((err = transfer_handlers[batch->ep->transfer_type](hc, transfer))) {
                 endpoint_deactivate_locked(ep);
-                fibril_mutex_unlock(&ep->guard);
+                fibril_mutex_unlock(&xhci_ep->guard);
                 return err;
         }
 
         hc_ring_ep_doorbell(xhci_ep, batch->target.stream);
-        fibril_mutex_unlock(&ep->guard);
+        fibril_mutex_unlock(&xhci_ep->guard);
         return EOK;
 }