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 E3EE6202E5458 for ; Mon, 3 Sep 2018 04:45:49 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B74C804BAAC; Mon, 3 Sep 2018 11:45:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-191.rdu2.redhat.com [10.10.120.191]) by smtp.corp.redhat.com (Postfix) with ESMTP id 567E110EE781; Mon, 3 Sep 2018 11:45:45 +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> From: Laszlo Ersek Message-ID: Date: Mon, 3 Sep 2018 13:45:44 +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: <0C09AFA07DD0434D9E2A0C6AEB0483103BBB4C09@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Mon, 03 Sep 2018 11:45:48 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Mon, 03 Sep 2018 11:45:48 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.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: Mon, 03 Sep 2018 11:45:50 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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 >