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

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/uhci

Files:

• hc.c (modified) (11 diffs)
• hc.h (modified) (1 diff)
• transfer_list.c (modified) (4 diffs)
• transfer_list.h (modified) (1 diff)

uspace/drv/bus/usb/uhci/hc.c

@@ -97 +97 @@
 
 static void hc_init_hw(const hc_t *instance);
-static int hc_init_mem_structures(hc_t *instance, hc_device_t *);
+static int hc_init_mem_structures(hc_t *instance);
 static int hc_init_transfer_lists(hc_t *instance);
 
@@ -164 +164 @@
         /* Lower 2 bits are transaction error and transaction complete */
         if (status & (UHCI_STATUS_INTERRUPT | UHCI_STATUS_ERROR_INTERRUPT)) {
-                LIST_INITIALIZE(done);
-                transfer_list_remove_finished(
-                    &instance->transfers_interrupt, &done);
-                transfer_list_remove_finished(
-                    &instance->transfers_control_slow, &done);
-                transfer_list_remove_finished(
-                    &instance->transfers_control_full, &done);
-                transfer_list_remove_finished(
-                    &instance->transfers_bulk_full, &done);
-
-                list_foreach_safe(done, current, next) {
-                        list_remove(current);
-                        uhci_transfer_batch_t *batch =
-                            uhci_transfer_batch_from_link(current);
-                        usb_transfer_batch_finish(&batch->base);
-                }
-        }
+                transfer_list_check_finished(&instance->transfers_interrupt);
+                transfer_list_check_finished(&instance->transfers_control_slow);
+                transfer_list_check_finished(&instance->transfers_control_full);
+                transfer_list_check_finished(&instance->transfers_bulk_full);
+        }
+
         /* Resume interrupts are not supported */
         if (status & UHCI_STATUS_RESUME) {
@@ -239 +228 @@
             hw_res->io_ranges.ranges[0].size);
 
-        ret = hc_init_mem_structures(instance, hcd);
+        ret = hc_init_mem_structures(instance);
         if (ret != EOK) {
                 usb_log_error("Failed to init UHCI memory structures: %s.",
@@ -328 +317 @@
 }
 
+static int endpoint_register(endpoint_t *ep)
+{
+        hc_t * const hc = bus_to_hc(endpoint_get_bus(ep));
+
+        const int err = usb2_bus_ops.endpoint_register(ep);
+        if (err)
+                return err;
+
+        transfer_list_t *list = hc->transfers[ep->device->speed][ep->transfer_type];
+        if (!list)
+                /*
+                 * We don't support this combination (e.g. isochronous). Do not
+                 * fail early, because that would block any device with these
+                 * endpoints from connecting. Instead, make sure these transfers
+                 * are denied soon enough with ENOTSUP not to fail on asserts.
+                 */
+                return EOK;
+
+        endpoint_set_online(ep, &list->guard);
+        return EOK;
+}
+
 static void endpoint_unregister(endpoint_t *ep)
 {
         hc_t * const hc = bus_to_hc(endpoint_get_bus(ep));
         usb2_bus_ops.endpoint_unregister(ep);
-
-        uhci_transfer_batch_t *batch = NULL;
 
         // Check for the roothub, as it does not schedule into lists
@@ -344 +353 @@
 
         transfer_list_t *list = hc->transfers[ep->device->speed][ep->transfer_type];
-
         if (!list)
                 /*
@@ -352 +360 @@
                 return;
 
-        // To avoid ABBA deadlock, we need to take the list first
         fibril_mutex_lock(&list->guard);
-        fibril_mutex_lock(&ep->guard);
-        if (ep->active_batch) {
-                batch = uhci_transfer_batch_get(ep->active_batch);
-                endpoint_deactivate_locked(ep);
-                transfer_list_remove_batch(list, batch);
-        }
-        fibril_mutex_unlock(&ep->guard);
+
+        endpoint_set_offline_locked(ep);
+        /* From now on, no other transfer will be scheduled. */
+
+        if (!ep->active_batch) {
+                fibril_mutex_unlock(&list->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(&list->guard);
+                return;
+        }
+
+        uhci_transfer_batch_t * const batch =
+                uhci_transfer_batch_get(ep->active_batch);
+
+        /* Remove the batch from the schedule to stop it from being finished. */
+        endpoint_deactivate_locked(ep);
+        transfer_list_remove_batch(list, batch);
+
         fibril_mutex_unlock(&list->guard);
 
-        if (batch) {
-                // The HW could have been looking at the batch.
-                // Better wait two frames before we release the buffers.
-                async_usleep(2000);
-                batch->base.error = EINTR;
-                batch->base.transferred_size = 0;
-                usb_transfer_batch_finish(&batch->base);
-        }
+        /*
+         * We removed the batch from software schedule only, it's still possible
+         * that HC has it in its caches. Better wait a while before we release
+         * the buffers.
+         */
+        async_usleep(20000);
+        batch->base.error = EINTR;
+        batch->base.transferred_size = 0;
+        usb_transfer_batch_finish(&batch->base);
 }
 
@@ -382 +407 @@
         .status = hc_status,
 
+        .endpoint_register = endpoint_register,
         .endpoint_unregister = endpoint_unregister,
         .endpoint_count_bw = bandwidth_count_usb11,
@@ -400 +426 @@
  *  - frame list page (needs to be one UHCI hw accessible 4K page)
  */
-int hc_init_mem_structures(hc_t *instance, hc_device_t *hcd)
+int hc_init_mem_structures(hc_t *instance)
 {
         assert(instance);
@@ -425 +451 @@
                 return ENOMEM;
         }
+        list_initialize(&instance->pending_endpoints);
         usb_log_debug("Initialized transfer lists.");
 
@@ -514 +541 @@
 }
 
-/** Schedule batch for execution.
+/**
+ * Schedule batch for execution.
  *
  * @param[in] instance UHCI structure to use.
  * @param[in] batch Transfer batch to schedule.
  * @return Error code
- *
- * Checks for bandwidth availability and appends the batch to the proper queue.
  */
 static int hc_schedule(usb_transfer_batch_t *batch)
@@ -531 +557 @@
                 return uhci_rh_schedule(&hc->rh, batch);
 
-
-        const int err = uhci_transfer_batch_prepare(uhci_batch);
-        if (err)
+        transfer_list_t * const list =
+            hc->transfers[ep->device->speed][ep->transfer_type];
+
+        if (!list)
+                return ENOTSUP;
+
+        int err;
+        if ((err = uhci_transfer_batch_prepare(uhci_batch)))
                 return err;
 
-        transfer_list_t *list = hc->transfers[ep->device->speed][ep->transfer_type];
-        assert(list);
-        transfer_list_add_batch(list, uhci_batch);
-
-        return EOK;
-}
-
-int hc_unschedule_batch(usb_transfer_batch_t *batch)
-{
-
-        return EOK;
+        return transfer_list_add_batch(list, uhci_batch);
 }
 

uspace/drv/bus/usb/uhci/hc.h

@@ -123 +123 @@
         transfer_list_t *transfers[2][4];
 
+        /**
+         * Guard for the pending list. Can be locked under EP guard, but not
+         * vice versa.
+         */
+        fibril_mutex_t guard;
+        /** List of endpoints with a transfer scheduled */
+        list_t pending_endpoints;
+
         /** Number of hw failures detected. */
         unsigned hw_failures;

uspace/drv/bus/usb/uhci/transfer_list.c

@@ -102 +102 @@
 }
 
-/** Add transfer batch to the list and queue.
- *
- * @param[in] instance List to use.
- * @param[in] batch Transfer batch to submit.
+/**
+ * Add transfer batch to the list and queue.
  *
  * The batch is added to the end of the list and queue.
- */
-void transfer_list_add_batch(
+ *
+ * @param[in] instance List to use.
+ * @param[in] batch Transfer batch to submit. After return, the batch must
+ *                  not be used further.
+ */
+int transfer_list_add_batch(
     transfer_list_t *instance, uhci_transfer_batch_t *uhci_batch)
 {
@@ -117 +119 @@
         endpoint_t *ep = uhci_batch->base.ep;
 
-        /* First, wait until the endpoint is free to use */
-        fibril_mutex_lock(&ep->guard);
-        endpoint_activate_locked(ep, &uhci_batch->base);
-        fibril_mutex_unlock(&ep->guard);
+        fibril_mutex_lock(&instance->guard);
+
+        const int err = endpoint_activate_locked(ep, &uhci_batch->base);
+        if (err) {
+                fibril_mutex_unlock(&instance->guard);
+                return err;
+        }
 
         usb_log_debug2("Batch %p adding to queue %s.",
             uhci_batch, instance->name);
-
-        fibril_mutex_lock(&instance->guard);
 
         /* Assume there is nothing scheduled */
@@ -155 +158 @@
             USB_TRANSFER_BATCH_ARGS(uhci_batch->base), instance->name);
         fibril_mutex_unlock(&instance->guard);
+        return EOK;
 }
 
@@ -171 +175 @@
  * @param[in] done list to fill
  */
-void transfer_list_remove_finished(transfer_list_t *instance, list_t *done)
-{
-        assert(instance);
-        assert(done);
+void transfer_list_check_finished(transfer_list_t *instance)
+{
+        assert(instance);
 
         fibril_mutex_lock(&instance->guard);
-        link_t *current = list_first(&instance->batch_list);
-        while (current && current != &instance->batch_list.head) {
-                link_t * const next = current->next;
-                uhci_transfer_batch_t *batch =
-                    uhci_transfer_batch_from_link(current);
+        list_foreach_safe(instance->batch_list, current, next) {
+                uhci_transfer_batch_t *batch = uhci_transfer_batch_from_link(current);
 
                 if (uhci_transfer_batch_check_completed(batch)) {
-                        /* Remove from schedule, save for processing */
-                        fibril_mutex_lock(&batch->base.ep->guard);
                         assert(batch->base.ep->active_batch == &batch->base);
+                        endpoint_deactivate_locked(batch->base.ep);
                         hc_reset_toggles(&batch->base, &uhci_reset_toggle);
-                        endpoint_deactivate_locked(batch->base.ep);
                         transfer_list_remove_batch(instance, batch);
-                        fibril_mutex_unlock(&batch->base.ep->guard);
-
-                        list_append(current, done);
+                        usb_transfer_batch_finish(&batch->base);
                 }
-                current = next;
         }
         fibril_mutex_unlock(&instance->guard);

uspace/drv/bus/usb/uhci/transfer_list.h

@@ -59 +59 @@
 int transfer_list_init(transfer_list_t *, const char *);
 void transfer_list_set_next(transfer_list_t *, transfer_list_t *);
-void transfer_list_add_batch(transfer_list_t *, uhci_transfer_batch_t *);
+int transfer_list_add_batch(transfer_list_t *, uhci_transfer_batch_t *);
 void transfer_list_remove_batch(transfer_list_t *, uhci_transfer_batch_t *);
-void transfer_list_remove_finished(transfer_list_t *, list_t *);
+void transfer_list_check_finished(transfer_list_t *);
 void transfer_list_abort_all(transfer_list_t *);