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

[Laszlo Ersek <lersek@redhat.com>]

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