[edk2] [PATCH 0/2] [Bug & Workaround] Fix USB stack when calling ConnectController with a USB device Device Path

[Olivier Martin]

The current USB stack is broken when you call BS.ConnectController()
with a USB device Device Path for instance like
PciRoot(0)/PCI(31,2)/USB(1,0)/USB(3,0)
The issue is not new and has probably always existed.

What the UEFI specification says about EFI_DRIVER_BINDING_PROTOCOL.Start():

If the driver specified by This is a bus driver that is capable
of creating one child handle at a time and RemainingDevicePath
is not NULL and does not begin with the End of Device Path node,
then an attempt is made to create the device handle for the child
device specified by RemainingDevicePath. Depending on the bus type,
all of the child devices may need to be discovered and enumerated,
but at most only the device handle for the one child specified by
RemainingDevicePath shall be created.

Source: 10.1 EFI Driver Binding Protocol (UEFI spec 2.5)

With the current USB stack implementation, BS.ConnectController()
would return EFI_SUCCESS without having connecting 'USB(3,0)'.

The reason is the USB stack initialization is based on Timer Event.
The initialization and the creation of USB(3,0) device node would
happen asynchronously.

The reason why the issue has not been highlighted earlier is because
people tend to 'connect' all the drivers when they start the Intel BDS.
These initializations take time. And it is likely all the timer based
initialization are completed by the end of ConnectAllControllers().

The design choice I made for the ARM BDS is to only start the required
devices (ie: the console devices). This USB issue appears straight
away in this use case.

My workaround is to replace gBS->SignalEvent() by direct calls to
the functions that should have replied to these events.
The patch is not perfect. I was told USB Hub are not enumerated
correctly.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Olivier Martin <***@arm.com>

Olivier Martin (2):
MdeModulePkg/Usb: Force the USB initialization to be synchronous
MdeModulePkg/EhciDxe: Do not process a same URB twice

MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 6 ++++++
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +++++++++
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c | 1 +
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h | 3 ++-
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 11 +++++++++--
5 files changed, 27 insertions(+), 3 deletions(-)
mode change 100644 => 100755 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
mode change 100644 => 100755 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c

[Olivier Martin]

After changing the USB stack to make it synchronous at initialization,
we were checking if there was a pending interrupt when the USB interrupt
was initialized for a specific device.
At the first enumeration, all the connected USB devices are initialized.
USB device drivers initialize their USB interrupt and process the completed
URBs. There was no way to check if a URB was in process because before
there was a single periodic task to process these URBs.

Note: this change is only required when using the temporary hack to make
the USB stack synchronous.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Olivier Martin <***@arm.com>

---
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +++++++++
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c | 1 +
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h | 3 ++-
3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index 5594e66..cc6e77e 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -573,6 +573,12 @@ EhcCheckUrbResult (

ASSERT ((Ehc != NULL) && (Urb != NULL) && (Urb->Qh != NULL));

+ // The URB is already being processed, we do not want the callback to be
+ // called twice for the same URB
+ if (Urb->InProgress) {
+ return FALSE;
+ }
+
Finished = TRUE;
Urb->Completed = 0;

@@ -992,6 +998,9 @@ EhcMonitorAsyncRequests (
continue;
}

+ // Mark the URB as 'in-progress' to prevent the URB to be processed twice.
+ Urb->InProgress = TRUE;
+
//
// Flush any PCI posted write transactions from a PCI host
// bridge to system memory.
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
index 6afb327..1ad37d5 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
@@ -600,6 +600,7 @@ EhcCreateUrb (
Urb->DataLen = DataLen;
Urb->Callback = Callback;
Urb->Context = Context;
+ Urb->InProgress = FALSE;

PciIo = Ehc->PciIo;
Urb->Qh = EhcCreateQh (Ehc, &Urb->Ep);
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
index 02e9af8..0c80e76 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
@@ -232,7 +232,8 @@ struct _URB {
// Transaction result
//
UINT32 Result;
- UINTN Completed; // completed data length
+ BOOLEAN InProgress; // defined when the URB is being processed
+ UINTN Completed; // Length of the data being processed
UINT8 DataToggle;
};

[Olivier Martin]

After changing the USB stack to make it synchronous at initialization,
we were checking if there was a pending interrupt when the USB interrupt
was initialized for a specific device.
At the first enumeration, all the connected USB devices are initialized.
USB device drivers initialize their USB interrupt and process the completed
URBs. There was no way to check if a URB was in process because before
there was a single periodic task to process these URBs.

Note: this change is only required when using the temporary hack to make
the USB stack synchronous.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Olivier Martin <***@arm.com>
---
MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 9 +++++++++
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c | 1 +
MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h | 3 ++-
3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index 5594e66..cc6e77e 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -573,6 +573,12 @@ EhcCheckUrbResult (

ASSERT ((Ehc != NULL) && (Urb != NULL) && (Urb->Qh != NULL));

+ // The URB is already being processed, we do not want the callback to be
+ // called twice for the same URB
+ if (Urb->InProgress) {
+ return FALSE;
+ }
+
Finished = TRUE;
Urb->Completed = 0;

@@ -992,6 +998,9 @@ EhcMonitorAsyncRequests (
continue;
}

+ // Mark the URB as 'in-progress' to prevent the URB to be processed twice.
+ Urb->InProgress = TRUE;
+
//
// Flush any PCI posted write transactions from a PCI host
// bridge to system memory.
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
index 6afb327..1ad37d5 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
@@ -600,6 +600,7 @@ EhcCreateUrb (
Urb->DataLen = DataLen;
Urb->Callback = Callback;
Urb->Context = Context;
+ Urb->InProgress = FALSE;

PciIo = Ehc->PciIo;
Urb->Qh = EhcCreateQh (Ehc, &Urb->Ep);
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
index 02e9af8..0c80e76 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
@@ -232,7 +232,8 @@ struct _URB {
// Transaction result
//
UINT32 Result;
- UINTN Completed; // completed data length
+ BOOLEAN InProgress; // defined when the URB is being processed
+ UINTN Completed; // Length of the data being processed
UINT8 DataToggle;
};

[Olivier Martin]

The USB stack uses BS.SignalEvent and Timer event to initialize the USB stack.
It means a USB device initialization might not be completed when returning
from BS.ConnectController().
This behaviour is not compliant with the UEFI spec.

This change is only a _temporary hack_ as it forces the enumeration of the
entire USB bus when the USB Root Hub is initialized which makes this solution
non optimal.

Change-Id: I7d569f0cff9bd7780f9949a6cad93fd1eb669f5b
---
MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 6 ++++++
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 11 +++++++++--
2 files changed, 15 insertions(+), 2 deletions(-)
mode change 100644 => 100755 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
mode change 100644 => 100755 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
old mode 100644
new mode 100755
index 315f2cb..edde95f
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -1095,6 +1095,12 @@ EhcAsyncInterruptTransfer (
EhcLinkQhToPeriod (Ehc, Urb->Qh);
InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);

+ // ARM: Force an asynchonous transfer after waiting an interval
+ // Polling interval is in milliseconds while BS.Stall except
+ // Microseconds.
+ gBS->Stall (PollingInterval * 1000);
+ EhcMonitorAsyncRequests (Ehc->PollTimer, Ehc);
+
ON_EXIT:
Ehc->PciIo->Flush (Ehc->PciIo);
gBS->RestoreTPL (OldTpl);
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
old mode 100644
new mode 100755
index e3752d1..be07a74
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
@@ -668,7 +668,10 @@ UsbOnHubInterrupt (
}

CopyMem (HubIf->ChangeMap, Data, DataLength);
- gBS->SignalEvent (HubIf->HubNotify);
+
+ //ARM: We do not use BS.SignalEvent in order to initialize the new device immediately
+ //gBS->SignalEvent (HubIf->HubNotify);
+ UsbHubEnumeration (HubIf->HubNotify, HubIf);

return EFI_SUCCESS;
}
@@ -1112,7 +1115,11 @@ UsbRootHubInit (
// It should signal the event immediately here, or device detection
// by bus enumeration might be delayed by the timer interval.
//
- gBS->SignalEvent (HubIf->HubNotify);
+
+ //ARM: We invoke the function directly to ensure the enumeration is
+ // done immediately.
+ //gBS->SignalEvent (HubIf->HubNotify);
+ UsbRootHubEnumeration (NULL, HubIf);

Status = gBS->SetTimer (
HubIf->HubNotify,

[Olivier Martin]

The USB stack uses BS.SignalEvent and Timer event to initialize the USB stack.
It means a USB device initialization might not be completed when returning
from BS.ConnectController().
This behaviour is not compliant with the UEFI spec.

This change is only a _temporary hack_ as it forces the enumeration of the
entire USB bus when the USB Root Hub is initialized which makes this solution
non optimal.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Olivier Martin <***@arm.com>
---
MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 6 ++++++
MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 11 +++++++++--
2 files changed, 15 insertions(+), 2 deletions(-)
mode change 100644 => 100755 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
mode change 100644 => 100755 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
old mode 100644
new mode 100755
index 315f2cb..edde95f
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -1095,6 +1095,12 @@ EhcAsyncInterruptTransfer (
EhcLinkQhToPeriod (Ehc, Urb->Qh);
InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);

+ // ARM: Force an asynchonous transfer after waiting an interval
+ // Polling interval is in milliseconds while BS.Stall except
+ // Microseconds.
+ gBS->Stall (PollingInterval * 1000);
+ EhcMonitorAsyncRequests (Ehc->PollTimer, Ehc);
+
ON_EXIT:
Ehc->PciIo->Flush (Ehc->PciIo);
gBS->RestoreTPL (OldTpl);
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
old mode 100644
new mode 100755
index e3752d1..be07a74
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
@@ -668,7 +668,10 @@ UsbOnHubInterrupt (
}

CopyMem (HubIf->ChangeMap, Data, DataLength);
- gBS->SignalEvent (HubIf->HubNotify);
+
+ //ARM: We do not use BS.SignalEvent in order to initialize the new device immediately
+ //gBS->SignalEvent (HubIf->HubNotify);
+ UsbHubEnumeration (HubIf->HubNotify, HubIf);

return EFI_SUCCESS;
}
@@ -1112,7 +1115,11 @@ UsbRootHubInit (
// It should signal the event immediately here, or device detection
// by bus enumeration might be delayed by the timer interval.
//
- gBS->SignalEvent (HubIf->HubNotify);
+
+ //ARM: We invoke the function directly to ensure the enumeration is
+ // done immediately.
+ //gBS->SignalEvent (HubIf->HubNotify);
+ UsbRootHubEnumeration (NULL, HubIf);

Status = gBS->SetTimer (
HubIf->HubNotify,