subject:"\[edk2\] \[patch 2\/3\] MdeModulePkg\/UsbBus\: reduce the port status polling before port reset"

Re: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

2016-07-11 Thread Tian, Feng

I am ok on this. Will update the variable name during check-in.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zeng, 
Star
Sent: Monday, July 11, 2016 3:44 PM
To: Tian, Feng <feng.t...@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status 
polling before port reset

How about using "ResetIsNeeded" to instead "Reset" as the parameter to make it 
more clear?

Reviewed-by: Star Zeng <star.z...@intel.com>


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Feng Tian
Sent: Monday, July 11, 2016 11:22 AM
To: Zeng, Star <star.z...@intel.com>
Cc: edk2-devel@lists.01.org
Subject: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling 
before port reset

This change is used to remove the port status polling in port reset functions.

Why it's needed is because:
1) The same polling on same port has taken place prior to this removed one. See 
UsbEnumeratePort()->GetPortStatus(). So this polling in
UsbEnumerateNewDev()->ResetPort() is redundant.
2) EDKII Xhci driver hooks all GetPortStatus() operations. If we don't remove 
this one, XHCI driver's XhcPollPortStatusChange() may enter twice in reset 
process and wrongly think the device is unplugged.

Cc: Star Zeng <star.z...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian <feng.t...@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 28 ++--
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c| 20 +---
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
index 79453fe..90a8b8c 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
@@ -643,6 +643,7 @@ UsbFindChild (
 
   @param  HubIf The HUB that has the device connected.
   @param  Port  The port index of the hub (started with zero).
+  @param  Reset The boolean to control whether skip the reset 
of the port.
 
   @retval EFI_SUCCESS   The device is enumerated (added or removed).
   @retval EFI_OUT_OF_RESOURCES  Failed to allocate resource for the device.
@@ -652,7 +653,8 @@ UsbFindChild (
 EFI_STATUS
 UsbEnumerateNewDev (
   IN USB_INTERFACE*HubIf,
-  IN UINT8Port
+  IN UINT8Port,
+  IN BOOLEAN  Reset
   )
 {
   USB_BUS *Bus;
@@ -677,16 +679,18 @@ UsbEnumerateNewDev (
   // and the hub is a EHCI root hub, ResetPort will release
   // the device to its companion UHCI and return an error.
   //
-  Status = HubApi->ResetPort (HubIf, Port);
-
-  if (EFI_ERROR (Status)) {
-DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d - %r\n", 
Port, Status));
-
-return Status;
+  if (Reset) {
+Status = HubApi->ResetPort (HubIf, Port);
+if (EFI_ERROR (Status)) {
+  DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d
+ - %r\n", Port, Status));
+  
+  return Status;
+}
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", 
+ Port));  } else {
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d reset is 
+ skipped\n", Port));
   }
 
-  DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", Port));
-
   Child = UsbCreateDevice (HubIf, Port);
 
   if (Child == NULL) {
@@ -964,7 +968,11 @@ UsbEnumeratePort (
 // Now, new device connected, enumerate and configure the device 
 //
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: new device connected at port 
%d\n", Port));
-Status = UsbEnumerateNewDev (HubIf, Port);
+if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET)) {
+  Status = UsbEnumerateNewDev (HubIf, Port, FALSE);
+} else {
+  Status = UsbEnumerateNewDev (HubIf, Port, TRUE);
+}
   
   } else {
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: device disconnected event on port 
%d\n", Port)); diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
index e3752d1..f51a51f 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
@@ -2,7 +2,7 @@
 
 Unified interface for RootHub and Hub.
 
-Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved. 
+Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions of the BSD License  which accompanies this 
distribution.  The full text of the license may be found at @@ -968,15 +968,6 
@@ UsbHubResetPort (
   UINTN   Index;
   EFI_STATUS  Status

Re: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

2016-07-11 Thread Zeng, Star

How about using "ResetIsNeeded" to instead "Reset" as the parameter to make it 
more clear?

Reviewed-by: Star Zeng <star.z...@intel.com>


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Feng Tian
Sent: Monday, July 11, 2016 11:22 AM
To: Zeng, Star <star.z...@intel.com>
Cc: edk2-devel@lists.01.org
Subject: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling 
before port reset

This change is used to remove the port status polling in port reset functions.

Why it's needed is because:
1) The same polling on same port has taken place prior to this removed one. See 
UsbEnumeratePort()->GetPortStatus(). So this polling in
UsbEnumerateNewDev()->ResetPort() is redundant.
2) EDKII Xhci driver hooks all GetPortStatus() operations. If we don't remove 
this one, XHCI driver's XhcPollPortStatusChange() may enter twice in reset 
process and wrongly think the device is unplugged.

Cc: Star Zeng <star.z...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian <feng.t...@intel.com>
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 28 ++--
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c| 20 +---
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
index 79453fe..90a8b8c 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
@@ -643,6 +643,7 @@ UsbFindChild (
 
   @param  HubIf The HUB that has the device connected.
   @param  Port  The port index of the hub (started with zero).
+  @param  Reset The boolean to control whether skip the reset 
of the port.
 
   @retval EFI_SUCCESS   The device is enumerated (added or removed).
   @retval EFI_OUT_OF_RESOURCES  Failed to allocate resource for the device.
@@ -652,7 +653,8 @@ UsbFindChild (
 EFI_STATUS
 UsbEnumerateNewDev (
   IN USB_INTERFACE*HubIf,
-  IN UINT8Port
+  IN UINT8Port,
+  IN BOOLEAN  Reset
   )
 {
   USB_BUS *Bus;
@@ -677,16 +679,18 @@ UsbEnumerateNewDev (
   // and the hub is a EHCI root hub, ResetPort will release
   // the device to its companion UHCI and return an error.
   //
-  Status = HubApi->ResetPort (HubIf, Port);
-
-  if (EFI_ERROR (Status)) {
-DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d - %r\n", 
Port, Status));
-
-return Status;
+  if (Reset) {
+Status = HubApi->ResetPort (HubIf, Port);
+if (EFI_ERROR (Status)) {
+  DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d 
+ - %r\n", Port, Status));
+  
+  return Status;
+}
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", 
+ Port));  } else {
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d reset is 
+ skipped\n", Port));
   }
 
-  DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", Port));
-
   Child = UsbCreateDevice (HubIf, Port);
 
   if (Child == NULL) {
@@ -964,7 +968,11 @@ UsbEnumeratePort (
 // Now, new device connected, enumerate and configure the device 
 //
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: new device connected at port 
%d\n", Port));
-Status = UsbEnumerateNewDev (HubIf, Port);
+if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET)) {
+  Status = UsbEnumerateNewDev (HubIf, Port, FALSE);
+} else {
+  Status = UsbEnumerateNewDev (HubIf, Port, TRUE);
+}
   
   } else {
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: device disconnected event on port 
%d\n", Port)); diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
index e3752d1..f51a51f 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
@@ -2,7 +2,7 @@
 
 Unified interface for RootHub and Hub.
 
-Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved. 
+Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions of the BSD License  which accompanies this 
distribution.  The full text of the license may be found at @@ -968,15 +968,6 
@@ UsbHubResetPort (
   UINTN   Index;
   EFI_STATUS  Status;
 
-  Status = UsbHubGetPortStatus (HubIf, Port, );
-
-  if (EFI_ERROR (Status)) {
-return Status;
-  } else if (USB_BIT_IS_SET (PortState.PortChangeStatus, 
USB_PORT_STAT_C_RESET)) {
-DEBUG (( EFI_D_INFO, "UsbHubResetPort: skip reset on hub %p port %d\n", 
HubIf, Port));
-return EFI_SUCCESS;
-  }
-
   Status  = UsbHubSetPortFeature (HubIf, Port, (EFI_USB_PORT_FEATURE)

[edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

2016-07-10 Thread Feng Tian

This change is used to remove the port status polling in port reset
functions.

Why it's needed is because:
1) The same polling on same port has taken place prior to this removed
one. See UsbEnumeratePort()->GetPortStatus(). So this polling in
UsbEnumerateNewDev()->ResetPort() is redundant.
2) EDKII Xhci driver hooks all GetPortStatus() operations. If we don't
remove this one, XHCI driver's XhcPollPortStatusChange() may enter twice
in reset process and wrongly think the device is unplugged.

Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Feng Tian 
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c | 28 ++--
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c| 20 +---
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
index 79453fe..90a8b8c 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c
@@ -643,6 +643,7 @@ UsbFindChild (
 
   @param  HubIf The HUB that has the device connected.
   @param  Port  The port index of the hub (started with zero).
+  @param  Reset The boolean to control whether skip the reset 
of the port.
 
   @retval EFI_SUCCESS   The device is enumerated (added or removed).
   @retval EFI_OUT_OF_RESOURCES  Failed to allocate resource for the device.
@@ -652,7 +653,8 @@ UsbFindChild (
 EFI_STATUS
 UsbEnumerateNewDev (
   IN USB_INTERFACE*HubIf,
-  IN UINT8Port
+  IN UINT8Port,
+  IN BOOLEAN  Reset
   )
 {
   USB_BUS *Bus;
@@ -677,16 +679,18 @@ UsbEnumerateNewDev (
   // and the hub is a EHCI root hub, ResetPort will release
   // the device to its companion UHCI and return an error.
   //
-  Status = HubApi->ResetPort (HubIf, Port);
-
-  if (EFI_ERROR (Status)) {
-DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d - %r\n", 
Port, Status));
-
-return Status;
+  if (Reset) {
+Status = HubApi->ResetPort (HubIf, Port);
+if (EFI_ERROR (Status)) {
+  DEBUG ((EFI_D_ERROR, "UsbEnumerateNewDev: failed to reset port %d - 
%r\n", Port, Status));
+  
+  return Status;
+}
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", Port));
+  } else {
+DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d reset is skipped\n", 
Port));
   }
 
-  DEBUG (( EFI_D_INFO, "UsbEnumerateNewDev: hub port %d is reset\n", Port));
-
   Child = UsbCreateDevice (HubIf, Port);
 
   if (Child == NULL) {
@@ -964,7 +968,11 @@ UsbEnumeratePort (
 // Now, new device connected, enumerate and configure the device 
 //
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: new device connected at port 
%d\n", Port));
-Status = UsbEnumerateNewDev (HubIf, Port);
+if (USB_BIT_IS_SET (PortState.PortChangeStatus, USB_PORT_STAT_C_RESET)) {
+  Status = UsbEnumerateNewDev (HubIf, Port, FALSE);
+} else {
+  Status = UsbEnumerateNewDev (HubIf, Port, TRUE);
+}
   
   } else {
 DEBUG (( EFI_D_INFO, "UsbEnumeratePort: device disconnected event on port 
%d\n", Port));
diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
index e3752d1..f51a51f 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c
@@ -2,7 +2,7 @@
 
 Unified interface for RootHub and Hub.
 
-Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved. 
+Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved. 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -968,15 +968,6 @@ UsbHubResetPort (
   UINTN   Index;
   EFI_STATUS  Status;
 
-  Status = UsbHubGetPortStatus (HubIf, Port, );
-
-  if (EFI_ERROR (Status)) {
-return Status;
-  } else if (USB_BIT_IS_SET (PortState.PortChangeStatus, 
USB_PORT_STAT_C_RESET)) {
-DEBUG (( EFI_D_INFO, "UsbHubResetPort: skip reset on hub %p port %d\n", 
HubIf, Port));
-return EFI_SUCCESS;
-  }
-
   Status  = UsbHubSetPortFeature (HubIf, Port, (EFI_USB_PORT_FEATURE) 
USB_HUB_PORT_RESET);
 
   if (EFI_ERROR (Status)) {
@@ -1282,15 +1273,6 @@ UsbRootHubResetPort (
   //
   Bus = RootIf->Device->Bus;
 
-  Status = UsbHcGetRootHubPortStatus (Bus, Port, );
-
-  if (EFI_ERROR (Status)) {
-return Status;
-  } else if (USB_BIT_IS_SET (PortState.PortChangeStatus, 
USB_PORT_STAT_C_RESET)) {
-DEBUG (( EFI_D_INFO, "UsbRootHubResetPort: skip reset on root port %d\n", 
Port));
-return EFI_SUCCESS;
-  }
-
   Status  = UsbHcSetRootHubPortFeature (Bus, Port, EfiUsbPortReset);
 
   if (EFI_ERROR (Status)) {
-- 
2.7.1.windows.2

Re: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

Re: [edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

[edk2] [patch 2/3] MdeModulePkg/UsbBus: reduce the port status polling before port reset

3 matches

Site Navigation

Mail list logo

Footer information