[02/20] usb: host: xhci: check DYING state earlier

[Alan Stern <stern@rowland.harvard.edu>]

On Wed, 2 May 2018, Felipe Balbi wrote:

> >> Look at the result. With this change we don't need to take a lock,
> >> allocate memory, search for endpoint index, search for endpoint
> >> state. All of those are needed for proper operation of the function, but
> >> if the controller has already died, there's no point in going any
> >> further.
> >
> > But we might miss the fact that host died, and go even further, adding URB to list,
> > writing TRBs to ringbuffers etc.
> >
> > In code we save one line,
> > goto: free_priv
> 
> We're saving a lot more than that, actually. All of the following ends
> up being skipped. All of these are unnecessary work when xHC has already
> died:
> 
> 8<------------------------------------------------------------------------
> 
> slot_id = urb->dev->slot_id;
> ep_index = xhci_get_endpoint_index(&urb->ep->desc);
> ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
> 
> if (!HCD_HW_ACCESSIBLE(hcd)) {
>         if (!in_interrupt())
>                 xhci_dbg(xhci, "urb submitted during PCI suspend\n");
>         return -ESHUTDOWN;
> }
> 
> if (usb_endpoint_xfer_isoc(&urb->ep->desc))
>         num_tds = urb->number_of_packets;
> else if (usb_endpoint_is_bulk_out(&urb->ep->desc) &&
>     urb->transfer_buffer_length > 0 &&
>     urb->transfer_flags & URB_ZERO_PACKET &&
>     !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc)))
>         num_tds = 2;
> else
>         num_tds = 1;
> 
> urb_priv = kzalloc(sizeof(struct urb_priv) +
>                    num_tds * sizeof(struct xhci_td), mem_flags);
> if (!urb_priv)
>         return -ENOMEM;
> 
> urb_priv->num_tds = num_tds;
> urb_priv->num_tds_done = 0;
> urb->hcpriv = urb_priv;
> 
> trace_xhci_urb_enqueue(urb);
> 
> if (usb_endpoint_xfer_control(&urb->ep->desc)) {
>         /* Check to see if the max packet size for the default control
>          * endpoint changed during FS device enumeration
>          */
>         if (urb->dev->speed == USB_SPEED_FULL) {
>                 ret = xhci_check_maxpacket(xhci, slot_id,
>                                 ep_index, urb);
>                 if (ret < 0) {
>                         xhci_urb_free_priv(urb_priv);
>                         urb->hcpriv = NULL;
>                         return ret;
>                 }
>         }
> }
> 
> spin_lock_irqsave(&xhci->lock, flags);
> 
> 8<------------------------------------------------------------------------

Regardless, how often does it happen that an xHCI host controller dies?  
A few times a day for every xHCI controller in the world seems like a
gross overestimate.

But even if it isn't, who cares if we end up executing the code above a
few extra times per day?  It's the "almost never used" path.  :-)

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html