[PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[Jeremy Linton]

During port reset, the device descriptors should be checked
before attempting to set an endpoint configuration.

In particular this fixes a crash due to
ASSERT(TrsRing != NULL) in XhcSyncTrsRing(). That crash
happens during error recovery on devices attached to XHCI
hosts. This is because the port disable clears and deallocats
all the EP data structures. When the port is reconfigured
without first requesting the EP descriptors,
XhcSetConfigCmd[64]() is not being called because the
NumConfigurations remains 0.

We could attempt to rebuild the EP descriptions directly
from the XHCI driver. OTOH, its probably good practice to
assure the device description is what we expect from within
the core USB subsystem during reset.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
index 4b4915c019..17bb691bf8 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
@@ -874,6 +874,14 @@ UsbIoPortReset (
   // is in CONFIGURED state.
   //
   if (Dev->ActiveConfig != NULL) {
+    Status = UsbBuildDescTable (Dev);
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((EFI_D_ERROR, "UsbIoPortReset: failed to build descriptor table for %d - %r\n",
+                 Dev->Address, Status));
+      goto ON_EXIT;
+    }
+
     Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
 
     if (EFI_ERROR (Status)) {
-- 
2.24.1

Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[Guomin Jiang]

Hi Jeremy,

You can refer https://edk2.groups.io/g/devel/message/58125 for discussion about this solution.

Two issue I found:
1. Memory leakage may occur if doing so and I am investigating it.
2. It test pass with OVMF but fail in real platform, and I am figuring out the flow.

Best Regards
Guomin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeremy
> Linton
> Sent: Tuesday, April 28, 2020 6:16 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; ard.biesheuvel@arm.com; Jeremy
> Linton <jeremy.linton@arm.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild
> descriptor table
> 
> During port reset, the device descriptors should be checked
> before attempting to set an endpoint configuration.
> 
> In particular this fixes a crash due to
> ASSERT(TrsRing != NULL) in XhcSyncTrsRing(). That crash
> happens during error recovery on devices attached to XHCI
> hosts. This is because the port disable clears and deallocats
> all the EP data structures. When the port is reconfigured
> without first requesting the EP descriptors,
> XhcSetConfigCmd[64]() is not being called because the
> NumConfigurations remains 0.
> 
> We could attempt to rebuild the EP descriptions directly
> from the XHCI driver. OTOH, its probably good practice to
> assure the device description is what we expect from within
> the core USB subsystem during reset.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index 4b4915c019..17bb691bf8 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -874,6 +874,14 @@ UsbIoPortReset (
>    // is in CONFIGURED state.
> 
>    //
> 
>    if (Dev->ActiveConfig != NULL) {
> 
> +    Status = UsbBuildDescTable (Dev);
> 
> +
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      DEBUG ((EFI_D_ERROR, "UsbIoPortReset: failed to build descriptor table
> for %d - %r\n",
> 
> +                 Dev->Address, Status));
> 
> +      goto ON_EXIT;
> 
> +    }
> 
> +
> 
>      Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
> 
> 
> 
>      if (EFI_ERROR (Status)) {
> 
> --
> 2.24.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#58193): https://edk2.groups.io/g/devel/message/58193
> Mute This Topic: https://groups.io/mt/73315690/4399222
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [guomin.jiang@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[Jeremy Linton]

Hi!

On 4/27/20 7:15 PM, Jiang, Guomin wrote:
> Hi Jeremy,
> 
> You can refer https://edk2.groups.io/g/devel/message/58125 for discussion about this solution.

Oh fun, odd how a bug can exist in a code base for years and then this 
happens.. I will move the discussion there.

Thanks,

> 
> Two issue I found:
> 1. Memory leakage may occur if doing so and I am investigating it.

It seems our solutions differ a bit?

> 2. It test pass with OVMF but fail in real platform, and I am figuring out the flow.

Hmm, I've been seeing this on a RPI with an attached USB3 hub and 5 bay 
USB JBOD. (there is another problem but the reset crash is keeps the 
machine from booting).




> 
> Best Regards
> Guomin
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeremy
>> Linton
>> Sent: Tuesday, April 28, 2020 6:16 AM
>> To: devel@edk2.groups.io
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>;
>> Wang, Jian J <jian.j.wang@intel.com>; ard.biesheuvel@arm.com; Jeremy
>> Linton <jeremy.linton@arm.com>
>> Subject: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild
>> descriptor table
>>
>> During port reset, the device descriptors should be checked
>> before attempting to set an endpoint configuration.
>>
>> In particular this fixes a crash due to
>> ASSERT(TrsRing != NULL) in XhcSyncTrsRing(). That crash
>> happens during error recovery on devices attached to XHCI
>> hosts. This is because the port disable clears and deallocats
>> all the EP data structures. When the port is reconfigured
>> without first requesting the EP descriptors,
>> XhcSetConfigCmd[64]() is not being called because the
>> NumConfigurations remains 0.
>>
>> We could attempt to rebuild the EP descriptions directly
>> from the XHCI driver. OTOH, its probably good practice to
>> assure the device description is what we expect from within
>> the core USB subsystem during reset.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
>> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
>> index 4b4915c019..17bb691bf8 100644
>> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
>> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
>> @@ -874,6 +874,14 @@ UsbIoPortReset (
>>     // is in CONFIGURED state.
>>
>>     //
>>
>>     if (Dev->ActiveConfig != NULL) {
>>
>> +    Status = UsbBuildDescTable (Dev);
>>
>> +
>>
>> +    if (EFI_ERROR (Status)) {
>>
>> +      DEBUG ((EFI_D_ERROR, "UsbIoPortReset: failed to build descriptor table
>> for %d - %r\n",
>>
>> +                 Dev->Address, Status));
>>
>> +      goto ON_EXIT;
>>
>> +    }
>>
>> +
>>
>>       Status = UsbSetConfig (Dev, Dev->ActiveConfig->Desc.ConfigurationValue);
>>
>>
>>
>>       if (EFI_ERROR (Status)) {
>>
>> --
>> 2.24.1
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#58193): https://edk2.groups.io/g/devel/message/58193
>> Mute This Topic: https://groups.io/mt/73315690/4399222
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [guomin.jiang@intel.com]
>> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset rebuild descriptor table

[Guomin Jiang]

Hi Jeremy,

Let move the discussion into https://edk2.groups.io/g/devel/message/58125, and I will add history in that message.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeremy
> Linton
> Sent: Tuesday, April 28, 2020 8:44 AM
> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; ard.biesheuvel@arm.com
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset
> rebuild descriptor table
> 
> Hi!
> 
> On 4/27/20 7:15 PM, Jiang, Guomin wrote:
> > Hi Jeremy,
> >
> > You can refer https://edk2.groups.io/g/devel/message/58125 for discussion
> about this solution.
> 
> Oh fun, odd how a bug can exist in a code base for years and then this
> happens.. I will move the discussion there.
> 
> Thanks,
> 
> >
> > Two issue I found:
> > 1. Memory leakage may occur if doing so and I am investigating it.
> 
> It seems our solutions differ a bit?

Memory leakage will happened when invoke UsbBuildDescTable(), because the old allocated buffer haven't been freed but allocated new
> 
> > 2. It test pass with OVMF but fail in real platform, and I am figuring out the
> flow.
> 
> Hmm, I've been seeing this on a RPI with an attached USB3 hub and 5 bay USB
> JBOD. (there is another problem but the reset crash is keeps the machine
> from booting).
> 
> 
> 
> 
> >
> > Best Regards
> > Guomin
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Jeremy
> >> Linton
> >> Sent: Tuesday, April 28, 2020 6:16 AM
> >> To: devel@edk2.groups.io
> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> >> Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng
> >> <feng.tian@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> >> ard.biesheuvel@arm.com; Jeremy Linton <jeremy.linton@arm.com>
> >> Subject: [edk2-devel] [PATCH] MdeModulePkg/UsbBusDxe: On reset
> >> rebuild descriptor table
> >>
> >> During port reset, the device descriptors should be checked before
> >> attempting to set an endpoint configuration.
> >>
> >> In particular this fixes a crash due to ASSERT(TrsRing != NULL) in
> >> XhcSyncTrsRing(). That crash happens during error recovery on devices
> >> attached to XHCI hosts. This is because the port disable clears and
> >> deallocats all the EP data structures. When the port is reconfigured
> >> without first requesting the EP descriptors,
> >> XhcSetConfigCmd[64]() is not being called because the
> >> NumConfigurations remains 0.
> >>
> >> We could attempt to rebuild the EP descriptions directly from the
> >> XHCI driver. OTOH, its probably good practice to assure the device
> >> description is what we expect from within the core USB subsystem
> >> during reset.
> >>
> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >> ---
> >>   MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> index 4b4915c019..17bb691bf8 100644
> >> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> >> @@ -874,6 +874,14 @@ UsbIoPortReset (
> >>     // is in CONFIGURED state.
> >>
> >>     //
> >>
> >>     if (Dev->ActiveConfig != NULL) {
> >>
> >> +    Status = UsbBuildDescTable (Dev);
> >>
> >> +
> >>
> >> +    if (EFI_ERROR (Status)) {
> >>
> >> +      DEBUG ((EFI_D_ERROR, "UsbIoPortReset: failed to build
> >> + descriptor table
> >> for %d - %r\n",
> >>
> >> +                 Dev->Address, Status));
> >>
> >> +      goto ON_EXIT;
> >>
> >> +    }
> >>
> >> +
> >>
> >>       Status = UsbSetConfig (Dev,
> >> Dev->ActiveConfig->Desc.ConfigurationValue);
> >>
> >>
> >>
> >>       if (EFI_ERROR (Status)) {
> >>
> >> --
> >> 2.24.1
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >>
> >> View/Reply Online (#58193):
> >> https://edk2.groups.io/g/devel/message/58193
> >> Mute This Topic: https://groups.io/mt/73315690/4399222
> >> Group Owner: devel+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >> [guomin.jiang@intel.com] -=-=-=-=-=-=
> >
> 
> 
>