From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B18F621A07A80 for ; Wed, 5 Sep 2018 03:13:55 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3C3F040241C3; Wed, 5 Sep 2018 10:13:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-25.rdu2.redhat.com [10.10.121.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4871C20389E0; Wed, 5 Sep 2018 10:13:50 +0000 (UTC) To: "Zeng, Star" , edk2-devel-01 Cc: Gerd Hoffmann , "Wang, Jian J" , "Ni, Ruiyu" , "Shi, Steven" References: <20180831140330.26888-1-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BBB4C09@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BBB4E60@shsmsx102.ccr.corp.intel.com> <9aa5a24b-44b4-fb14-cb68-35028edf0906@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BBB55E8@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 5 Sep 2018 12:13:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BBB55E8@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 05 Sep 2018 10:13:54 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 05 Sep 2018 10:13:54 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] MdeModulePkg/EhciDxe: fix host controller reset condition in BindingStart X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Sep 2018 10:13:56 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/05/18 11:55, Zeng, Star wrote: > I agree to commit this patch first. Reviewed-by: Star Zeng 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 ; edk2-devel-01 > Cc: Gerd Hoffmann ; Wang, Jian J ; Ni, Ruiyu ; Shi, Steven > 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 ; edk2-devel-01 >> >> Cc: Gerd Hoffmann ; Wang, Jian J >> ; Ni, Ruiyu ; Shi, Steven >> >> 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 >>> Cc: Gerd Hoffmann ; Wang, Jian J >>> ; Ni, Ruiyu ; Zeng, Star >>> ; Shi, Steven >>> 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 >>> Cc: Jian J Wang >>> Cc: Ruiyu Ni >>> Cc: Star Zeng >>> Cc: Steven Shi >>> Reported-by: Steven Shi >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1129 >>> Fixes: 09943f5ecc0fbc0c98c511c82703a0ba3b2b5819 >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Laszlo Ersek >>> --- >>> >>> 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 >>> >> >