Re: USB transaction errors causing RCU stalls and kernel panics

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 4.3.2020 16.12, Oliver Neukum wrote:
> Am Mittwoch, den 04.03.2020, 14:11 +0200 schrieb Mathias Nyman:
>> On 3.3.2020 22.08, Jonas Karlsson wrote:
> 
>>
>> I recently got a report about a bit similar issue on a 4.4 stable kernel, so this
>> might not be xhci-cdns specific.
>>
>> That case involved autosuspend of the cdc-acm, and there was only a short burst of
>> transaction erros and resubmitted URBs even if the device was supposed to be suspended.
>> It looks like cdc_acm autosuspended even if it had URBs pending.
> 
> That must not happen. Do you have details?

Shared what I got in:
https://drive.google.com/open?id=1PjwmIK97bIfugWL697lJCe65yuDVOZhx

cdc-acm is unfortunately not traced, but usb core and xhci is.
grep for "MATTU" to find the relevant places. 

> 
>> I'm guessing that in that case the transfer ring restarted even if link was already "suspeded",
>> causing transaction errors. Ring could be restarted if URBs were resubmitted
>> by the class driver when usb core suspends all interfaces, flushing all pending URBs which
>> calls the URB completion handler.
> 
> How should a class driver do that? It gets -EPROTO and that's it,
> 
> 	Regards
> 		Oliver
> 

This is just my speculation, I haven't checked details yet.

It's not just the class driver that causes this, but a combination of
the following gaps in xhci, cdc-acm, and usb core.

A class driver autosuspending with URBs pending,
and not killing all URBs synchronously when usb core calls suspend for the interface drivers.

xhci restarting an endpoint ring due to a race between xhci_stop_device(), handling 
a Transaction error, and having pending URB (unhandled trb) on the ring. [1]

Sleeping between setting port to U3 link state, and clearing udev->can_submit.
allowing URBs to be submitted during that gap when link is in U3 already.
URBs will complete with -EPROTO, and resubmitted until udev->can_submit is cleared.

usb_runtime_suspend()      
  usb_suspend_both() // suspend a device and its interfaces
    for (each udev->actconfig->interface)
      usb_suspend_interface(udev, intf, msg);
	driver = to_usb_driver(intf->dev.driver);
	status = driver->suspend(intf, msg);  // URBs shuold be killed, sync, miss one??
    usb_suspend_device()
      generic_suspend()
        usb_port_suspend()
          hub_set_port_link_state(USB_SS_PORT_LS_U3)
            xhci_stop_device()
            xhci_set_link_state(USB_SS_PORT_LS_U3)  // port link is in U3
            msleep()       // during this URBs can be resubmitteded and complete with -EPROTO loop       
    udev->can_submit = 0;  // we can submit URBs until here (except the killed ones, they are flagged).
    for (each endpoint)
      usb_hcd_flush_endpoint(udev, udev->ep_out[i]);
      usb_hcd_flush_endpoint(udev, udev->ep_in[i]);

[1]
xhci_stop_device()
  xhci_queue_stop_endpoint()
-> interrupt (transfer event, ring has not stopped yet)
  handle_tx_event()  // bulk transfer with Transaction error
    process_bulk_intr_td()
      finish_td()
	xhci_cleanup_halted_endpoint()
          xhci_queue_reset_ep()
          xhci_queue_new_dequeue_state()
-> interupt, command completion event for stop endpoint, 
-> interrupt handle reset_ep command, xhci_handle_cmd_stop_ep()
-> interrupt handle new deq state
      xhci_handle_cmd_set_deq()
	ring_doorbell_for_active_rings(xhci, slot_id, ep_index)  - this restarts the ring.

-Mathias