tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.58k stars 2.47k forks source link

MdeModulePkg/UsbBusDxe: USB issue fix when the port reset #5794

Closed changab closed 3 months ago

changab commented 3 months ago

BZ #4456 Fixed a bug which led to an ASSERT due to the USB device context being maintained after a port reset, but the underlying XHCI context was uninitialized. Specifically, Xhc->UsbDevContext is freed after a reset and only re-allocates the default [0] enpoint transfer ring. In order to avoid a memory leak, device enumeration is performed after freeing the necessary buffers. This allocates the Xhc->UsbDevContext for all endpoints of the USB device.

Description

As mentioned in the commit message. Our conversation in the edk2 mailing list before the tianocore PR process is copied to this PR conversation.

How This Was Tested

Tested on AMD server platform as the remote USB device exposed by BMC triggers this symptom. The issue was gone after this fix.

changab commented 3 months ago

Hao Wu: 07/31/23 Really sorry for another question.

My take is that the transfer ring of other endpoints (besides the Default Control Endpoint) will be re-initialized by the below flow: UsbSetConfig -> UsbCtrlRequest (USB_REQ_SET_CONFIG) -> XhcSetConfigCmd(64) -> XhcInitializeEndpointContext(64)

Could you help to elaborate a bit more on what is the issue with the above (current) flow and why rebuilding the Descriptor Table before UsbSetConfig can resolve it?

Brit 08/17/23

_Its no problem. I agree that the endpoint transfer rings should be allocated after the UsbSetConfig command, but this is not the case. In XhcControlTransfer, after the if statement checking for USB_REQ_SETCONFIG, the for-loop loops through all of DevDesc.NumConfigurations and calls XhcSetConfigCmd. The issue here is that after a reset is issued XhcInitializeDeviceSlot64 is called which frees Xhc->UsbDevContext[SlotId]. This sets Xhc->UsbDevContext[SlotId].DevDesc.NumConfigurations to 0. So XhcSetConfigCmd is never called, and the other endpoint transfer rings besides the default are never reinitialized. From what I could tell the easiest way to reacquire the proper NumConfigurations value was to call UsbBuildDescTable for the device.

changab commented 3 months ago

@lgao4 @niruiyu , have time to take a look at this? Thanks

changab commented 3 months ago

@niruiyu and @lgao4, Ping!

lgao4 commented 3 months ago

I am not expert for USB module. I know this problem. I think this fix can resolve it. The change looks good to me.

changab commented 3 months ago

Thank you Liming, I will set the push lable on this PR.