[usb] Reset endpoints without waiting for a new transfer to be enqueued

The current endpoint reset logic defers the reset until the caller
attempts to enqueue a new transfer to that endpoint.  This is
insufficient when dealing with endpoints behind a transaction
translator, since the transaction translator is a resource shared
between multiple endpoints.

We cannot reset the endpoint as part of the completion handling, since
that would introduce recursive calls to usb_poll().  Instead, we
add the endpoint to a list of halted endpoints, and perform the reset
on the next call to usb_step().

Signed-off-by: Michael Brown <mcb30@ipxe.org>

diff --git a/src/drivers/bus/usb.c b/src/drivers/bus/usb.c
index 54a115efc7e8cf7c8f7369f4fe317e7ca6eef1d0..a0334243a544142fc4f38b85f57ec88e2baa3121 100644 (file)
--- a/src/drivers/bus/usb.c
+++ b/src/drivers/bus/usb.c
@@ -296,9 +296,7 @@ int usb_endpoint_open ( struct usb_endpoint *ep ) {
                goto err_already;
        }
        usb->ep[idx] = ep;
-
-       /* Clear any stale error status */
-       ep->rc = 0;
+       INIT_LIST_HEAD ( &ep->halted );
 
        /* Open endpoint */
        if ( ( rc = ep->host->open ( ep ) ) != 0 ) {
@@ -342,6 +340,7 @@ void usb_endpoint_close ( struct usb_endpoint *ep ) {
 
        /* Remove from endpoint list */
        usb->ep[idx] = NULL;
+       list_del ( &ep->halted );
 
        /* Discard any recycled buffers, if applicable */
        if ( ep->max )
@@ -359,6 +358,9 @@ static int usb_endpoint_reset ( struct usb_endpoint *ep ) {
        unsigned int type;
        int rc;
 
+       /* Sanity check */
+       assert ( ! list_empty ( &ep->halted ) );
+
        /* Reset endpoint */
        if ( ( rc = ep->host->reset ( ep ) ) != 0 ) {
                DBGC ( usb, "USB %s %s could not reset: %s\n",
@@ -379,8 +381,9 @@ static int usb_endpoint_reset ( struct usb_endpoint *ep ) {
                return rc;
        }
 
-       /* Clear recorded error */
-       ep->rc = 0;
+       /* Remove from list of halted endpoints */
+       list_del ( &ep->halted );
+       INIT_LIST_HEAD ( &ep->halted );
 
        DBGC ( usb, "USB %s %s reset\n",
               usb->name, usb_endpoint_name ( ep->address ) );
@@ -434,7 +437,8 @@ int usb_message ( struct usb_endpoint *ep, unsigned int request,
                return -ENODEV;
 
        /* Reset endpoint if required */
-       if ( ( ep->rc != 0 ) && ( ( rc = usb_endpoint_reset ( ep ) ) != 0 ) )
+       if ( ( ! list_empty ( &ep->halted ) ) &&
+            ( ( rc = usb_endpoint_reset ( ep ) ) != 0 ) )
                return rc;
 
        /* Zero input data buffer (if applicable) */
@@ -480,7 +484,8 @@ int usb_stream ( struct usb_endpoint *ep, struct io_buffer *iobuf,
                return -ENODEV;
 
        /* Reset endpoint if required */
-       if ( ( ep->rc != 0 ) && ( ( rc = usb_endpoint_reset ( ep ) ) != 0 ) )
+       if ( ( ! list_empty ( &ep->halted ) ) &&
+            ( ( rc = usb_endpoint_reset ( ep ) ) != 0 ) )
                return rc;
 
        /* Enqueue stream transfer */
@@ -507,17 +512,19 @@ int usb_stream ( struct usb_endpoint *ep, struct io_buffer *iobuf,
 void usb_complete_err ( struct usb_endpoint *ep, struct io_buffer *iobuf,
                        int rc ) {
        struct usb_device *usb = ep->usb;
+       struct usb_bus *bus = usb->port->hub->bus;
 
        /* Decrement fill level */
        assert ( ep->fill > 0 );
        ep->fill--;
 
-       /* Record error (if any) */
-       ep->rc = rc;
+       /* Schedule reset, if applicable */
        if ( ( rc != 0 ) && ep->open ) {
                DBGC ( usb, "USB %s %s completion failed: %s\n",
                       usb->name, usb_endpoint_name ( ep->address ),
                       strerror ( rc ) );
+               list_del ( &ep->halted );
+               list_add_tail ( &ep->halted, &bus->halted );
        }
 
        /* Report completion */
@@ -642,6 +649,12 @@ void usb_flush ( struct usb_endpoint *ep ) {
  ******************************************************************************
  */
 
+/** USB control transfer pseudo-header */
+struct usb_control_pseudo_header {
+       /** Completion status */
+       int rc;
+};
+
 /**
  * Complete USB control transfer
  *
@@ -652,13 +665,14 @@ void usb_flush ( struct usb_endpoint *ep ) {
 static void usb_control_complete ( struct usb_endpoint *ep,
                                   struct io_buffer *iobuf, int rc ) {
        struct usb_device *usb = ep->usb;
+       struct usb_control_pseudo_header *pshdr;
 
-       /* Check for failures */
+       /* Record completion status in buffer */
+       pshdr = iob_push ( iobuf, sizeof ( *pshdr ) );
+       pshdr->rc = rc;
        if ( rc != 0 ) {
                DBGC ( usb, "USB %s control transaction failed: %s\n",
                       usb->name, strerror ( rc ) );
-               free_iob ( iobuf );
-               return;
        }
 
        /* Add to list of completed I/O buffers */
@@ -686,17 +700,19 @@ int usb_control ( struct usb_device *usb, unsigned int request,
                  size_t len ) {
        struct usb_bus *bus = usb->port->hub->bus;
        struct usb_endpoint *ep = &usb->control;
+       struct usb_control_pseudo_header *pshdr;
        struct io_buffer *iobuf;
        struct io_buffer *cmplt;
        unsigned int i;
        int rc;
 
        /* Allocate I/O buffer */
-       iobuf = alloc_iob ( len );
+       iobuf = alloc_iob ( sizeof ( *pshdr ) + len );
        if ( ! iobuf ) {
                rc = -ENOMEM;
                goto err_alloc;
        }
+       iob_reserve ( iobuf, sizeof ( *pshdr ) );
        iob_put ( iobuf, len );
        if ( request & USB_DIR_IN ) {
                memset ( data, 0, len );
@@ -722,16 +738,27 @@ int usb_control ( struct usb_device *usb, unsigned int request,
                        /* Remove from completion list */
                        list_del ( &cmplt->list );
 
+                       /* Extract and strip completion status */
+                       pshdr = cmplt->data;
+                       iob_pull ( cmplt, sizeof ( *pshdr ) );
+                       rc = pshdr->rc;
+
                        /* Discard stale completions */
                        if ( cmplt != iobuf ) {
-                               DBGC ( usb, "USB %s stale control "
-                                      "completion:\n", usb->name );
+                               DBGC ( usb, "USB %s stale control completion: "
+                                      "%s\n", usb->name, strerror ( rc ) );
                                DBGC_HDA ( usb, 0, cmplt->data,
                                           iob_len ( cmplt ) );
                                free_iob ( cmplt );
                                continue;
                        }
 
+                       /* Fail immediately if completion was in error */
+                       if ( rc != 0 ) {
+                               free_iob ( cmplt );
+                               return rc;
+                       }
+
                        /* Copy completion to data buffer, if applicable */
                        assert ( iob_len ( cmplt ) <= len );
                        if ( request & USB_DIR_IN )
@@ -740,10 +767,6 @@ int usb_control ( struct usb_device *usb, unsigned int request,
                        return 0;
                }
 
-               /* Fail immediately if endpoint is in an error state */
-               if ( ep->rc )
-                       return ep->rc;
-
                /* Delay */
                mdelay ( 1 );
        }
@@ -1549,8 +1572,8 @@ void usb_port_changed ( struct usb_port *port ) {
        struct usb_bus *bus = hub->bus;
 
        /* Record hub port status change */
-       list_del ( &port->list );
-       list_add_tail ( &port->list, &bus->changed );
+       list_del ( &port->changed );
+       list_add_tail ( &port->changed, &bus->changed );
 }
 
 /**
@@ -1559,23 +1582,35 @@ void usb_port_changed ( struct usb_port *port ) {
  * @v bus              USB bus
  */
 static void usb_step ( struct usb_bus *bus ) {
+       struct usb_endpoint *ep;
        struct usb_port *port;
 
        /* Poll bus */
        usb_poll ( bus );
 
+       /* Attempt to reset first halted endpoint in list, if any.  We
+        * do not attempt to process the complete list, since this
+        * would require extra code to allow for the facts that the
+        * halted endpoint list may change as we do so, and that
+        * resetting an endpoint may fail.
+        */
+       if ( ( ep = list_first_entry ( &bus->halted, struct usb_endpoint,
+                                      halted ) ) != NULL )
+               usb_endpoint_reset ( ep );
+
        /* Handle any changed ports, allowing for the fact that the
         * port list may change as we perform hotplug actions.
         */
        while ( ! list_empty ( &bus->changed ) ) {
 
                /* Get first changed port */
-               port = list_first_entry ( &bus->changed, struct usb_port, list);
+               port = list_first_entry ( &bus->changed, struct usb_port,
+                                         changed );
                assert ( port != NULL );
 
                /* Remove from list of changed ports */
-               list_del ( &port->list );
-               INIT_LIST_HEAD ( &port->list );
+               list_del ( &port->changed );
+               INIT_LIST_HEAD ( &port->changed );
 
                /* Perform appropriate hotplug action */
                usb_hotplug ( port );
@@ -1628,7 +1663,7 @@ struct usb_hub * alloc_usb_hub ( struct usb_bus *bus, struct usb_device *usb,
                port->address = i;
                if ( usb )
                        port->protocol = usb->port->protocol;
-               INIT_LIST_HEAD ( &port->list );
+               INIT_LIST_HEAD ( &port->changed );
        }
 
        return hub;
@@ -1702,8 +1737,8 @@ void unregister_usb_hub ( struct usb_hub *hub ) {
        /* Cancel any pending port status changes */
        for ( i = 1 ; i <= hub->ports ; i++ ) {
                port = usb_port ( hub, i );
-               list_del ( &port->list );
-               INIT_LIST_HEAD ( &port->list );
+               list_del ( &port->changed );
+               INIT_LIST_HEAD ( &port->changed );
        }
 
        /* Remove from hub list */
@@ -1724,7 +1759,7 @@ void free_usb_hub ( struct usb_hub *hub ) {
                port = usb_port ( hub, i );
                assert ( ! port->attached );
                assert ( port->usb == NULL );
-               assert ( list_empty ( &port->list ) );
+               assert ( list_empty ( &port->changed ) );
        }
 
        /* Free hub */
@@ -1762,6 +1797,7 @@ struct usb_bus * alloc_usb_bus ( struct device *dev, unsigned int ports,
        INIT_LIST_HEAD ( &bus->devices );
        INIT_LIST_HEAD ( &bus->hubs );
        INIT_LIST_HEAD ( &bus->changed );
+       INIT_LIST_HEAD ( &bus->halted );
        process_init_stopped ( &bus->process, &usb_process_desc, NULL );
        bus->host = &bus->op->bus;
 

diff --git a/src/include/ipxe/usb.h b/src/include/ipxe/usb.h
index a9c185f6c82e3221d42e0ee12fd642a68e734365..5b95c20c98799cd8daea314a109f965dbcbeab64 100644 (file)
--- a/src/include/ipxe/usb.h
+++ b/src/include/ipxe/usb.h
@@ -380,11 +380,12 @@ struct usb_endpoint {
 
        /** Endpoint is open */
        int open;
-       /** Current failure state (if any) */
-       int rc;
        /** Buffer fill level */
        unsigned int fill;
 
+       /** List of halted endpoints */
+       struct list_head halted;
+
        /** Host controller operations */
        struct usb_endpoint_host_operations *host;
        /** Host controller private data */
@@ -754,7 +755,7 @@ struct usb_port {
         */
        struct usb_device *usb;
        /** List of changed ports */
-       struct list_head list;
+       struct list_head changed;
 };
 
 /** A USB hub */
@@ -888,6 +889,8 @@ struct usb_bus {
        struct list_head hubs;
        /** List of changed ports */
        struct list_head changed;
+       /** List of halted endpoints */
+       struct list_head halted;
        /** Process */
        struct process process;