Changeset 4db49344 in mainline for uspace/lib/usbhost

Timestamp:

2018-01-23T21:52:28Z (7 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/lib/usbhost

Files:

• include/usb/host/endpoint.h (modified) (4 diffs)
• src/endpoint.c (modified) (3 diffs)

uspace/lib/usbhost/include/usb/host/endpoint.h

@@ -53 +53 @@
 typedef struct usb_transfer_batch usb_transfer_batch_t;
 
-/** Host controller side endpoint structure. */
+/**
+ * Host controller side endpoint structure.
+ *
+ * This structure, though reference-counted, is very fragile. It is responsible
+ * for synchronizing transfer batch scheduling and completion.
+ *
+ * To avoid situations, in which two locks must be obtained to schedule/finish
+ * a transfer, the endpoint inherits a lock from the outside. Because the
+ * concrete instance of mutex can be unknown at the time of initialization,
+ * the HC shall pass the right lock at the time of onlining the endpoint.
+ *
+ * The fields used for scheduling (online, active_batch) are to be used only
+ * under that guard and by functions designed for this purpose. The driver can
+ * also completely avoid using this mechanism, in which case it is on its own in
+ * question of transfer aborting.
+ *
+ * Relevant information can be found in the documentation of HelenOS xHCI
+ * project.
+ */
 typedef struct endpoint {
         /** USB device */
@@ -59 +77 @@
         /** Reference count. */
         atomic_t refcnt;
-        /** Reserved bandwidth. */
-        size_t bandwidth;
-        /** The currently active transfer batch. Write using methods, read under guard. */
+
+        /** An inherited guard */
+        fibril_mutex_t *guard;
+        /** Whether it's allowed to schedule on this endpoint */
+        bool online;
+        /** The currently active transfer batch. */
         usb_transfer_batch_t *active_batch;
-        /** Protects resources and active status changes. */
-        fibril_mutex_t guard;
         /** Signals change of active status. */
         fibril_condvar_t avail;
 
-        /** Enpoint number */
+        /** Reserved bandwidth. Needed for USB2 bus. */
+        size_t bandwidth;
+        /** Endpoint number */
         usb_endpoint_t endpoint;
         /** Communication direction. */
@@ -79 +100 @@
         /** Maximum size of one transfer */
         size_t max_transfer_size;
-        /** Number of packats that can be sent in one service interval (not necessarily uframe) */
+        /**
+         * Number of packets that can be sent in one service interval
+         * (not necessarily uframe, despite its name)
+         */
         unsigned packets_per_uframe;
 
@@ -90 +114 @@
 extern void endpoint_del_ref(endpoint_t *);
 
+extern void endpoint_set_online(endpoint_t *, fibril_mutex_t *);
+extern void endpoint_set_offline_locked(endpoint_t *);
+
 extern void endpoint_wait_timeout_locked(endpoint_t *ep, suseconds_t);
-extern void endpoint_activate_locked(endpoint_t *, usb_transfer_batch_t *);
+extern int endpoint_activate_locked(endpoint_t *, usb_transfer_batch_t *);
 extern void endpoint_deactivate_locked(endpoint_t *);
 

uspace/lib/usbhost/src/endpoint.c

@@ -61 +61 @@
 
         atomic_set(&ep->refcnt, 0);
-        fibril_mutex_initialize(&ep->guard);
         fibril_condvar_initialize(&ep->avail);
 
@@ -122 +121 @@
 
 /**
- * Wait until the endpoint have no transfer scheduled.
+ * Mark the endpoint as online. Supply a guard to be used for this endpoint
+ * synchronization.
+ */
+void endpoint_set_online(endpoint_t *ep, fibril_mutex_t *guard)
+{
+        ep->guard = guard;
+        ep->online = true;
+}
+
+/**
+ * Mark the endpoint as offline. All other fibrils waiting to activate this
+ * endpoint will be interrupted.
+ */
+void endpoint_set_offline_locked(endpoint_t *ep)
+{
+        assert(ep);
+        assert(fibril_mutex_is_locked(ep->guard));
+
+        ep->online = false;
+        fibril_condvar_broadcast(&ep->avail);
+}
+
+/**
+ * Wait until a transfer finishes. Can be used even when the endpoint is
+ * offline (and is interrupted by the endpoint going offline).
  */
 void endpoint_wait_timeout_locked(endpoint_t *ep, suseconds_t timeout)
 {
-        assert(fibril_mutex_is_locked(&ep->guard));
-
-        if (ep->active_batch != NULL)
-                fibril_condvar_wait_timeout(&ep->avail, &ep->guard, timeout);
-
-        while (timeout == 0 && ep->active_batch != NULL)
-                fibril_condvar_wait_timeout(&ep->avail, &ep->guard, timeout);
+        assert(ep);
+        assert(fibril_mutex_is_locked(ep->guard));
+
+        if (ep->active_batch == NULL)
+                return;
+
+        fibril_condvar_wait_timeout(&ep->avail, ep->guard, timeout);
 }
 
@@ -140 +163 @@
  *
  * Call only under endpoint guard. After you activate the endpoint and release
- * the guard, you must assume that particular transfer is already finished/aborted.
- *
- * @param ep endpoint_t structure.
- * @param batch Transfer batch this endpoint is bocked by.
- */
-void endpoint_activate_locked(endpoint_t *ep, usb_transfer_batch_t *batch)
+ * the guard, you must assume that particular transfer is already
+ * finished/aborted.
+ *
+ * Activation and deactivation is not done by the library to maximize
+ * performance. The HC might want to prepare some memory buffers prior to
+ * interfering with other world.
+ *
+ * @param batch Transfer batch this endpoint is blocked by.
+ */
+int endpoint_activate_locked(endpoint_t *ep, usb_transfer_batch_t *batch)
 {
         assert(ep);
         assert(batch);
         assert(batch->ep == ep);
-
-        endpoint_wait_timeout_locked(ep, 0);
+        assert(ep->guard);
+        assert(fibril_mutex_is_locked(ep->guard));
+
+        while (ep->online && ep->active_batch != NULL)
+                fibril_condvar_wait(&ep->avail, ep->guard);
+
+        if (!ep->online)
+                return EINTR;
+
+        assert(ep->active_batch == NULL);
         ep->active_batch = batch;
+        return EOK;
 }
 
 /**
  * Mark the endpoint as inactive and allow access for further fibrils.
- *
- * @param ep endpoint_t structure.
  */
 void endpoint_deactivate_locked(endpoint_t *ep)
 {
         assert(ep);
-        assert(fibril_mutex_is_locked(&ep->guard));
+        assert(fibril_mutex_is_locked(ep->guard));
+
         ep->active_batch = NULL;
         fibril_condvar_signal(&ep->avail);