[PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in EDKII
EHCI driver if it's used by usb debug port driver", 2012-04-28) made the
host controller reset in EhcDriverBindingStart() conditional. The intent
was to avoid the reset when
- one of the USB ports on the host controller was a Debug Port, AND
- the Debug Port was in use.

This translates to the following positive condition: reset the controller
only if:
- no USB port on the host controller is a Debug Port, OR
- the Debug Port is not in use.

Commit 09943f5ecc0f failed to implement the first subcondition, and thus
the result is too strict now (for resetting the host controller). Relax it
to the correct condition.

This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does
not have a Debug Port. In repeated disconnect / connect cycles, the
controller is never reset in EhcDriverBindingStart(), therefore the
devices on the controller are never re-enumerated after a disconnect.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Reported-by: Steven Shi <steven.shi@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Repo:   https://github.com/lersek/edk2.git
    Branch: ehci_start_reset_1129

 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
index 30ad2b2ffeef..89ed034b9093 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
   //
   if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
     EhcClearLegacySupport (Ehc);
   }
 
-  if (Ehc->DebugPortNum != 0) {
+  if (Ehc->DebugPortNum == 0) {
+    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
+  } else {
     State = EhcReadDbgRegister(Ehc, 0);
     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
     }
   }
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Zeng, Star]

Good finding.

There is another place at https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c#L150 which has similar logic, please update it also.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, August 31, 2018 10:04 PM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28) made the host controller reset in EhcDriverBindingStart() conditional. The intent was to avoid the reset when
- one of the USB ports on the host controller was a Debug Port, AND
- the Debug Port was in use.

This translates to the following positive condition: reset the controller only if:
- no USB port on the host controller is a Debug Port, OR
- the Debug Port is not in use.

Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.

This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Reported-by: Steven Shi <steven.shi@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Repo:   https://github.com/lersek/edk2.git
    Branch: ehci_start_reset_1129

 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
index 30ad2b2ffeef..89ed034b9093 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
   //
   if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
     EhcClearLegacySupport (Ehc);
   }
 
-  if (Ehc->DebugPortNum != 0) {
+  if (Ehc->DebugPortNum == 0) {
+    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
     State = EhcReadDbgRegister(Ehc, 0);
     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
     }
   }
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

Hi Star,

On 09/03/18 10:35, Zeng, Star wrote:
> Good finding.
>
> There is another place at
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c#L150
> which has similar logic, please update it also.

I audited all the "DebugPortNum" checks that were added by commit
09943f5ecc0f, here:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1129#c16

The location that you mention is in the EhcReset() function. It is
covered in my analysis linked above, and it needs no update, because it
is correct.

Namely, in EhcReset(), the condition captures when the reset must be
*skipped*. For that, the condition is,

  one of the USB ports on the host controller is a debug port, AND
  the debug port is in use

When this condition holds, we set Status to EFI_SUCCESS, and jump to
ON_EXIT (that is, we skip the re-set and return success without doing
anything). This is written in C as

>     if (Ehc->DebugPortNum != 0) {
>       DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>       if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>         Status = EFI_SUCCESS;
>         goto ON_EXIT;
>       }
>     }

However, at the location that the patch modifies, the condition refers
to the *opposite* case, that is, when the reset must *not* be skipped.
For that, we have to negate the condition seen above.

For the negation, we use De Morgan's Laws, that is:

  !(A && B) == (!A || !B)

That is:

  *no* USB port on the host controller is a debug port, *OR*
  the debug port is *not* in use

And this is written, in C, as

>   if (Ehc->DebugPortNum == 0) {
>     EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>   } else {
>     State = EhcReadDbgRegister(Ehc, 0);
>     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>     }
>   }

The original commit (09943f5ecc0f) got the EhcReset() modification right
(that is, the condition is already correct); what it got wrong was the
negation of the condition, for EhcDriverBindingStart(). That's the only
thing we have to correct in this patch.

Thanks,
Laszlo


>
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 31, 2018 10:04 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
> Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart
>
> Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28) made the host controller reset in EhcDriverBindingStart() conditional. The intent was to avoid the reset when
> - one of the USB ports on the host controller was a Debug Port, AND
> - the Debug Port was in use.
>
> This translates to the following positive condition: reset the controller only if:
> - no USB port on the host controller is a Debug Port, OR
> - the Debug Port is not in use.
>
> Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.
>
> This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Reported-by: Steven Shi <steven.shi@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: ehci_start_reset_1129
>
>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> index 30ad2b2ffeef..89ed034b9093 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> @@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
>    //
>    if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
>      EhcClearLegacySupport (Ehc);
>    }
>
> -  if (Ehc->DebugPortNum != 0) {
> +  if (Ehc->DebugPortNum == 0) {
> +    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
>      State = EhcReadDbgRegister(Ehc, 0);
>      if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>        EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>      }
>    }
> --
> 2.14.1.3.gb7cf6e02401b
>

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Zeng, Star]

Oh, yes. Got the point, the patch is correct indeed. :)
Thanks for the explanation.

Maybe, an abstracted function will make the logic more clear, like EhcIsDebugPortInUse().
What do you think?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Monday, September 3, 2018 7:46 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven <steven.shi@intel.com>
Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

Hi Star,

On 09/03/18 10:35, Zeng, Star wrote:
> Good finding.
>
> There is another place at
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Ehc
> iDxe/Ehci.c#L150 which has similar logic, please update it also.

I audited all the "DebugPortNum" checks that were added by commit 09943f5ecc0f, here:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1129#c16

The location that you mention is in the EhcReset() function. It is covered in my analysis linked above, and it needs no update, because it is correct.

Namely, in EhcReset(), the condition captures when the reset must be *skipped*. For that, the condition is,

  one of the USB ports on the host controller is a debug port, AND
  the debug port is in use

When this condition holds, we set Status to EFI_SUCCESS, and jump to ON_EXIT (that is, we skip the re-set and return success without doing anything). This is written in C as

>     if (Ehc->DebugPortNum != 0) {
>       DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>       if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>         Status = EFI_SUCCESS;
>         goto ON_EXIT;
>       }
>     }

However, at the location that the patch modifies, the condition refers to the *opposite* case, that is, when the reset must *not* be skipped.
For that, we have to negate the condition seen above.

For the negation, we use De Morgan's Laws, that is:

  !(A && B) == (!A || !B)

That is:

  *no* USB port on the host controller is a debug port, *OR*
  the debug port is *not* in use

And this is written, in C, as

>   if (Ehc->DebugPortNum == 0) {
>     EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>   } else {
>     State = EhcReadDbgRegister(Ehc, 0);
>     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>     }
>   }

The original commit (09943f5ecc0f) got the EhcReset() modification right (that is, the condition is already correct); what it got wrong was the negation of the condition, for EhcDriverBindingStart(). That's the only thing we have to correct in this patch.

Thanks,
Laszlo


>
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 31, 2018 10:04 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star 
> <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
> Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset 
> condition in BindingStart
>
> Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in 
> EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28) 
> made the host controller reset in EhcDriverBindingStart() conditional. 
> The intent was to avoid the reset when
> - one of the USB ports on the host controller was a Debug Port, AND
> - the Debug Port was in use.
>
> This translates to the following positive condition: reset the controller only if:
> - no USB port on the host controller is a Debug Port, OR
> - the Debug Port is not in use.
>
> Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.
>
> This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Reported-by: Steven Shi <steven.shi@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     Repo:   https://github.com/lersek/edk2.git
>     Branch: ehci_start_reset_1129
>
>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c 
> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> index 30ad2b2ffeef..89ed034b9093 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> @@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
>    //
>    if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
>      EhcClearLegacySupport (Ehc);
>    }
>
> -  if (Ehc->DebugPortNum != 0) {
> +  if (Ehc->DebugPortNum == 0) {
> +    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
>      State = EhcReadDbgRegister(Ehc, 0);
>      if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>        EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>      }
>    }
> --
> 2.14.1.3.gb7cf6e02401b
>

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

On 09/04/18 03:27, Zeng, Star wrote:
> Oh, yes. Got the point, the patch is correct indeed. :)
> Thanks for the explanation.
>
> Maybe, an abstracted function will make the logic more clear, like
> EhcIsDebugPortInUse().
> What do you think?

Haha, I thought of almost exactly the same name (just
"IsDebugPortInUse()", and making it STATIC) :) Ultimately however, I
didn't propose it, for the following reason:

There is another conditional hunk added by commit 09943f5ecc0f, namely
in the EhcGetRootHubPortStatus() function. In the Bugzilla, I wrote:

>   - in the EhcGetRootHubPortStatus() function, if the status request
>     refers to the debug port, and the debug port is in use, then we
>     don't actually fetch the status of the port, we just report
>     status=0, status_change=0.

and the code goes like this:

>   if ((Ehc->DebugPortNum != 0) && (PortNumber == (Ehc->DebugPortNum - 1))) {
>     DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>     if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>       goto ON_EXIT;
>     }
>   }

In this case, we not only check whether the host controller has a debug
port (*any* port) and that port is in use, we also check whether the
status request refers specifically to that port.

This additional restriction is not easy to express with
EhcIsDebugPortInUse(). If we wrote

  if (EhcIsDebugPortInUse () && (PortNumber == (Ehc->DebugPortNum - 1))) {
    goto ON_EXIT;
  }

then that wouldn't be 100% the same, because in this reformulated
condition, we could access the MMIO debug status register *before* we
decided that the root hub port status request refers to *another* port.

How about the following function:

> /**
>   Check whether the host controller has an in-use debug port.
>
>   @param[in] Ehc         The Enhanced Host Controller to query.
>
>   @param[in] PortNumber  If PortNumber is in [0, MAX_UINT8], then query whether
>                          PortNumber on Ehc is an in-use debug port. Otherwise,
>                          query whether Ehc has any in-use debug port.
>
>   @retval TRUE   PortNumber on Ehc is an in-use debug port (if PortNumber is in
>                  [0, MAX_UINT8]), or some port on Ehc is an in-use debug port
>                  (otherwise).
>
>   @retval FALSE  PortNumber on Ehc is not an in-use debug port (if PortNumber
>                  is in [0, MAX_UINT8]), or no port on Ehc is an in-use debug
>                  port (otherwise).
> **/
> BOOLEAN
> EhcIsDebugPortInUse (
>   IN CONST USB2_HC_DEV *Ehc,
>   IN INT16             PortNumber
>   )
> {
>   UINT32 State;
>   UINT32 Mask;
>
>   if (Ehc->DebugPortNum == 0) {
>     return FALSE;
>   }
>
>   if (PortNumber >= 0 && PortNumber <= MAX_UINT8 &&
>       (UINT8)PortNumber != Ehc->DebugPortNum - 1) {
>     return FALSE;
>   }
>
>   State = EhcReadDbgRegister(Ehc, 0);
>   Mask = USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER;
>   return (State & Mask) == Mask;
> }

Then we can call

  EhcIsDebugPortInUse (Ehc, -1)

in EhcReset() and EhcDriverBindingStart(), and we can call

  EhcIsDebugPortInUse (Ehc, PortNumber)

in EhcGetRootHubPortStatus().

What do you think?

... Anyway, I would like to append such a patch separately to the series
-- I'd like to keep the bugfix and the refactoring separate.

Thanks,
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, September 3, 2018 7:46 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven <steven.shi@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart
>
> Hi Star,
>
> On 09/03/18 10:35, Zeng, Star wrote:
>> Good finding.
>>
>> There is another place at
>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Ehc
>> iDxe/Ehci.c#L150 which has similar logic, please update it also.
>
> I audited all the "DebugPortNum" checks that were added by commit 09943f5ecc0f, here:
>
>   https://bugzilla.tianocore.org/show_bug.cgi?id=1129#c16
>
> The location that you mention is in the EhcReset() function. It is covered in my analysis linked above, and it needs no update, because it is correct.
>
> Namely, in EhcReset(), the condition captures when the reset must be *skipped*. For that, the condition is,
>
>   one of the USB ports on the host controller is a debug port, AND
>   the debug port is in use
>
> When this condition holds, we set Status to EFI_SUCCESS, and jump to ON_EXIT (that is, we skip the re-set and return success without doing anything). This is written in C as
>
>>     if (Ehc->DebugPortNum != 0) {
>>       DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>>       if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>         Status = EFI_SUCCESS;
>>         goto ON_EXIT;
>>       }
>>     }
>
> However, at the location that the patch modifies, the condition refers to the *opposite* case, that is, when the reset must *not* be skipped.
> For that, we have to negate the condition seen above.
>
> For the negation, we use De Morgan's Laws, that is:
>
>   !(A && B) == (!A || !B)
>
> That is:
>
>   *no* USB port on the host controller is a debug port, *OR*
>   the debug port is *not* in use
>
> And this is written, in C, as
>
>>   if (Ehc->DebugPortNum == 0) {
>>     EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>   } else {
>>     State = EhcReadDbgRegister(Ehc, 0);
>>     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>     }
>>   }
>
> The original commit (09943f5ecc0f) got the EhcReset() modification right (that is, the condition is already correct); what it got wrong was the negation of the condition, for EhcDriverBindingStart(). That's the only thing we have to correct in this patch.
>
> Thanks,
> Laszlo
>
>
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, August 31, 2018 10:04 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
>> Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset
>> condition in BindingStart
>>
>> Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in
>> EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28)
>> made the host controller reset in EhcDriverBindingStart() conditional.
>> The intent was to avoid the reset when
>> - one of the USB ports on the host controller was a Debug Port, AND
>> - the Debug Port was in use.
>>
>> This translates to the following positive condition: reset the controller only if:
>> - no USB port on the host controller is a Debug Port, OR
>> - the Debug Port is not in use.
>>
>> Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.
>>
>> This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Steven Shi <steven.shi@intel.com>
>> Reported-by: Steven Shi <steven.shi@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
>> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     Repo:   https://github.com/lersek/edk2.git
>>     Branch: ehci_start_reset_1129
>>
>>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> index 30ad2b2ffeef..89ed034b9093 100644
>> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> @@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
>>    //
>>    if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
>>      EhcClearLegacySupport (Ehc);
>>    }
>>
>> -  if (Ehc->DebugPortNum != 0) {
>> +  if (Ehc->DebugPortNum == 0) {
>> +    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
>>      State = EhcReadDbgRegister(Ehc, 0);
>>      if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>        EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>      }
>>    }
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Zeng, Star]

I agree to commit this patch first. Reviewed-by: Star Zeng <star.zeng@intel.com> to this patch.

To further enhance with an abstracted function, how about like below?

BOOLEAN
EhcIsDebugPortInUse (
IN CONST USB2_HC_DEV *Ehc,
IN UINT8             *PortNumber OPTIONAL
)

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, September 4, 2018 6:38 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven <steven.shi@intel.com>
Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

On 09/04/18 03:27, Zeng, Star wrote:
> Oh, yes. Got the point, the patch is correct indeed. :) Thanks for the 
> explanation.
>
> Maybe, an abstracted function will make the logic more clear, like 
> EhcIsDebugPortInUse().
> What do you think?

Haha, I thought of almost exactly the same name (just "IsDebugPortInUse()", and making it STATIC) :) Ultimately however, I didn't propose it, for the following reason:

There is another conditional hunk added by commit 09943f5ecc0f, namely in the EhcGetRootHubPortStatus() function. In the Bugzilla, I wrote:

>   - in the EhcGetRootHubPortStatus() function, if the status request
>     refers to the debug port, and the debug port is in use, then we
>     don't actually fetch the status of the port, we just report
>     status=0, status_change=0.

and the code goes like this:

>   if ((Ehc->DebugPortNum != 0) && (PortNumber == (Ehc->DebugPortNum - 1))) {
>     DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>     if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>       goto ON_EXIT;
>     }
>   }

In this case, we not only check whether the host controller has a debug port (*any* port) and that port is in use, we also check whether the status request refers specifically to that port.

This additional restriction is not easy to express with EhcIsDebugPortInUse(). If we wrote

  if (EhcIsDebugPortInUse () && (PortNumber == (Ehc->DebugPortNum - 1))) {
    goto ON_EXIT;
  }

then that wouldn't be 100% the same, because in this reformulated condition, we could access the MMIO debug status register *before* we decided that the root hub port status request refers to *another* port.

How about the following function:

> /**
>   Check whether the host controller has an in-use debug port.
>
>   @param[in] Ehc         The Enhanced Host Controller to query.
>
>   @param[in] PortNumber  If PortNumber is in [0, MAX_UINT8], then query whether
>                          PortNumber on Ehc is an in-use debug port. Otherwise,
>                          query whether Ehc has any in-use debug port.
>
>   @retval TRUE   PortNumber on Ehc is an in-use debug port (if PortNumber is in
>                  [0, MAX_UINT8]), or some port on Ehc is an in-use debug port
>                  (otherwise).
>
>   @retval FALSE  PortNumber on Ehc is not an in-use debug port (if PortNumber
>                  is in [0, MAX_UINT8]), or no port on Ehc is an in-use debug
>                  port (otherwise).
> **/
> BOOLEAN
> EhcIsDebugPortInUse (
>   IN CONST USB2_HC_DEV *Ehc,
>   IN INT16             PortNumber
>   )
> {
>   UINT32 State;
>   UINT32 Mask;
>
>   if (Ehc->DebugPortNum == 0) {
>     return FALSE;
>   }
>
>   if (PortNumber >= 0 && PortNumber <= MAX_UINT8 &&
>       (UINT8)PortNumber != Ehc->DebugPortNum - 1) {
>     return FALSE;
>   }
>
>   State = EhcReadDbgRegister(Ehc, 0);
>   Mask = USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER;
>   return (State & Mask) == Mask;
> }

Then we can call

  EhcIsDebugPortInUse (Ehc, -1)

in EhcReset() and EhcDriverBindingStart(), and we can call

  EhcIsDebugPortInUse (Ehc, PortNumber)

in EhcGetRootHubPortStatus().

What do you think?

... Anyway, I would like to append such a patch separately to the series
-- I'd like to keep the bugfix and the refactoring separate.

Thanks,
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, September 3, 2018 7:46 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 
> <edk2-devel@lists.01.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven 
> <steven.shi@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset 
> condition in BindingStart
>
> Hi Star,
>
> On 09/03/18 10:35, Zeng, Star wrote:
>> Good finding.
>>
>> There is another place at
>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Eh
>> c
>> iDxe/Ehci.c#L150 which has similar logic, please update it also.
>
> I audited all the "DebugPortNum" checks that were added by commit 09943f5ecc0f, here:
>
>   https://bugzilla.tianocore.org/show_bug.cgi?id=1129#c16
>
> The location that you mention is in the EhcReset() function. It is covered in my analysis linked above, and it needs no update, because it is correct.
>
> Namely, in EhcReset(), the condition captures when the reset must be 
> *skipped*. For that, the condition is,
>
>   one of the USB ports on the host controller is a debug port, AND
>   the debug port is in use
>
> When this condition holds, we set Status to EFI_SUCCESS, and jump to 
> ON_EXIT (that is, we skip the re-set and return success without doing 
> anything). This is written in C as
>
>>     if (Ehc->DebugPortNum != 0) {
>>       DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>>       if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>         Status = EFI_SUCCESS;
>>         goto ON_EXIT;
>>       }
>>     }
>
> However, at the location that the patch modifies, the condition refers to the *opposite* case, that is, when the reset must *not* be skipped.
> For that, we have to negate the condition seen above.
>
> For the negation, we use De Morgan's Laws, that is:
>
>   !(A && B) == (!A || !B)
>
> That is:
>
>   *no* USB port on the host controller is a debug port, *OR*
>   the debug port is *not* in use
>
> And this is written, in C, as
>
>>   if (Ehc->DebugPortNum == 0) {
>>     EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>   } else {
>>     State = EhcReadDbgRegister(Ehc, 0);
>>     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>     }
>>   }
>
> The original commit (09943f5ecc0f) got the EhcReset() modification right (that is, the condition is already correct); what it got wrong was the negation of the condition, for EhcDriverBindingStart(). That's the only thing we have to correct in this patch.
>
> Thanks,
> Laszlo
>
>
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, August 31, 2018 10:04 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J 
>> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star 
>> <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
>> Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset 
>> condition in BindingStart
>>
>> Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in 
>> EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28) 
>> made the host controller reset in EhcDriverBindingStart() conditional.
>> The intent was to avoid the reset when
>> - one of the USB ports on the host controller was a Debug Port, AND
>> - the Debug Port was in use.
>>
>> This translates to the following positive condition: reset the controller only if:
>> - no USB port on the host controller is a Debug Port, OR
>> - the Debug Port is not in use.
>>
>> Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.
>>
>> This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Steven Shi <steven.shi@intel.com>
>> Reported-by: Steven Shi <steven.shi@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
>> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     Repo:   https://github.com/lersek/edk2.git
>>     Branch: ehci_start_reset_1129
>>
>>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> index 30ad2b2ffeef..89ed034b9093 100644
>> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>> @@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
>>    //
>>    if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
>>      EhcClearLegacySupport (Ehc);
>>    }
>>
>> -  if (Ehc->DebugPortNum != 0) {
>> +  if (Ehc->DebugPortNum == 0) {
>> +    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
>>      State = EhcReadDbgRegister(Ehc, 0);
>>      if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>        EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>      }
>>    }
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

On 09/05/18 11:55, Zeng, Star wrote:
> I agree to commit this patch first. Reviewed-by: Star Zeng <star.zeng@intel.com> to this patch.

Thanks!

Steven, can you confirm the patch fixes the issue you saw?

> To further enhance with an abstracted function, how about like below?
> 
> BOOLEAN
> EhcIsDebugPortInUse (
> IN CONST USB2_HC_DEV *Ehc,
> IN UINT8             *PortNumber OPTIONAL
> )

Yes, that works too; I had thought of this. I didn't suggest it because
in case the programmer wants to pass a known constant for PortNumber,
they need a separate variable, which is a bit inconvenient. But, I agree
that given the current source code, that won't happen. I'll post another
patch for this.

Thanks!
Laszlo

> 
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Tuesday, September 4, 2018 6:38 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven <steven.shi@intel.com>
> Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart
> 
> On 09/04/18 03:27, Zeng, Star wrote:
>> Oh, yes. Got the point, the patch is correct indeed. :) Thanks for the 
>> explanation.
>>
>> Maybe, an abstracted function will make the logic more clear, like 
>> EhcIsDebugPortInUse().
>> What do you think?
> 
> Haha, I thought of almost exactly the same name (just "IsDebugPortInUse()", and making it STATIC) :) Ultimately however, I didn't propose it, for the following reason:
> 
> There is another conditional hunk added by commit 09943f5ecc0f, namely in the EhcGetRootHubPortStatus() function. In the Bugzilla, I wrote:
> 
>>   - in the EhcGetRootHubPortStatus() function, if the status request
>>     refers to the debug port, and the debug port is in use, then we
>>     don't actually fetch the status of the port, we just report
>>     status=0, status_change=0.
> 
> and the code goes like this:
> 
>>   if ((Ehc->DebugPortNum != 0) && (PortNumber == (Ehc->DebugPortNum - 1))) {
>>     DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>>     if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>       goto ON_EXIT;
>>     }
>>   }
> 
> In this case, we not only check whether the host controller has a debug port (*any* port) and that port is in use, we also check whether the status request refers specifically to that port.
> 
> This additional restriction is not easy to express with EhcIsDebugPortInUse(). If we wrote
> 
>   if (EhcIsDebugPortInUse () && (PortNumber == (Ehc->DebugPortNum - 1))) {
>     goto ON_EXIT;
>   }
> 
> then that wouldn't be 100% the same, because in this reformulated condition, we could access the MMIO debug status register *before* we decided that the root hub port status request refers to *another* port.
> 
> How about the following function:
> 
>> /**
>>   Check whether the host controller has an in-use debug port.
>>
>>   @param[in] Ehc         The Enhanced Host Controller to query.
>>
>>   @param[in] PortNumber  If PortNumber is in [0, MAX_UINT8], then query whether
>>                          PortNumber on Ehc is an in-use debug port. Otherwise,
>>                          query whether Ehc has any in-use debug port.
>>
>>   @retval TRUE   PortNumber on Ehc is an in-use debug port (if PortNumber is in
>>                  [0, MAX_UINT8]), or some port on Ehc is an in-use debug port
>>                  (otherwise).
>>
>>   @retval FALSE  PortNumber on Ehc is not an in-use debug port (if PortNumber
>>                  is in [0, MAX_UINT8]), or no port on Ehc is an in-use debug
>>                  port (otherwise).
>> **/
>> BOOLEAN
>> EhcIsDebugPortInUse (
>>   IN CONST USB2_HC_DEV *Ehc,
>>   IN INT16             PortNumber
>>   )
>> {
>>   UINT32 State;
>>   UINT32 Mask;
>>
>>   if (Ehc->DebugPortNum == 0) {
>>     return FALSE;
>>   }
>>
>>   if (PortNumber >= 0 && PortNumber <= MAX_UINT8 &&
>>       (UINT8)PortNumber != Ehc->DebugPortNum - 1) {
>>     return FALSE;
>>   }
>>
>>   State = EhcReadDbgRegister(Ehc, 0);
>>   Mask = USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER;
>>   return (State & Mask) == Mask;
>> }
> 
> Then we can call
> 
>   EhcIsDebugPortInUse (Ehc, -1)
> 
> in EhcReset() and EhcDriverBindingStart(), and we can call
> 
>   EhcIsDebugPortInUse (Ehc, PortNumber)
> 
> in EhcGetRootHubPortStatus().
> 
> What do you think?
> 
> ... Anyway, I would like to append such a patch separately to the series
> -- I'd like to keep the bugfix and the refactoring separate.
> 
> Thanks,
> Laszlo
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, September 3, 2018 7:46 PM
>> To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 
>> <edk2-devel@lists.01.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J 
>> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Shi, Steven 
>> <steven.shi@intel.com>
>> Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset 
>> condition in BindingStart
>>
>> Hi Star,
>>
>> On 09/03/18 10:35, Zeng, Star wrote:
>>> Good finding.
>>>
>>> There is another place at
>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/Eh
>>> c
>>> iDxe/Ehci.c#L150 which has similar logic, please update it also.
>>
>> I audited all the "DebugPortNum" checks that were added by commit 09943f5ecc0f, here:
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=1129#c16
>>
>> The location that you mention is in the EhcReset() function. It is covered in my analysis linked above, and it needs no update, because it is correct.
>>
>> Namely, in EhcReset(), the condition captures when the reset must be 
>> *skipped*. For that, the condition is,
>>
>>   one of the USB ports on the host controller is a debug port, AND
>>   the debug port is in use
>>
>> When this condition holds, we set Status to EFI_SUCCESS, and jump to 
>> ON_EXIT (that is, we skip the re-set and return success without doing 
>> anything). This is written in C as
>>
>>>     if (Ehc->DebugPortNum != 0) {
>>>       DbgCtrlStatus = EhcReadDbgRegister(Ehc, 0);
>>>       if ((DbgCtrlStatus & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) == (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>>         Status = EFI_SUCCESS;
>>>         goto ON_EXIT;
>>>       }
>>>     }
>>
>> However, at the location that the patch modifies, the condition refers to the *opposite* case, that is, when the reset must *not* be skipped.
>> For that, we have to negate the condition seen above.
>>
>> For the negation, we use De Morgan's Laws, that is:
>>
>>   !(A && B) == (!A || !B)
>>
>> That is:
>>
>>   *no* USB port on the host controller is a debug port, *OR*
>>   the debug port is *not* in use
>>
>> And this is written, in C, as
>>
>>>   if (Ehc->DebugPortNum == 0) {
>>>     EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>>   } else {
>>>     State = EhcReadDbgRegister(Ehc, 0);
>>>     if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>>       EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>>     }
>>>   }
>>
>> The original commit (09943f5ecc0f) got the EhcReset() modification right (that is, the condition is already correct); what it got wrong was the negation of the condition, for EhcDriverBindingStart(). That's the only thing we have to correct in this patch.
>>
>> Thanks,
>> Laszlo
>>
>>
>>>
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Friday, August 31, 2018 10:04 PM
>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>; Wang, Jian J 
>>> <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star 
>>> <star.zeng@intel.com>; Shi, Steven <steven.shi@intel.com>
>>> Subject: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset 
>>> condition in BindingStart
>>>
>>> Commit 09943f5ecc0f ("MdeModulePkg: Skip to manage usb debug port in 
>>> EDKII EHCI driver if it's used by usb debug port driver", 2012-04-28) 
>>> made the host controller reset in EhcDriverBindingStart() conditional.
>>> The intent was to avoid the reset when
>>> - one of the USB ports on the host controller was a Debug Port, AND
>>> - the Debug Port was in use.
>>>
>>> This translates to the following positive condition: reset the controller only if:
>>> - no USB port on the host controller is a Debug Port, OR
>>> - the Debug Port is not in use.
>>>
>>> Commit 09943f5ecc0f failed to implement the first subcondition, and thus the result is too strict now (for resetting the host controller). Relax it to the correct condition.
>>>
>>> This issue was found by Steven Shi on QEMU. QEMU's EHCI device model does not have a Debug Port. In repeated disconnect / connect cycles, the controller is never reset in EhcDriverBindingStart(), therefore the devices on the controller are never re-enumerated after a disconnect.
>>>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Cc: Steven Shi <steven.shi@intel.com>
>>> Reported-by: Steven Shi <steven.shi@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129
>>> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     Repo:   https://github.com/lersek/edk2.git
>>>     Branch: ehci_start_reset_1129
>>>
>>>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>>> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>>> index 30ad2b2ffeef..89ed034b9093 100644
>>> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>>> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
>>> @@ -1916,11 +1916,13 @@ EhcDriverBindingStart (
>>>    //
>>>    if (FeaturePcdGet (PcdTurnOffUsbLegacySupport)) {
>>>      EhcClearLegacySupport (Ehc);
>>>    }
>>>
>>> -  if (Ehc->DebugPortNum != 0) {
>>> +  if (Ehc->DebugPortNum == 0) {
>>> +    EhcResetHC (Ehc, EHC_RESET_TIMEOUT);  } else {
>>>      State = EhcReadDbgRegister(Ehc, 0);
>>>      if ((State & (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) != (USB_DEBUG_PORT_IN_USE | USB_DEBUG_PORT_OWNER)) {
>>>        EhcResetHC (Ehc, EHC_RESET_TIMEOUT);
>>>      }
>>>    }
>>> --
>>> 2.14.1.3.gb7cf6e02401b
>>>
>>
> 

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Shi, Steven]

> Steven, can you confirm the patch fixes the issue you saw?
Yes. The patch fixes my issue in https://bugzilla.tianocore.org/show_bug.cgi?id=1129 .

Thanks
Steven Shi

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

On 09/05/18 12:55, Shi, Steven wrote:
>> Steven, can you confirm the patch fixes the issue you saw?
> Yes. The patch fixes my issue in https://bugzilla.tianocore.org/show_bug.cgi?id=1129 .

Thanks! I've added your Tested-by.

Pushed as commit c3d5d800d775.

I'll follow up with the EhcIsDebugPortInUse() refactoring soon.

Laszlo

Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart

[Laszlo Ersek]

Hi Star,

On 09/05/18 11:55, Zeng, Star wrote:
> I agree to commit this patch first. Reviewed-by: Star Zeng <star.zeng@intel.com> to this patch.
> 
> To further enhance with an abstracted function, how about like below?
> 
> BOOLEAN
> EhcIsDebugPortInUse (
> IN CONST USB2_HC_DEV *Ehc,
> IN UINT8             *PortNumber OPTIONAL
> )

do you want me to define the new EhcIsDebugPortInUse() function in
"Ehci.c", or in "EhciReg.c"?

"EhciReg.c" looks more appropriate to me:

- EhcIsHalt() is defined there,

- EhcIsSysError() is defined there,

- the EhcReadDbgRegister() function would become internal to
  "EhciReg.c", because all of the current call sites (in "Ehci.c") would
  be replaced by calls to EhcIsDebugPortInUse(). In turn this would
  allow us to remove the declaration of EhcReadDbgRegister(), from
  "EhciReg.h".

Thanks!
Laszlo