[PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[brit.chesley]

From: Britton Chesley <Brit.Chesley@amd.com>

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. Added build descriptor table call in UsbIoPortReset. BZ 4456

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Abner Chang <Abner.Chang@amd.com>
Signed-off-by: Britton Chesley <Brit.Chesley@amd.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 26 ++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
index c25f3cc2f279..a5b798bd8d6c 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
@@ -821,6 +821,7 @@ UsbIoPortReset (
   EFI_TPL        OldTpl;
   EFI_STATUS     Status;
   UINT8          DevAddress;
+  UINT8          Config;
 
   OldTpl = gBS->RaiseTPL (USB_BUS_TPL);
 
@@ -882,8 +883,26 @@ UsbIoPortReset (
   // is in CONFIGURED state.
   //
   if (Dev->ActiveConfig != NULL) {
-    Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
+    UsbFreeDevDesc (Dev->DevDesc);
 
+    Status = UsbRemoveConfig (Dev);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to remove configuration - %r\n", Status));
+    }
+
+    Status = UsbGetMaxPacketSize0 (Dev);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to get max packet size - %r\n", Status));
+    }
+
+    Status = UsbBuildDescTable (Dev);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to build descriptor table - %r\n", Status));
+    }
+
+    Config = Dev->DevDesc->Configs[0]->Desc.ConfigurationValue;
+
+    Status = UsbSetConfig (Dev, Config);
     if (EFI_ERROR (Status)) {
       DEBUG ((
         DEBUG_ERROR,
@@ -892,6 +911,11 @@ UsbIoPortReset (
         Status
         ));
     }
+
+    Status = UsbSelectConfig (Dev, Config);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to set configuration - %r\n", Status));
+    }
   }
 
 ON_EXIT:
-- 
2.36.1

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> brit.chesley via groups.io
> Sent: Saturday, July 8, 2023 1:07 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Abner Chang <Abner.Chang@amd.com>
> Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild
> Descriptor Table
> 
> From: Britton Chesley <Brit.Chesley@amd.com>
> 
> 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. Added build


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?

Thanks in advance.

Best Regards,
Hao Wu


> descriptor table call in UsbIoPortReset. BZ 4456
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Abner Chang <Abner.Chang@amd.com>
> Signed-off-by: Britton Chesley <Brit.Chesley@amd.com>
> ---
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 26
> ++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index c25f3cc2f279..a5b798bd8d6c 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -821,6 +821,7 @@ UsbIoPortReset (
>    EFI_TPL        OldTpl;
>    EFI_STATUS     Status;
>    UINT8          DevAddress;
> +  UINT8          Config;
> 
>    OldTpl = gBS->RaiseTPL (USB_BUS_TPL);
> 
> @@ -882,8 +883,26 @@ UsbIoPortReset (
>    // is in CONFIGURED state.
>    //
>    if (Dev->ActiveConfig != NULL) {
> -    Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
> +    UsbFreeDevDesc (Dev->DevDesc);
> 
> +    Status = UsbRemoveConfig (Dev);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to remove
> configuration - %r\n", Status));
> +    }
> +
> +    Status = UsbGetMaxPacketSize0 (Dev);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to get max packet size -
>  %r\n", Status));
> +    }
> +
> +    Status = UsbBuildDescTable (Dev);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to build descriptor
> table - %r\n", Status));
> +    }
> +
> +    Config = Dev->DevDesc->Configs[0]->Desc.ConfigurationValue;
> +
> +    Status = UsbSetConfig (Dev, Config);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((
>          DEBUG_ERROR,
> @@ -892,6 +911,11 @@ UsbIoPortReset (
>          Status
>          ));
>      }
> +
> +    Status = UsbSelectConfig (Dev, Config);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to set configuration -
>  %r\n", Status));
> +    }
>    }
> 
>  ON_EXIT:
> --
> 2.36.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107396): https://edk2.groups.io/g/devel/message/107396
Mute This Topic: https://groups.io/mt/100010162/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[Chesley, Brit via groups.io]

[AMD Official Use Only - General]

Hello Hao,

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_SET_CONFIG, 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.

Best,

Brit Chesley

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Monday, July 31, 2023 2:37 AM
> To: devel@edk2.groups.io; Chesley, Brit <Brit.Chesley@amd.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>; Chang, Abner
> <Abner.Chang@amd.com>
> Subject: RE: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild
> Descriptor Table
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > brit.chesley via groups.io
> > Sent: Saturday, July 8, 2023 1:07 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Abner Chang <Abner.Chang@amd.com>
> > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild
> > Descriptor Table
> >
> > From: Britton Chesley <Brit.Chesley@amd.com>
> >
> > 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. Added
> > build
>
>
> 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?
>
> Thanks in advance.
>
> Best Regards,
> Hao Wu
>
>
> > descriptor table call in UsbIoPortReset. BZ 4456
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Abner Chang <Abner.Chang@amd.com>
> > Signed-off-by: Britton Chesley <Brit.Chesley@amd.com>
> > ---
> >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 26
> > ++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > index c25f3cc2f279..a5b798bd8d6c 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > @@ -821,6 +821,7 @@ UsbIoPortReset (
> >    EFI_TPL        OldTpl;
> >    EFI_STATUS     Status;
> >    UINT8          DevAddress;
> > +  UINT8          Config;
> >
> >    OldTpl = gBS->RaiseTPL (USB_BUS_TPL);
> >
> > @@ -882,8 +883,26 @@ UsbIoPortReset (
> >    // is in CONFIGURED state.
> >    //
> >    if (Dev->ActiveConfig != NULL) {
> > -    Status = UsbSetConfig (Dev, Dev->ActiveConfig-
> >Desc.ConfigurationValue);
> > +    UsbFreeDevDesc (Dev->DevDesc);
> >
> > +    Status = UsbRemoveConfig (Dev);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to remove
> > configuration - %r\n", Status));
> > +    }
> > +
> > +    Status = UsbGetMaxPacketSize0 (Dev);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to get max packet
> > + size -
> >  %r\n", Status));
> > +    }
> > +
> > +    Status = UsbBuildDescTable (Dev);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to build
> > + descriptor
> > table - %r\n", Status));
> > +    }
> > +
> > +    Config = Dev->DevDesc->Configs[0]->Desc.ConfigurationValue;
> > +
> > +    Status = UsbSetConfig (Dev, Config);
> >      if (EFI_ERROR (Status)) {
> >        DEBUG ((
> >          DEBUG_ERROR,
> > @@ -892,6 +911,11 @@ UsbIoPortReset (
> >          Status
> >          ));
> >      }
> > +
> > +    Status = UsbSelectConfig (Dev, Config);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "UsbIoPortReset: Failed to set
> > + configuration -
> >  %r\n", Status));
> > +    }
> >    }
> >
> >  ON_EXIT:
> > --
> > 2.36.1
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107798): https://edk2.groups.io/g/devel/message/107798
Mute This Topic: https://groups.io/mt/100010162/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[Chang, Abner via groups.io]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

Hi Hao,
What is your comment on the response from Brit? Hoping that I didn't miss the latest conversation of this thread.

Thanks
Abner


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119598): https://edk2.groups.io/g/devel/message/119598
Mute This Topic: https://groups.io/mt/100010162/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 935 bytes --]

回复: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[gaoliming via groups.io]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Abner:

  Is PR for this change?

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Chang, Abner via groups.io
发送时间: 2024年6月18日 10:50
收件人: Chesley, Brit <brit.chesley@amd.com>; devel@edk2.groups.io
主题: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

 

Hi Hao,
What is your comment on the response from Brit? Hoping that I didn't miss the latest conversation of this thread.

Thanks
Abner 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119600): https://edk2.groups.io/g/devel/message/119600
Mute This Topic: https://groups.io/mt/106736045/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 4687 bytes --]

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[Chang, Abner via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

There is no PR for this one as we are still in the middle of conversation. Would you like to have a PR for this one? We can create one if necessary.

Thanks
Abner

From: gaoliming <gaoliming@byosoft.com.cn>
Sent: Tuesday, June 18, 2024 1:37 PM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>; Chesley, Brit <Brit.Chesley@amd.com>
Subject: 回复: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Abner:
  Is PR for this change?

Thanks
Liming
发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Chang, Abner via groups.io
发送时间: 2024年6月18日 10:50
收件人: Chesley, Brit <brit.chesley@amd.com<mailto:brit.chesley@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
主题: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

Hi Hao,
What is your comment on the response from Brit? Hoping that I didn't miss the latest conversation of this thread.

Thanks
Abner



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119601): https://edk2.groups.io/g/devel/message/119601
Mute This Topic: https://groups.io/mt/106736083/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 8252 bytes --]

Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

[Chang, Abner via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Liming,
I created PR here: https://github.com/tianocore/edk2/pull/5794

Thanks
Abner

From: gaoliming <gaoliming@byosoft.com.cn>
Sent: Tuesday, June 18, 2024 1:37 PM
To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>; Chesley, Brit <Brit.Chesley@amd.com>
Subject: 回复: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

Abner:
  Is PR for this change?

Thanks
Liming
发件人: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> 代表 Chang, Abner via groups.io
发送时间: 2024年6月18日 10:50
收件人: Chesley, Brit <brit.chesley@amd.com<mailto:brit.chesley@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
主题: Re: [edk2-devel] [PATCH v2 1/1] MdeModulePkg: UsbBusDxe: Rebuild Descriptor Table

Hi Hao,
What is your comment on the response from Brit? Hoping that I didn't miss the latest conversation of this thread.

Thanks
Abner



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119622): https://edk2.groups.io/g/devel/message/119622
Mute This Topic: https://groups.io/mt/106752962/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 8352 bytes --]