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

[Laszlo Ersek <lersek@redhat.com>]

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
>