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

["Zeng, Star" <star.zeng@intel.com>]

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
>>
>