Re: [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify func at TPL_CALLBACK

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

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 <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 1/5] MdeModulePkg UsbKbDxe: Execute key notify func at TPL_CALLBACK

Star,
I have one comment:
When call Enque(), can we put comments above the call to say that the key notification function needs to run at TPL_CALLBACK while current TPL is TPL_NOTIFY. It will be called in KeyNotifyProcessHandler() which runs at TPL_CALLBACK?

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 <ruiyu.ni@intel.com>; Kinney, Michael D 
><michael.d.kinney@intel.com>; Tian, Feng <feng.tian@intel.com>; Zeng, 
>Star <star.zeng@intel.com>
>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 <Ruiyu.ni@intel.com>
>Cc: Michael Kinney <michael.d.kinney@intel.com>
>Cc: Feng Tian <feng.tian@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Star Zeng <star.zeng@intel.com>
>---
> 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.<BR>
>+Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
> 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 = 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 != NULL) {
>       gBS->CloseEvent (UsbKeyboardDevice->SimpleInputEx.WaitForKeyEx);
>     }
>+    if (UsbKeyboardDevice->KeyNotifyProcessEvent != NULL) {
>+      gBS->CloseEvent (UsbKeyboardDevice->KeyNotifyProcessEvent);
>+    }
>     if (UsbKeyboardDevice->KeyboardLayoutEvent != 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 function.
>+  @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 = (USB_KB_DEV *) Context;
>+
>+  //
>+  // Invoke notification functions.
>+  //
>+  NotifyList = &UsbKeyboardDevice->NotifyList;  while (!IsQueueEmpty 
>+ (&UsbKeyboardDevice->EfiKeyQueueForNotify)) {
>+    //
>+    // Enter critical section
>+    //
>+    OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
>+    Dequeue (&UsbKeyboardDevice->EfiKeyQueueForNotify, &KeyData, sizeof (KeyData));
>+    //
>+    // Leave critical section
>+    //
>+    gBS->RestoreTPL (OldTpl);
>+    for (Link = GetFirstNode (NotifyList); !IsNull (NotifyList, Link); Link = GetNextNode (NotifyList, Link)) {
>+      CurrentNotify = 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.<BR>
>+Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
> 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 function.
>+  @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 |= EFI_KEY_STATE_EXPOSED;
>   }
>   //
>-  // Invoke notification functions if the key is registered.
>+  // Signal KeyNotify process event if this key pressed matches any key registered.
>   //
>   NotifyList = &UsbKeyboardDevice->NotifyList;
>   for (Link = GetFirstNode (NotifyList); !IsNull (NotifyList, Link); Link = GetNextNode (NotifyList, Link)) {
>     CurrentNotify = 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