[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>

src/drivers/bus/usb.c

@@ -296,9 +296,7 @@ int usb_endpoint_open ( struct usb_endpoint *ep ) {
 		goto err_already;
 	}
 	usb->ep[idx] = ep;
-
+	INIT_LIST_HEAD ( &ep->halted );
-	/* Clear any stale error status */
-	ep->rc = 0;
 
 	/* 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 */
+	/* Remove from list of halted endpoints */
-	ep->rc = 0;
+	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) */
+	/* Schedule reset, if applicable */
-	ep->rc = rc;
 	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 "
+				DBGC ( usb, "USB %s stale control completion: "
-				       "completion:\n", usb->name );
+				       "%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_del ( &port->changed );
-	list_add_tail ( &port->list, &bus->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 );
+		list_del ( &port->changed );
-		INIT_LIST_HEAD ( &port->list );
+		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 );
+		list_del ( &port->changed );
-		INIT_LIST_HEAD ( &port->list );
+		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;
 

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;