From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 BB21982420 for ; Thu, 22 Dec 2016 23:28:32 -0800 (PST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 22 Dec 2016 23:28:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,392,1477983600"; d="scan'208,217";a="1103332060" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 22 Dec 2016 23:28:32 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 22 Dec 2016 23:28:32 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 22 Dec 2016 23:28:31 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Fri, 23 Dec 2016 15:28:29 +0800 From: "Zeng, Star" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Tian, Feng" , "Zeng, Star" Thread-Topic: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify func at TPL_CALLBACK Thread-Index: AQHSXFmoN7Pk2EWeqUWuL2JIoJiER6EUldwAgACHECD//3tNAIAAilsg Date: Fri, 23 Dec 2016 07:28:28 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B7EF792@shsmsx102.ccr.corp.intel.com> References: <1482414310-33036-1-git-send-email-star.zeng@intel.com> <1482414310-33036-2-git-send-email-star.zeng@intel.com> <734D49CCEBEEF84792F5B80ED585239D5B86226D@SHSMSX104.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B7EF748@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5B8622C1@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5B8622C1@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify func at TPL_CALLBACK X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Dec 2016 07:28:32 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ray, Yes, it is a problem. After discussion with you, we can eliminate the call to IsQueueEmpty() as D= equeue() has included the logic of IsQueueEmpty() and return status accordi= ngly. I will resend the V2 patch series. Thanks again for the comments. :) Star From: Ni, Ruiyu Sent: Friday, December 23, 2016 3:10 PM To: Zeng, Star ; edk2-devel@lists.01.org Cc: Kinney, Michael D ; Tian, Feng Subject: RE: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify f= unc at TPL_CALLBACK Star, Another comment: I think we need to use TPL_NOTIFY to guard the call to IsQ= ueueEmpty(). To simplify the code, how about create a EFI_LOCK (TPL_NOTIFY) to guard the= access to EfiKeyQueueForNotify? EfiAcquireLock() can be used to replace the RaiseTPL(). Regards, Ray From: Zeng, Star Sent: Friday, December 23, 2016 3:05 PM To: Ni, Ruiyu >; edk2-devel@l= ists.01.org Cc: Kinney, Michael D >; Tian, Feng >; Z= eng, Star > Subject: RE: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify f= unc at TPL_CALLBACK Ray, Good comment, it can make the code more clear, I agree it. Thanks, Star -----Original Message----- From: Ni, Ruiyu Sent: Friday, December 23, 2016 3:01 PM To: Zeng, Star >; edk2-deve= l@lists.01.org Cc: Kinney, Michael D >; Tian, Feng >; Z= eng, Star > Subject: RE: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify f= unc at TPL_CALLBACK Star, I have one comment: When call Enque(), can we put comments above the call to say that the key n= otification function needs to run at TPL_CALLBACK while current TPL is TPL_= NOTIFY. It will be called in KeyNotifyProcessHandler() which runs at TPL_CA= LLBACK? Regards, Ray >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >Star Zeng >Sent: Thursday, December 22, 2016 9:45 PM >To: edk2-devel@lists.01.org >Cc: Ni, Ruiyu >; Kinney, Mic= hael D >>; Tian, Fen= g >; Zeng, >Star > >Subject: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify >func at TPL_CALLBACK > >Current implementation executes key notify function in TimerHandler at >TPL_NOTIFY. The code change is to make key notify function executed at >TPL_CALLBACK to reduce the time occupied at TPL_NOTIFY. > >The code will signal KeyNotify process event if the key pressed matches >any key registered and insert the KeyData to the EFI Key queue for >notify, then the KeyNotify process handler will invoke key notify >functions at TPL_CALLBACK. > >Cc: Ruiyu Ni > >Cc: Michael Kinney > >Cc: Feng Tian > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Star Zeng > >--- > MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.c | 64 +++++++++++++++++++++++++++= ++++- > MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.h | 17 ++++++++- > MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 10 +++-- > 3 files changed, 86 insertions(+), 5 deletions(-) > >diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.c >b/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.c >index fb7558b73070..b1e1657173c3 100644 >--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.c >+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.c >@@ -2,7 +2,7 @@ > USB Keyboard Driver that manages USB keyboard and produces Simple Text = Input > Protocol and Simple Text Input Ex Protocol. > >-Copyright (c) 2004 - 2012, Intel Corporation. All rights reserved.
>+Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
> This program and the accompanying materials are licensed and made >available under the terms and conditions of the BSD License which >accompanies this distribution. The full text of the license may be >found at @@ -312,6 +312,17 @@ USBKeyboardDriverBindingStart ( > goto ErrorExit; > } > >+ Status =3D gBS->CreateEvent ( >+ EVT_NOTIFY_SIGNAL, >+ TPL_CALLBACK, >+ KeyNotifyProcessHandler, >+ UsbKeyboardDevice, >+ &UsbKeyboardDevice->KeyNotifyProcessEvent >+ ); >+ if (EFI_ERROR (Status)) { >+ goto ErrorExit; >+ } >+ > // > // Install Simple Text Input Protocol and Simple Text Input Ex Protocol > // for the USB keyboard device. >@@ -427,6 +438,9 @@ ErrorExit: > if (UsbKeyboardDevice->SimpleInputEx.WaitForKeyEx !=3D NULL) { > gBS->CloseEvent (UsbKeyboardDevice->SimpleInputEx.WaitForKeyEx); > } >+ if (UsbKeyboardDevice->KeyNotifyProcessEvent !=3D NULL) { >+ gBS->CloseEvent (UsbKeyboardDevice->KeyNotifyProcessEvent); >+ } > if (UsbKeyboardDevice->KeyboardLayoutEvent !=3D NULL) { > ReleaseKeyboardLayoutResources (UsbKeyboardDevice); > gBS->CloseEvent (UsbKeyboardDevice->KeyboardLayoutEvent); >@@ -548,6 +562,7 @@ USBKeyboardDriverBindingStop ( > gBS->CloseEvent (UsbKeyboardDevice->DelayedRecoveryEvent); > gBS->CloseEvent (UsbKeyboardDevice->SimpleInput.WaitForKey); > gBS->CloseEvent (UsbKeyboardDevice->SimpleInputEx.WaitForKeyEx); >+ gBS->CloseEvent (UsbKeyboardDevice->KeyNotifyProcessEvent); > KbdFreeNotifyList (&UsbKeyboardDevice->NotifyList); > > ReleaseKeyboardLayoutResources (UsbKeyboardDevice); @@ -559,6 +574,7 >@@ USBKeyboardDriverBindingStop ( > > DestroyQueue (&UsbKeyboardDevice->UsbKeyQueue); > DestroyQueue (&UsbKeyboardDevice->EfiKeyQueue); >+ DestroyQueue (&UsbKeyboardDevice->EfiKeyQueueForNotify); > > FreePool (UsbKeyboardDevice); > >@@ -647,6 +663,7 @@ USBKeyboardReset ( > // > InitQueue (&UsbKeyboardDevice->UsbKeyQueue, sizeof (USB_KEY)); > InitQueue (&UsbKeyboardDevice->EfiKeyQueue, sizeof >(EFI_KEY_DATA)); >+ InitQueue (&UsbKeyboardDevice->EfiKeyQueueForNotify, sizeof >+ (EFI_KEY_DATA)); > > return EFI_SUCCESS; > } >@@ -1170,3 +1187,48 @@ USBKeyboardUnregisterKeyNotify ( > return EFI_INVALID_PARAMETER; > } > >+/** >+ Process key notify. >+ >+ @param Event Indicates the event that invoke this func= tion. >+ @param Context Indicates the calling context. >+**/ >+VOID >+EFIAPI >+KeyNotifyProcessHandler ( >+ IN EFI_EVENT Event, >+ IN VOID *Context >+ ) >+{ >+ USB_KB_DEV *UsbKeyboardDevice; >+ EFI_KEY_DATA KeyData; >+ LIST_ENTRY *Link; >+ LIST_ENTRY *NotifyList; >+ KEYBOARD_CONSOLE_IN_EX_NOTIFY *CurrentNotify; >+ EFI_TPL OldTpl; >+ >+ UsbKeyboardDevice =3D (USB_KB_DEV *) Context; >+ >+ // >+ // Invoke notification functions. >+ // >+ NotifyList =3D &UsbKeyboardDevice->NotifyList; while (!IsQueueEmpty >+ (&UsbKeyboardDevice->EfiKeyQueueForNotify)) { >+ // >+ // Enter critical section >+ // >+ OldTpl =3D gBS->RaiseTPL (TPL_NOTIFY); >+ Dequeue (&UsbKeyboardDevice->EfiKeyQueueForNotify, &KeyData, sizeof (= KeyData)); >+ // >+ // Leave critical section >+ // >+ gBS->RestoreTPL (OldTpl); >+ for (Link =3D GetFirstNode (NotifyList); !IsNull (NotifyList, Link); = Link =3D GetNextNode (NotifyList, Link)) { >+ CurrentNotify =3D CR (Link, KEYBOARD_CONSOLE_IN_EX_NOTIFY, >+ NotifyEntry, >USB_KB_CONSOLE_IN_EX_NOTIFY_SIGNATURE); >+ if (IsKeyRegistered (&CurrentNotify->KeyData, &KeyData)) { >+ CurrentNotify->KeyNotificationFn (&KeyData); >+ } >+ } >+ } >+} >+ >diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.h >b/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.h >index 58edb3f65f2b..089f113d5fd4 100644 >--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.h >+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/EfiKey.h >@@ -1,7 +1,7 @@ > /** @file > Header file for USB Keyboard Driver's Data Structures. > >-Copyright (c) 2004 - 2012, Intel Corporation. All rights reserved.
>+Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
> This program and the accompanying materials are licensed and made >available under the terms and conditions of the BSD License which >accompanies this distribution. The full text of the license may be >found at @@ -114,6 +114,7 @@ typedef struct { > > USB_SIMPLE_QUEUE UsbKeyQueue; > USB_SIMPLE_QUEUE EfiKeyQueue; >+ USB_SIMPLE_QUEUE EfiKeyQueueForNotify; > BOOLEAN CtrlOn; > BOOLEAN AltOn; > BOOLEAN ShiftOn; >@@ -149,6 +150,7 @@ typedef struct { > // Notification function list > // > LIST_ENTRY NotifyList; >+ EFI_EVENT KeyNotifyProcessEvent; > > // > // Non-spacing key list >@@ -596,5 +598,18 @@ USBKeyboardTimerHandler ( > IN VOID *Context > ); > >+/** >+ Process key notify. >+ >+ @param Event Indicates the event that invoke this func= tion. >+ @param Context Indicates the calling context. >+**/ >+VOID >+EFIAPI >+KeyNotifyProcessHandler ( >+ IN EFI_EVENT Event, >+ IN VOID *Context >+ ); >+ > #endif > >diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c >b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c >index fef1449e3c35..a017d51e3b97 100644 >--- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c >+++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c >@@ -820,6 +820,7 @@ InitUSBKeyboard ( > > InitQueue (&UsbKeyboardDevice->UsbKeyQueue, sizeof (USB_KEY)); > InitQueue (&UsbKeyboardDevice->EfiKeyQueue, sizeof (EFI_KEY_DATA)); >+ InitQueue (&UsbKeyboardDevice->EfiKeyQueueForNotify, sizeof >+ (EFI_KEY_DATA)); > > // > // Use the config out of the descriptor @@ -1665,20 +1666,23 @@ >UsbKeyCodeToEfiInputKey ( > KeyData->KeyState.KeyToggleState |=3D EFI_KEY_STATE_EXPOSED; > } > // >- // Invoke notification functions if the key is registered. >+ // Signal KeyNotify process event if this key pressed matches any key r= egistered. > // > NotifyList =3D &UsbKeyboardDevice->NotifyList; > for (Link =3D GetFirstNode (NotifyList); !IsNull (NotifyList, Link); Li= nk =3D GetNextNode (NotifyList, Link)) { > CurrentNotify =3D CR (Link, KEYBOARD_CONSOLE_IN_EX_NOTIFY, >NotifyEntry, USB_KB_CONSOLE_IN_EX_NOTIFY_SIGNATURE); > if (IsKeyRegistered (&CurrentNotify->KeyData, KeyData)) { >- CurrentNotify->KeyNotificationFn (KeyData); >+ // >+ // Insert to the EFI Key queue for notify. >+ // >+ Enqueue (&UsbKeyboardDevice->EfiKeyQueueForNotify, KeyData, sizeof = (*KeyData)); >+ gBS->SignalEvent (UsbKeyboardDevice->KeyNotifyProcessEvent); > } > } > > return EFI_SUCCESS; > } > >- > /** > Create the queue. > >-- >2.7.0.windows.1 > >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel