[PATCH 2/4] MdeModulePkg/XhciDxe/Xhci: Don't check for invalid PSIV

[PATCH 2/4] MdeModulePkg/XhciDxe/Xhci: Don't check for invalid PSIV

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

PSID matching relies on comparing the PSIV against the PortSpeed
value. This patch stops edk2 from checking for a PSIV of 0, as it
is not valid; this reduces the number of register access by
approximately 6 per second.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: If15c55ab66d2e7faa832ce8576d2e5b47157cc9a
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 44 ++++++++++++++++-------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index c05431ff30..51ae57db21 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -371,6 +371,7 @@ XhcGetRootHubPortStatus (
   UINT32             TotalPort;
   UINTN              Index;
   UINTN              MapSize;
+  UINTN              PortSpeed;
   EFI_STATUS         Status;
   USB_DEV_ROUTE      ParentRouteChart;
   EFI_TPL            OldTpl;
@@ -397,32 +398,37 @@ XhcGetRootHubPortStatus (
 
   State = XhcReadOpReg (Xhc, Offset);
 
+  PortSpeed = (State & XHC_PORTSC_PS) >> 10;
+
   //
   // According to XHCI 1.1 spec November 2017,
   // Section 7.2 xHCI Support Protocol Capability
   //
-  PortStatus->PortStatus = XhcCheckUsbPortSpeedUsedPsic (Xhc, ((State & XHC_PORTSC_PS) >> 10));
-  if (PortStatus->PortStatus == 0) {
-    //
-    // According to XHCI 1.1 spec November 2017,
-    // bit 10~13 of the root port status register identifies the speed of the attached device.
-    //
-    switch ((State & XHC_PORTSC_PS) >> 10) {
-      case 2:
-        PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED;
-        break;
+  if (PortSpeed > 0) {
+    PortStatus->PortStatus = XhcCheckUsbPortSpeedUsedPsic (Xhc, PortSpeed);
+    // If no match found in ext cap reg, fall back to PORTSC
+    if (PortStatus->PortStatus == 0) {
+      //
+      // According to XHCI 1.1 spec November 2017,
+      // bit 10~13 of the root port status register identifies the speed of the attached device.
+      //
+      switch (PortSpeed) {
+        case 2:
+          PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED;
+          break;
 
-      case 3:
-        PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED;
-        break;
+        case 3:
+          PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED;
+          break;
 
-      case 4:
-      case 5:
-        PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED;
-        break;
+        case 4:
+        case 5:
+          PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED;
+          break;
 
-      default:
-        break;
+        default:
+          break;
+      }
     }
   }
 
-- 
2.37.2

[PATCH 1/4] MdeModulePkg/XhciDxe/XhciReg: Handle incorrect PSIV indices

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

On some platforms, including Sky Lake and Kaby Lake, the PSIV (Protocol
Speed ID Value) indices are shared between Protocol Speed ID DWORD' in
the extended capabilities registers for both USB2 (Full Speed) and USB3
(Super Speed).

An example can be found below:

    XhcCheckUsbPortSpeedUsedPsic: checking for USB2 ext caps
    XhciPsivGetPsid: found 3 PSID entries
    XhciPsivGetPsid: looking for port speed 1
    XhciPsivGetPsid: PSIV 1 PSIE 2 PLT 0 PSIM 12
    XhciPsivGetPsid: PSIV 2 PSIE 1 PLT 0 PSIM 1500
    XhciPsivGetPsid: PSIV 3 PSIE 2 PLT 0 PSIM 480
    XhcCheckUsbPortSpeedUsedPsic: checking for USB3 ext caps
    XhciPsivGetPsid: found 3 PSID entries
    XhciPsivGetPsid: looking for port speed 1
    XhciPsivGetPsid: PSIV 1 PSIE 3 PLT 0 PSIM 5
    XhciPsivGetPsid: PSIV 2 PSIE 3 PLT 0 PSIM 10
    XhciPsivGetPsid: PSIV 34 PSIE 2 PLT 0 PSIM 1248

The result is edk2 detecting USB2 devices as USB3 devices, which
consequently causes enumeration to fail.

To avoid incorrect detection, check the extended capability registers
for USB2 before USB3. If edk2 finds a match for a USB 2 device, don't
check for USB 3.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: I5bcf32105ce85fda95b4ba98a5e420e8f522374c
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 36 +++++++++++++++-----------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h |  1 +
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 2b4a4b2444..c992323443 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -698,25 +698,11 @@ XhcCheckUsbPortSpeedUsedPsic (
   SpField.Dword = 0;
   UsbSpeedIdMap = 0;
 
-  //
-  // Check xHCI Supported Protocol Capability, find the PSIV field to match
-  // PortSpeed definition when the Major Revision is 03h.
-  //
-  if (Xhc->Usb3SupOffset != 0xFFFFFFFF) {
-    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset, PortSpeed);
-    if (SpField.Dword != 0) {
-      //
-      // Found the corresponding PORTSC value in PSIV field of USB3 offset.
-      //
-      UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;
-    }
-  }
-
   //
   // Check xHCI Supported Protocol Capability, find the PSIV field to match
   // PortSpeed definition when the Major Revision is 02h.
   //
-  if ((UsbSpeedIdMap == 0) && (Xhc->Usb2SupOffset != 0xFFFFFFFF)) {
+  if (Xhc->Usb2SupOffset != 0xFFFFFFFF) {
     SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb2SupOffset, PortSpeed);
     if (SpField.Dword != 0) {
       //
@@ -733,6 +719,12 @@ XhcCheckUsbPortSpeedUsedPsic (
           // PSIM shows as default High-speed protocol, apply to High-speed mapping
           //
           UsbSpeedIdMap = USB_PORT_STAT_HIGH_SPEED;
+        } else if (SpField.Data.Psim == XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM) {
+          //
+          // PSIM shows as default Full-speed protocol, return 0
+          // to ensure no port status set
+          //
+          return 0;
         }
       } else if (SpField.Data.Psie == 1) {
         //
@@ -750,6 +742,20 @@ XhcCheckUsbPortSpeedUsedPsic (
     }
   }
 
+  //
+  // Check xHCI Supported Protocol Capability, find the PSIV field to match
+  // PortSpeed definition when the Major Revision is 03h.
+  //
+  if ((UsbSpeedIdMap == 0) && (Xhc->Usb3SupOffset != 0xFFFFFFFF)) {
+    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset, PortSpeed);
+    if (SpField.Dword != 0) {
+      //
+      // Found the corresponding PORTSC value in PSIV field of USB3 offset.
+      //
+      UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;
+    }
+  }
+
   return UsbSpeedIdMap;
 }
 
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
index 5fe2ba4f0e..74ac6297ba 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
@@ -85,6 +85,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define XHC_SUPPORTED_PROTOCOL_DW2_OFFSET               0x08
 #define XHC_SUPPORTED_PROTOCOL_PSI_OFFSET               0x10
 #define XHC_SUPPORTED_PROTOCOL_USB2_HIGH_SPEED_PSIM     480
+#define XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM     12
 #define XHC_SUPPORTED_PROTOCOL_USB2_LOW_SPEED_PSIM      1500
 
 #pragma pack (1)
-- 
2.37.2

[PATCH 2/4] MdeModulePkg/XhciDxe/Xhci: Don't check for invalid PSIV

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

PSID matching relies on comparing the PSIV against the PortSpeed
value. This patch stops edk2 from checking for a PSIV of 0, as it
is not valid; this reduces the number of register access by
approximately 6 per second.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: If15c55ab66d2e7faa832ce8576d2e5b47157cc9a
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 44 ++++++++++++++++-------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index c05431ff30..62535cad54 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -371,6 +371,7 @@ XhcGetRootHubPortStatus (
   UINT32             TotalPort;
   UINTN              Index;
   UINTN              MapSize;
+  UINT8              PortSpeed;
   EFI_STATUS         Status;
   USB_DEV_ROUTE      ParentRouteChart;
   EFI_TPL            OldTpl;
@@ -397,32 +398,37 @@ XhcGetRootHubPortStatus (
 
   State = XhcReadOpReg (Xhc, Offset);
 
+  PortSpeed = (State & XHC_PORTSC_PS) >> 10;
+
   //
   // According to XHCI 1.1 spec November 2017,
   // Section 7.2 xHCI Support Protocol Capability
   //
-  PortStatus->PortStatus = XhcCheckUsbPortSpeedUsedPsic (Xhc, ((State & XHC_PORTSC_PS) >> 10));
-  if (PortStatus->PortStatus == 0) {
-    //
-    // According to XHCI 1.1 spec November 2017,
-    // bit 10~13 of the root port status register identifies the speed of the attached device.
-    //
-    switch ((State & XHC_PORTSC_PS) >> 10) {
-      case 2:
-        PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED;
-        break;
+  if (PortSpeed > 0) {
+    PortStatus->PortStatus = XhcCheckUsbPortSpeedUsedPsic (Xhc, PortSpeed);
+    // If no match found in ext cap reg, fall back to PORTSC
+    if (PortStatus->PortStatus == 0) {
+      //
+      // According to XHCI 1.1 spec November 2017,
+      // bit 10~13 of the root port status register identifies the speed of the attached device.
+      //
+      switch (PortSpeed) {
+        case 2:
+          PortStatus->PortStatus |= USB_PORT_STAT_LOW_SPEED;
+          break;
 
-      case 3:
-        PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED;
-        break;
+        case 3:
+          PortStatus->PortStatus |= USB_PORT_STAT_HIGH_SPEED;
+          break;
 
-      case 4:
-      case 5:
-        PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED;
-        break;
+        case 4:
+        case 5:
+          PortStatus->PortStatus |= USB_PORT_STAT_SUPER_SPEED;
+          break;
 
-      default:
-        break;
+        default:
+          break;
+      }
     }
   }
 
-- 
2.37.2

[PATCH 3/4] MdeModulePkg/BmBoot: Skip removable media if it is not present

[Sean Rhodes]

From: Matt DeVillier <matt.devillier@gmail.com>

Only enumerate devices that have media present.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: I78a0b8be3e2f33edce2d43bbdd7670e6174d0ff8
---
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 962892d38f..bde22fa659 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2218,6 +2218,15 @@ BmEnumerateBootOptions (
         continue;
       }
 
+      //
+      // Skip removable media if not present
+      //
+      if ((BlkIo->Media->RemovableMedia == TRUE) &&
+          (BlkIo->Media->MediaPresent == FALSE))
+      {
+        continue;
+      }
+
       Description = BmGetBootDescription (Handles[Index]);
       BootOptions = ReallocatePool (
                       sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount),
-- 
2.37.2

[PATCH 4/4] MdeModulePkg/UsbBusDxe: Adjust the MaxPacketLength to real world values

[Sean Rhodes]

Adjusts the requirements for the MaxPacketLength to match what is seen on
real world devices that do not follow the USB specification.

This fixes enumeration on the multiple USB 3 devices made by SanDisk,
Integral, Kingston and other generic brands.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index 62535cad54..043b7d4cea 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -906,19 +906,16 @@ XhcControlTransfer (
     return EFI_INVALID_PARAMETER;
   }
 
-  if ((MaximumPacketLength != 8)  && (MaximumPacketLength != 16) &&
-      (MaximumPacketLength != 32) && (MaximumPacketLength != 64) &&
-      (MaximumPacketLength != 512)
-      )
-  {
+  // Check for valid maximum packet size
+  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength > 1024)) {
     return EFI_INVALID_PARAMETER;
   }
 
-  if ((DeviceSpeed == EFI_USB_SPEED_LOW) && (MaximumPacketLength != 8)) {
+  if ((DeviceSpeed == EFI_USB_SPEED_HIGH) && (MaximumPacketLength > 512)) {
     return EFI_INVALID_PARAMETER;
   }
 
-  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength != 512)) {
+  if ((DeviceSpeed == EFI_USB_SPEED_FULL) && (MaximumPacketLength > 64)) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.37.2

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/UsbBusDxe: Adjust the MaxPacketLength to real world values

[Sean Rhodes]

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

Link to PR passing CI - https://github.com/tianocore/edk2/pull/3353

On Mon, 5 Dec 2022 at 09:18, Sean Rhodes via groups.io <sean=
starlabs.systems@groups.io> wrote:

> Adjusts the requirements for the MaxPacketLength to match what is seen on
> real world devices that do not follow the USB specification.
>
> This fixes enumeration on the multiple USB 3 devices made by SanDisk,
> Integral, Kingston and other generic brands.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Sean Rhodes <sean@starlabs.systems>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 62535cad54..043b7d4cea 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -906,19 +906,16 @@ XhcControlTransfer (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  if ((MaximumPacketLength != 8)  && (MaximumPacketLength != 16) &&
> -      (MaximumPacketLength != 32) && (MaximumPacketLength != 64) &&
> -      (MaximumPacketLength != 512)
> -      )
> -  {
> +  // Check for valid maximum packet size
> +  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength >
> 1024)) {
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  if ((DeviceSpeed == EFI_USB_SPEED_LOW) && (MaximumPacketLength != 8)) {
> +  if ((DeviceSpeed == EFI_USB_SPEED_HIGH) && (MaximumPacketLength > 512))
> {
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength !=
> 512)) {
> +  if ((DeviceSpeed == EFI_USB_SPEED_FULL) && (MaximumPacketLength > 64)) {
>      return EFI_INVALID_PARAMETER;
>    }
>
> --
> 2.37.2
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96951): https://edk2.groups.io/g/devel/message/96951
> Mute This Topic: https://groups.io/mt/95465401/6718866
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [sean@starlabs.systems]
> ------------
>
>
>

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

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/UsbBusDxe: Adjust the MaxPacketLength to real world values

[Wu, Hao A]

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

I have concern for this patch.



It will make the implementation of XhcControlTransfer() no longer following the UEFI specification requirements on the EFI_USB2_HC_PROTOCOL.ControlTransfer() service:



Section 17.1.7 of UEFI spec Release 2.10:

EFI_INVALID_PARAMETER is returned if one of the following conditions is satisfied:

...

* MaximumPacketLength is not valid. If DeviceSpeed is EFI_USB_SPEED_LOW, then MaximumPacketLength

  must be 8. If DeviceSpeed is EFI_USB_SPEED_FULL or EFI_USB_SPEED_HIGH, then MaximumPac-

  ketLength must be 8, 16, 32, or 64. If DeviceSpeed is EFI_USB_SPEED_SUPER, then MaximumPacketLength

  must be 512.



My take is that it will need a UEFI spec update to integrate this proposed change.

Sorry Liming, could you help on the process of raising opens with regard to the UEFI specification? Thanks in advance.



Best Regards,

Hao Wu



> -----Original Message-----

> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean

> Rhodes

> Sent: Monday, December 5, 2022 5:18 PM

> To: devel@edk2.groups.io

> Cc: Rhodes, Sean <sean@starlabs.systems>; Wu, Hao A

> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>

> Subject: [edk2-devel] [PATCH 4/4] MdeModulePkg/UsbBusDxe: Adjust the

> MaxPacketLength to real world values

>

> Adjusts the requirements for the MaxPacketLength to match what is seen on

> real world devices that do not follow the USB specification.

>

> This fixes enumeration on the multiple USB 3 devices made by SanDisk,

> Integral, Kingston and other generic brands.

>

> Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>

> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>

> Signed-off-by: Sean Rhodes <sean@starlabs.systems<mailto:sean@starlabs.systems>>

> ---

>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 11 ++++-------

>  1 file changed, 4 insertions(+), 7 deletions(-)

>

> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

> index 62535cad54..043b7d4cea 100644

> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c

> @@ -906,19 +906,16 @@ XhcControlTransfer (

>      return EFI_INVALID_PARAMETER;

>

>    }

>

>

>

> -  if ((MaximumPacketLength != 8)  && (MaximumPacketLength != 16) &&

>

> -      (MaximumPacketLength != 32) && (MaximumPacketLength != 64) &&

>

> -      (MaximumPacketLength != 512)

>

> -      )

>

> -  {

>

> +  // Check for valid maximum packet size

>

> +  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength >

> 1024)) {

>

>      return EFI_INVALID_PARAMETER;

>

>    }

>

>

>

> -  if ((DeviceSpeed == EFI_USB_SPEED_LOW) && (MaximumPacketLength !=

> 8)) {

>

> +  if ((DeviceSpeed == EFI_USB_SPEED_HIGH) && (MaximumPacketLength >

> 512)) {

>

>      return EFI_INVALID_PARAMETER;

>

>    }

>

>

>

> -  if ((DeviceSpeed == EFI_USB_SPEED_SUPER) && (MaximumPacketLength !=

> 512)) {

>

> +  if ((DeviceSpeed == EFI_USB_SPEED_FULL) && (MaximumPacketLength >

> 64)) {

>

>      return EFI_INVALID_PARAMETER;

>

>    }

>

>

>

> --

> 2.37.2

>

>

>

> -=-=-=-=-=-=

> Groups.io Links: You receive all messages sent to this group.

> View/Reply Online (#96951): https://edk2.groups.io/g/devel/message/96951

> Mute This Topic: https://groups.io/mt/95465401/1768737

> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>

> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]

> -=-=-=-=-=-=

>



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

Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/BmBoot: Skip removable media if it is not present

[Wu, Hao A]

Hello,

Could you help to co-work with BDS modules reviewers for reviewing this patch?

py BaseTools/Scripts/GetMaintainer.py 294ce61c3b1a63c89bd16a78f8e8b40e889dac5a
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
  Jian J Wang <jian.j.wang@intel.com>
  Liming Gao <gaoliming@byosoft.com.cn>
  Zhichao Gao <zhichao.gao@intel.com>
  Ray Ni <ray.ni@intel.com>
  devel@edk2.groups.io

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean
> Rhodes
> Sent: Monday, December 5, 2022 5:18 PM
> To: devel@edk2.groups.io
> Cc: Matt DeVillier <matt.devillier@gmail.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean
> <sean@starlabs.systems>
> Subject: [edk2-devel] [PATCH 3/4] MdeModulePkg/BmBoot: Skip removable
> media if it is not present
> 
> From: Matt DeVillier <matt.devillier@gmail.com>
> 
> Only enumerate devices that have media present.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Sean Rhodes <sean@starlabs.systems>
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> Change-Id: I78a0b8be3e2f33edce2d43bbdd7670e6174d0ff8
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> index 962892d38f..bde22fa659 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> @@ -2218,6 +2218,15 @@ BmEnumerateBootOptions (
>          continue;
> 
>        }
> 
> 
> 
> +      //
> 
> +      // Skip removable media if not present
> 
> +      //
> 
> +      if ((BlkIo->Media->RemovableMedia == TRUE) &&
> 
> +          (BlkIo->Media->MediaPresent == FALSE))
> 
> +      {
> 
> +        continue;
> 
> +      }
> 
> +
> 
>        Description = BmGetBootDescription (Handles[Index]);
> 
>        BootOptions = ReallocatePool (
> 
>                        sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) *
> (*BootOptionCount),
> 
> --
> 2.37.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96952): https://edk2.groups.io/g/devel/message/96952
> Mute This Topic: https://groups.io/mt/95465402/1768737
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH 1/4] MdeModulePkg/XhciDxe/XhciReg: Handle incorrect PSIV indices

[ian.chiu]

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

Hi Sean, Matt DeVillier,

I checked the patch 0001-0004, we are find and agree with 0002-0004.
For the patch 0001-MdeModulePkg-XhciDxe-XhciReg-Handle-incorrect-PSIV-indices.
The case you mention below with same PSIV in USB3/USB2.

We consider if there exist a case that actually want to go with USB3 speed, 
then will fail when data transmit. Since USB3 protocol is different than USB2.
Otherwise we may need to fix it again, once issue coming out.

We think about a solution to using the "Compatible Port Count" & "Compatible Port Offset" to ensure the port is supported this protocol or not.
Would you able to dump the xHCI Supported Protocol Capability raw data and check if this solution works with you case.

Attach sample code snippet and data dump from my side.

Thanks,
Ian Chiu
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
Sent: Monday, December 5, 2022 5:18 PM
To: devel@edk2.groups.io
Cc: Matt DeVillier <matt.devillier@gmail.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>
Subject: [edk2-devel] [PATCH 1/4] MdeModulePkg/XhciDxe/XhciReg: Handle incorrect PSIV indices

From: Matt DeVillier <matt.devillier@gmail.com>

On some platforms, including Sky Lake and Kaby Lake, the PSIV (Protocol Speed ID Value) indices are shared between Protocol Speed ID DWORD' in the extended capabilities registers for both USB2 (Full Speed) and USB3 (Super Speed).

An example can be found below:

    XhcCheckUsbPortSpeedUsedPsic: checking for USB2 ext caps
    XhciPsivGetPsid: found 3 PSID entries
    XhciPsivGetPsid: looking for port speed 1
    XhciPsivGetPsid: PSIV 1 PSIE 2 PLT 0 PSIM 12
    XhciPsivGetPsid: PSIV 2 PSIE 1 PLT 0 PSIM 1500
    XhciPsivGetPsid: PSIV 3 PSIE 2 PLT 0 PSIM 480
    XhcCheckUsbPortSpeedUsedPsic: checking for USB3 ext caps
    XhciPsivGetPsid: found 3 PSID entries
    XhciPsivGetPsid: looking for port speed 1
    XhciPsivGetPsid: PSIV 1 PSIE 3 PLT 0 PSIM 5
    XhciPsivGetPsid: PSIV 2 PSIE 3 PLT 0 PSIM 10
    XhciPsivGetPsid: PSIV 34 PSIE 2 PLT 0 PSIM 1248

The result is edk2 detecting USB2 devices as USB3 devices, which consequently causes enumeration to fail.

To avoid incorrect detection, check the extended capability registers for USB2 before USB3. If edk2 finds a match for a USB 2 device, don't check for USB 3.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Reviewed-by: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
Change-Id: I5bcf32105ce85fda95b4ba98a5e420e8f522374c
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 36 +++++++++++++++-----------  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h |  1 +
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
index 2b4a4b2444..c992323443 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -698,25 +698,11 @@ XhcCheckUsbPortSpeedUsedPsic (
   SpField.Dword = 0;   UsbSpeedIdMap = 0; -  //-  // Check xHCI Supported Protocol Capability, find the PSIV field to match-  // PortSpeed definition when the Major Revision is 03h.-  //-  if (Xhc->Usb3SupOffset != 0xFFFFFFFF) {-    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset, PortSpeed);-    if (SpField.Dword != 0) {-      //-      // Found the corresponding PORTSC value in PSIV field of USB3 offset.-      //-      UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;-    }-  }-   //   // Check xHCI Supported Protocol Capability, find the PSIV field to match   // PortSpeed definition when the Major Revision is 02h.   //-  if ((UsbSpeedIdMap == 0) && (Xhc->Usb2SupOffset != 0xFFFFFFFF)) {+  if (Xhc->Usb2SupOffset != 0xFFFFFFFF) {     SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb2SupOffset, PortSpeed);     if (SpField.Dword != 0) {       //@@ -733,6 +719,12 @@ XhcCheckUsbPortSpeedUsedPsic (
           // PSIM shows as default High-speed protocol, apply to High-speed mapping           //           UsbSpeedIdMap = USB_PORT_STAT_HIGH_SPEED;+        } else if (SpField.Data.Psim == XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM) {+          //+          // PSIM shows as default Full-speed protocol, return 0+          // to ensure no port status set+          //+          return 0;         }       } else if (SpField.Data.Psie == 1) {         //@@ -750,6 +742,20 @@ XhcCheckUsbPortSpeedUsedPsic (
     }   } +  //+  // Check xHCI Supported Protocol Capability, find the PSIV field to match+  // PortSpeed definition when the Major Revision is 03h.+  //+  if ((UsbSpeedIdMap == 0) && (Xhc->Usb3SupOffset != 0xFFFFFFFF)) {+    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset, PortSpeed);+    if (SpField.Dword != 0) {+      //+      // Found the corresponding PORTSC value in PSIV field of USB3 offset.+      //+      UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;+    }+  }+   return UsbSpeedIdMap; } diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
index 5fe2ba4f0e..74ac6297ba 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
@@ -85,6 +85,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #define XHC_SUPPORTED_PROTOCOL_DW2_OFFSET               0x08 #define XHC_SUPPORTED_PROTOCOL_PSI_OFFSET               0x10 #define XHC_SUPPORTED_PROTOCOL_USB2_HIGH_SPEED_PSIM     480+#define XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM     12 #define XHC_SUPPORTED_PROTOCOL_USB2_LOW_SPEED_PSIM      1500  #pragma pack (1)-- 
2.37.2



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96950): https://edk2.groups.io/g/devel/message/96950
Mute This Topic: https://groups.io/mt/95391831/5427408
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ian.chiu@intel.com] -=-=-=-=-=-=



[-- Attachment #2: CompatiblePortOffset&Count.zip --]
[-- Type: application/x-zip-compressed, Size: 1389859 bytes --]

Re: [edk2-devel] [PATCH 1/4] MdeModulePkg/XhciDxe/XhciReg: Handle incorrect PSIV indices

[Sean Rhodes]

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

Thank you :)

On Thu, 8 Dec 2022 at 07:06, Chiu, Ian <ian.chiu@intel.com> wrote:

> Hi Sean, Matt DeVillier,
>
> I checked the patch 0001-0004, we are find and agree with 0002-0004.
> For the patch
> 0001-MdeModulePkg-XhciDxe-XhciReg-Handle-incorrect-PSIV-indices.
> The case you mention below with same PSIV in USB3/USB2.
>
> We consider if there exist a case that actually want to go with USB3
> speed,
> then will fail when data transmit. Since USB3 protocol is different than
> USB2.
> Otherwise we may need to fix it again, once issue coming out.
>
> We think about a solution to using the "Compatible Port Count" &
> "Compatible Port Offset" to ensure the port is supported this protocol or
> not.
> Would you able to dump the xHCI Supported Protocol Capability raw data and
> check if this solution works with you case.
>
> Attach sample code snippet and data dump from my side.
>
> Thanks,
> Ian Chiu
>
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sean Rhodes
> Sent: Monday, December 5, 2022 5:18 PM
> To: devel@edk2.groups.io
> Cc: Matt DeVillier <matt.devillier@gmail.com>; Wu, Hao A <
> hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean
> <sean@starlabs.systems>
> Subject: [edk2-devel] [PATCH 1/4] MdeModulePkg/XhciDxe/XhciReg: Handle
> incorrect PSIV indices
>
> From: Matt DeVillier <matt.devillier@gmail.com>
>
> On some platforms, including Sky Lake and Kaby Lake, the PSIV (Protocol
> Speed ID Value) indices are shared between Protocol Speed ID DWORD' in the
> extended capabilities registers for both USB2 (Full Speed) and USB3 (Super
> Speed).
>
> An example can be found below:
>
>     XhcCheckUsbPortSpeedUsedPsic: checking for USB2 ext caps
>     XhciPsivGetPsid: found 3 PSID entries
>     XhciPsivGetPsid: looking for port speed 1
>     XhciPsivGetPsid: PSIV 1 PSIE 2 PLT 0 PSIM 12
>     XhciPsivGetPsid: PSIV 2 PSIE 1 PLT 0 PSIM 1500
>     XhciPsivGetPsid: PSIV 3 PSIE 2 PLT 0 PSIM 480
>     XhcCheckUsbPortSpeedUsedPsic: checking for USB3 ext caps
>     XhciPsivGetPsid: found 3 PSID entries
>     XhciPsivGetPsid: looking for port speed 1
>     XhciPsivGetPsid: PSIV 1 PSIE 3 PLT 0 PSIM 5
>     XhciPsivGetPsid: PSIV 2 PSIE 3 PLT 0 PSIM 10
>     XhciPsivGetPsid: PSIV 34 PSIE 2 PLT 0 PSIM 1248
>
> The result is edk2 detecting USB2 devices as USB3 devices, which
> consequently causes enumeration to fail.
>
> To avoid incorrect detection, check the extended capability registers for
> USB2 before USB3. If edk2 finds a match for a USB 2 device, don't check for
> USB 3.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Sean Rhodes <sean@starlabs.systems>
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> Change-Id: I5bcf32105ce85fda95b4ba98a5e420e8f522374c
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 36 +++++++++++++++-----------
> MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h |  1 +
>  2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 2b4a4b2444..c992323443 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -698,25 +698,11 @@ XhcCheckUsbPortSpeedUsedPsic (
>    SpField.Dword = 0;   UsbSpeedIdMap = 0; -  //-  // Check xHCI Supported
> Protocol Capability, find the PSIV field to match-  // PortSpeed definition
> when the Major Revision is 03h.-  //-  if (Xhc->Usb3SupOffset !=
> 0xFFFFFFFF) {-    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset,
> PortSpeed);-    if (SpField.Dword != 0) {-      //-      // Found the
> corresponding PORTSC value in PSIV field of USB3 offset.-      //-
> UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;-    }-  }-   //   // Check xHCI
> Supported Protocol Capability, find the PSIV field to match   // PortSpeed
> definition when the Major Revision is 02h.   //-  if ((UsbSpeedIdMap == 0)
> && (Xhc->Usb2SupOffset != 0xFFFFFFFF)) {+  if (Xhc->Usb2SupOffset !=
> 0xFFFFFFFF) {     SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb2SupOffset,
> PortSpeed);     if (SpField.Dword != 0) {       //@@ -733,6 +719,12 @@
> XhcCheckUsbPortSpeedUsedPsic (
>            // PSIM shows as default High-speed protocol, apply to
> High-speed mapping           //           UsbSpeedIdMap =
> USB_PORT_STAT_HIGH_SPEED;+        } else if (SpField.Data.Psim ==
> XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM) {+          //+          //
> PSIM shows as default Full-speed protocol, return 0+          // to ensure
> no port status set+          //+          return 0;         }       } else
> if (SpField.Data.Psie == 1) {         //@@ -750,6 +742,20 @@
> XhcCheckUsbPortSpeedUsedPsic (
>      }   } +  //+  // Check xHCI Supported Protocol Capability, find the
> PSIV field to match+  // PortSpeed definition when the Major Revision is
> 03h.+  //+  if ((UsbSpeedIdMap == 0) && (Xhc->Usb3SupOffset != 0xFFFFFFFF))
> {+    SpField.Dword = XhciPsivGetPsid (Xhc, Xhc->Usb3SupOffset,
> PortSpeed);+    if (SpField.Dword != 0) {+      //+      // Found the
> corresponding PORTSC value in PSIV field of USB3 offset.+      //+
> UsbSpeedIdMap = USB_PORT_STAT_SUPER_SPEED;+    }+  }+   return
> UsbSpeedIdMap; } diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
> index 5fe2ba4f0e..74ac6297ba 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h
> @@ -85,6 +85,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #define XHC_SUPPORTED_PROTOCOL_DW2_OFFSET               0x08 #define
> XHC_SUPPORTED_PROTOCOL_PSI_OFFSET               0x10 #define
> XHC_SUPPORTED_PROTOCOL_USB2_HIGH_SPEED_PSIM     480+#define
> XHC_SUPPORTED_PROTOCOL_USB2_FULL_SPEED_PSIM     12 #define
> XHC_SUPPORTED_PROTOCOL_USB2_LOW_SPEED_PSIM      1500  #pragma pack (1)--
> 2.37.2
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96950): https://edk2.groups.io/g/devel/message/96950
> Mute This Topic: https://groups.io/mt/95391831/5427408
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ian.chiu@intel.com]
> -=-=-=-=-=-=
>
>
>
>
> 
>
>
>

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