Changeset 0892663a in mainline for uspace/lib/usbhost/src

Timestamp:

2018-01-11T04:14:37Z (8 years ago)

Author:

Ondřej Hlavatý <aearsis@…>

Branches:

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

Children:

9848c77

Parents:

bad4a05

git-author:

Ondřej Hlavatý <aearsis@…> (2018-01-11 03:59:03)

git-committer:

Ondřej Hlavatý <aearsis@…> (2018-01-11 04:14:37)

Message:

usbhost: device removal and off/onlining moved into the library

Also, it is just not possible to make generic transfer abortion. So the
current semantics of endpoint unregistering is also aborting the pending
transfer. As it is not yet implemented in XHCI, and the stub in UHCI is
missing the magic, it breaks offlining interrupt devices, such as mouse.

When finishing this commit, I came across the fact we need some more
synchronization above device. Guard can protect internal structures, but
it cannot synchronize multiple calls to offline, or offline & removal
between each other - they both need to allow driver to unregister
endpoints, and as such must release the guard.

Location:

uspace/lib/usbhost/src

Files:

• bus.c (modified) (4 diffs)
• ddf_helpers.c (modified) (3 diffs)
• endpoint.c (modified) (3 diffs)
• usb_transfer_batch.c (modified) (1 diff)

uspace/lib/usbhost/src/bus.c

@@ -43 +43 @@
 #include <mem.h>
 #include <stdio.h>
+#include <str_error.h>
 #include <usb/debug.h>
 
@@ -97 +98 @@
 /**
  * Invoke the device_enumerate bus operation.
+ *
+ * There's no need to synchronize here, because no one knows the device yet.
  */
 int bus_device_enumerate(device_t *dev)
@@ -106 +109 @@
                 return ENOTSUP;
 
-        return ops->device_enumerate(dev);
-}
-
-/**
- * Invoke the device_remove bus operation.
+        if (dev->online) {
+                fibril_mutex_unlock(&dev->guard);
+                return EINVAL;
+        }
+
+        const int r = ops->device_enumerate(dev);
+        if (!r) {
+                dev->online = true;
+
+                fibril_mutex_lock(&dev->hub->guard);
+                list_append(&dev->link, &dev->hub->devices);
+                fibril_mutex_unlock(&dev->hub->guard);
+        }
+
+        return r;
+}
+
+/**
+ * Clean endpoints and children that could have been left behind after
+ * asking the driver of device to offline/remove a device.
+ *
+ * Note that EP0's lifetime is shared with the device, and as such is not
+ * touched.
+ */
+static void device_clean_ep_children(device_t *dev, const char *op)
+{
+        assert(fibril_mutex_is_locked(&dev->guard));
+
+        /* Unregister endpoints left behind. */
+        for (usb_endpoint_t i = 1; i < USB_ENDPOINT_MAX; ++i) {
+                if (!dev->endpoints[i])
+                        continue;
+
+                usb_log_warning("USB device '%s' driver left endpoint %u registered after %s.",
+                    ddf_fun_get_name(dev->fun), i, op);
+
+                endpoint_t * const ep = dev->endpoints[i];
+                endpoint_add_ref(ep);
+                
+                fibril_mutex_unlock(&dev->guard);
+                bus_endpoint_remove(ep);
+                fibril_mutex_lock(&dev->guard);
+
+                assert(dev->endpoints[i] == NULL);
+        }
+
+        /* Remove also orphaned children. */
+        while (!list_empty(&dev->devices)) {
+                device_t * const child = list_get_instance(list_first(&dev->devices), device_t, link);
+
+                usb_log_warning("USB device '%s' driver left device '%s' behind after %s.",
+                    ddf_fun_get_name(dev->fun), ddf_fun_get_name(child->fun), op);
+                /*
+                 * The child node won't disappear, because its parent's driver
+                 * is already dead. And the child will need the guard to remove
+                 * itself from the list.
+                 */
+                fibril_mutex_unlock(&dev->guard);
+                bus_device_remove(child);
+                fibril_mutex_lock(&dev->guard);
+        }
+        assert(list_empty(&dev->devices));
+}
+
+/**
+ * Resolve a USB device that is gone.
  */
 void bus_device_remove(device_t *dev)
 {
         assert(dev);
+        assert(dev->fun == NULL);
 
         const bus_ops_t *ops = BUS_OPS_LOOKUP(dev->bus->ops, device_remove);
         assert(ops);
 
-        return ops->device_remove(dev);
-}
-
-/**
- * Invoke the device_online bus operation.
+        /* First, block new transfers and operations. */
+        fibril_mutex_lock(&dev->guard);
+        dev->online = false;
+
+        /* Unbinding will need guard unlocked. */
+        fibril_mutex_unlock(&dev->guard);
+
+        /* Remove our device from our hub's children. */
+        fibril_mutex_lock(&dev->hub->guard);
+        list_remove(&dev->link);
+        fibril_mutex_unlock(&dev->hub->guard);
+
+        /*
+         * Unbind the DDF function. That will result in dev_gone called in
+         * driver, which shall destroy its pipes and remove its children.
+         */
+        const int err = ddf_fun_unbind(dev->fun);
+        if (err) {
+                usb_log_error("Failed to unbind USB device '%s': %s",
+                    ddf_fun_get_name(dev->fun), str_error(err));
+                return;
+        }
+
+        /* Remove what driver left behind */
+        fibril_mutex_lock(&dev->guard);
+        device_clean_ep_children(dev, "removing");
+
+        /* Tell the HC to release its resources. */
+        ops->device_remove(dev);
+
+        /* Release the EP0 bus reference */
+        endpoint_del_ref(dev->endpoints[0]);
+
+        /* Destroy the function, freeing also the device, unlocking mutex. */
+        ddf_fun_destroy(dev->fun);
+}
+
+/**
+ * The user wants this device back online.
  */
 int bus_device_online(device_t *dev)
 {
+        int err;
         assert(dev);
 
+        fibril_mutex_lock(&dev->guard);
+        if (dev->online) {
+                fibril_mutex_unlock(&dev->guard);
+                return EINVAL;
+        }
+
+        /* First, tell the HC driver. */
         const bus_ops_t *ops = BUS_OPS_LOOKUP(dev->bus->ops, device_online);
-        if (!ops)
-                return ENOTSUP;
-
-        return ops->device_online(dev);
-}
-
-/**
- * Invoke the device_offline bus operation.
+        if (ops && (err = ops->device_online(dev))) {
+                usb_log_warning("Host controller refused to make device '%s' online: %s",
+                    ddf_fun_get_name(dev->fun), str_error(err));
+                return err;
+        }
+
+        /* Allow creation of new endpoints and communication with the device. */
+        dev->online = true;
+
+        /* Onlining will need the guard */
+        fibril_mutex_unlock(&dev->guard);
+
+        if ((err = ddf_fun_online(dev->fun))) {
+                usb_log_warning("Failed to take device '%s' online: %s",
+                    ddf_fun_get_name(dev->fun), str_error(err));
+                return err;
+        }
+
+        usb_log_info("USB Device '%s' offlined.", ddf_fun_get_name(dev->fun));
+        return EOK;
+}
+
+/**
+ * The user requested to take this device offline.
  */
 int bus_device_offline(device_t *dev)
 {
+        int err;
         assert(dev);
 
+        /* Make sure we're the one who offlines this device */
+        if (!dev->online)
+                return ENOENT;
+
+        /*
+         * XXX: If the device is removed/offlined just now, this can fail on
+         * assertion. We most probably need some kind of enum status field to
+         * make the synchronization work.
+         */
+        
+        /* Tear down all drivers working with the device. */
+        if ((err = ddf_fun_offline(dev->fun))) {
+                return err;
+        }
+
+        fibril_mutex_lock(&dev->guard);
+        dev->online = false;
+        device_clean_ep_children(dev, "offlining");
+
+        /* Tell also the HC driver. */
         const bus_ops_t *ops = BUS_OPS_LOOKUP(dev->bus->ops, device_offline);
-        if (!ops)
-                return ENOTSUP;
-
-        return ops->device_offline(dev);
+        if (ops)
+                ops->device_offline(dev);
+
+        fibril_mutex_unlock(&dev->guard);
+        usb_log_info("USB Device '%s' offlined.", ddf_fun_get_name(dev->fun));
+        return EOK;
 }
 
@@ -269 +415 @@
         fibril_mutex_unlock(&device->guard);
 
-        /* Abort a transfer batch, if there was any */
-        endpoint_abort(ep);
-
         /* Bus reference */
         endpoint_del_ref(ep);

uspace/lib/usbhost/src/ddf_helpers.c

@@ -336 +336 @@
         assert(device);
 
-        hc_device_t *hcd = dev_to_hcd(device);
-        assert(hcd);
-        assert(hcd->bus);
+        device_t *victim = NULL;
 
         fibril_mutex_lock(&hub->guard);
-
-        device_t *victim = NULL;
-
         list_foreach(hub->devices, link, device_t, it) {
                 if (it->port == port) {
@@ -350 +345 @@
                 }
         }
-        if (victim) {
-                assert(victim->fun);
-                assert(victim->port == port);
-                assert(victim->hub == hub);
-                list_remove(&victim->link);
-                fibril_mutex_unlock(&hub->guard);
-                const int ret = ddf_fun_unbind(victim->fun);
-                if (ret == EOK) {
-                        bus_device_remove(victim);
-                        ddf_fun_destroy(victim->fun);
-                } else {
-                        usb_log_warning("Failed to unbind device `%s': %s\n",
-                            ddf_fun_get_name(victim->fun), str_error(ret));
-                }
-                return EOK;
-        }
         fibril_mutex_unlock(&hub->guard);
-        return ENOENT;
+
+        if (!victim)
+                return ENOENT;
+
+        assert(victim->fun);
+        assert(victim->port == port);
+        assert(victim->hub == hub);
+
+        bus_device_remove(victim);
+        return EOK;
 }
 
@@ -477 +465 @@
         }
 
-        fibril_mutex_lock(&hub->guard);
-        list_append(&dev->link, &hub->devices);
-        fibril_mutex_unlock(&hub->guard);
-
         return EOK;
 

uspace/lib/usbhost/src/endpoint.c

@@ -124 +124 @@
 
 /**
+ * Wait until the endpoint have no transfer scheduled.
+ */
+void endpoint_wait_timeout_locked(endpoint_t *ep, suseconds_t timeout)
+{
+        assert(fibril_mutex_is_locked(&ep->guard));
+
+        while (ep->active_batch != NULL)
+                fibril_condvar_wait_timeout(&ep->avail, &ep->guard, timeout);
+}
+
+/**
  * Mark the endpoint as active and block access for further fibrils. If the
  * endpoint is already active, it will block on ep->avail condvar.
@@ -138 +149 @@
         assert(batch);
         assert(batch->ep == ep);
-        assert(fibril_mutex_is_locked(&ep->guard));
-
-        while (ep->active_batch != NULL)
-                fibril_condvar_wait(&ep->avail, &ep->guard);
+
+        endpoint_wait_timeout_locked(ep, 0);
         ep->active_batch = batch;
 }
@@ -160 +169 @@
         ep->active_batch = NULL;
         fibril_condvar_signal(&ep->avail);
-}
-
-/**
- * Abort an active batch on endpoint, if any.
- *
- * @param[in] ep endpoint_t structure.
- */
-void endpoint_abort(endpoint_t *ep)
-{
-        assert(ep);
-
-        fibril_mutex_lock(&ep->guard);
-        usb_transfer_batch_t *batch = ep->active_batch;
-        endpoint_deactivate_locked(ep);
-        fibril_mutex_unlock(&ep->guard);
-
-        if (batch)
-                usb_transfer_batch_abort(batch);
 }
 

uspace/lib/usbhost/src/usb_transfer_batch.c

@@ -128 +128 @@
 
 /**
- * Finish a transfer batch as an aborted one.
- */
-void usb_transfer_batch_abort(usb_transfer_batch_t *batch)
-{
-        assert(batch);
-        assert(batch->ep);
-
-        batch->error = EAGAIN;
-        usb_transfer_batch_finish(batch);
-}
-
-/**
  * @}
  */