public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
@ 2018-04-13  4:13 Ruiyu Ni
  2018-04-13  5:15 ` Zeng, Star
  0 siblings, 1 reply; 5+ messages in thread
From: Ruiyu Ni @ 2018-04-13  4:13 UTC (permalink / raw)
  To: edk2-devel; +Cc: Steven Shi, Star Zeng

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 60de2d4d6d..66dd3ad550 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -2,7 +2,7 @@
   Produces Simple Text Input Protocol, Simple Text Input Extended Protocol and
   Simple Text Output Protocol upon Serial IO Protocol.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, 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
@@ -488,6 +488,9 @@ TerminalDriverBindingStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
+    return Status;
+  }
 
   //
   // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
@@ -501,6 +504,9 @@ TerminalDriverBindingStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
+    return Status;
+  }
 
   if (!IsHotPlugDevice (ParentDevicePath)) {
     //
-- 
2.14.1.windows.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
  2018-04-13  4:13 [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart Ruiyu Ni
@ 2018-04-13  5:15 ` Zeng, Star
  2018-04-13  8:59   ` Laszlo Ersek
  2018-04-18  0:55   ` Ni, Ruiyu
  0 siblings, 2 replies; 5+ messages in thread
From: Zeng, Star @ 2018-04-13  5:15 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Shi, Steven, Zeng, Star

Generally, I am ok with the change.
But, what is the case for the change?
If the new "if condition" is TRUE, the ASSERT above it will happen, so the new "if condition" is for RELEASE build case?
And if the code passes Support() check, then seemingly, the "if condition" in Start() will not happen.
Could more detailed information in the commit log or code comments?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Friday, April 13, 2018 12:14 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Steven Shi <steven.shi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 60de2d4d6d..66dd3ad550 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -2,7 +2,7 @@
   Produces Simple Text Input Protocol, Simple Text Input Extended Protocol and
   Simple Text Output Protocol upon Serial IO Protocol.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, 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 @@ -488,6 +488,9 @@ TerminalDriverBindingStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
+    return Status;
+  }
 
   //
   // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
@@ -501,6 +504,9 @@ TerminalDriverBindingStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
+    return Status;
+  }
 
   if (!IsHotPlugDevice (ParentDevicePath)) {
     //
--
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
  2018-04-13  5:15 ` Zeng, Star
@ 2018-04-13  8:59   ` Laszlo Ersek
  2018-04-13 10:09     ` Zeng, Star
  2018-04-18  0:55   ` Ni, Ruiyu
  1 sibling, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2018-04-13  8:59 UTC (permalink / raw)
  To: Zeng, Star, Ni, Ruiyu, edk2-devel@lists.01.org

Hi Star,

On 04/13/18 07:15, Zeng, Star wrote:
> Generally, I am ok with the change.
> But, what is the case for the change?
> If the new "if condition" is TRUE, the ASSERT above it will happen, so the new "if condition" is for RELEASE build case?
> And if the code passes Support() check, then seemingly, the "if condition" in Start() will not happen.
> Could more detailed information in the commit log or code comments?

I believe this patch is for
<https://bugzilla.tianocore.org/show_bug.cgi?id=917>. It seems to be for
the "input Controller handle is invalid" case.

Comment 0 in that BZ describes the issue, but I agree the commit message
should at least reference the BZ.

Thanks
Laszlo


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
> Sent: Friday, April 13, 2018 12:14 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> index 60de2d4d6d..66dd3ad550 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> @@ -2,7 +2,7 @@
>    Produces Simple Text Input Protocol, Simple Text Input Extended Protocol and
>    Simple Text Output Protocol upon Serial IO Protocol.
>  
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, 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 @@ -488,6 +488,9 @@ TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
>  
>    //
>    // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
> @@ -501,6 +504,9 @@ TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
>  
>    if (!IsHotPlugDevice (ParentDevicePath)) {
>      //
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
  2018-04-13  8:59   ` Laszlo Ersek
@ 2018-04-13 10:09     ` Zeng, Star
  0 siblings, 0 replies; 5+ messages in thread
From: Zeng, Star @ 2018-04-13 10:09 UTC (permalink / raw)
  To: Laszlo Ersek, Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Zeng, Star

Thanks Laszlo.

If the commit log is updated to reference the BZ link or even add more detailed information.
Reviewed-by: Star Zeng <star.zeng@intel.com>


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, April 13, 2018 5:00 PM
To: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart

Hi Star,

On 04/13/18 07:15, Zeng, Star wrote:
> Generally, I am ok with the change.
> But, what is the case for the change?
> If the new "if condition" is TRUE, the ASSERT above it will happen, so the new "if condition" is for RELEASE build case?
> And if the code passes Support() check, then seemingly, the "if condition" in Start() will not happen.
> Could more detailed information in the commit log or code comments?

I believe this patch is for
<https://bugzilla.tianocore.org/show_bug.cgi?id=917>. It seems to be for the "input Controller handle is invalid" case.

Comment 0 in that BZ describes the issue, but I agree the commit message should at least reference the BZ.

Thanks
Laszlo


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ruiyu Ni
> Sent: Friday, April 13, 2018 12:14 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of 
> OpenProtocol in BindingStart
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c 
> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> index 60de2d4d6d..66dd3ad550 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> @@ -2,7 +2,7 @@
>    Produces Simple Text Input Protocol, Simple Text Input Extended Protocol and
>    Simple Text Output Protocol upon Serial IO Protocol.
>  
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights 
> reserved.<BR>
> +Copyright (c) 2006 - 2018, 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 @@ -488,6 +488,9 @@ TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == 
> EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
>  
>    //
>    // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
> @@ -501,6 +504,9 @@ TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == 
> EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
>  
>    if (!IsHotPlugDevice (ParentDevicePath)) {
>      //
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart
  2018-04-13  5:15 ` Zeng, Star
  2018-04-13  8:59   ` Laszlo Ersek
@ 2018-04-18  0:55   ` Ni, Ruiyu
  1 sibling, 0 replies; 5+ messages in thread
From: Ni, Ruiyu @ 2018-04-18  0:55 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org

Indeed UEFI Spec requires that Supported() should be called before Start().
But Steven's test cases directly call Start() which exposes this "issue".
I don't want to argue the call doesn't follow UEFI Spec.
Fixing it doesn't harm:).


> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, April 13, 2018 1:15 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Shi, Steven <steven.shi@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of
> OpenProtocol in BindingStart
> 
> Generally, I am ok with the change.
> But, what is the case for the change?
> If the new "if condition" is TRUE, the ASSERT above it will happen, so the new "if
> condition" is for RELEASE build case?
> And if the code passes Support() check, then seemingly, the "if condition" in
> Start() will not happen.
> Could more detailed information in the commit log or code comments?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Friday, April 13, 2018 12:14 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH] MdeModulePkg/Terminal: Check status of
> OpenProtocol in BindingStart
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> index 60de2d4d6d..66dd3ad550 100644
> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
> @@ -2,7 +2,7 @@
>    Produces Simple Text Input Protocol, Simple Text Input Extended Protocol and
>    Simple Text Output Protocol upon Serial IO Protocol.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, 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 @@ -488,6 +488,9 @@
> TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
> 
>    //
>    // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
> @@ -501,6 +504,9 @@ TerminalDriverBindingStart (
>                    EFI_OPEN_PROTOCOL_BY_DRIVER
>                    );
>    ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
> +  if (EFI_ERROR (Status) && (Status != EFI_ALREADY_STARTED)) {
> +    return Status;
> +  }
> 
>    if (!IsHotPlugDevice (ParentDevicePath)) {
>      //
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-18  0:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-13  4:13 [PATCH] MdeModulePkg/Terminal: Check status of OpenProtocol in BindingStart Ruiyu Ni
2018-04-13  5:15 ` Zeng, Star
2018-04-13  8:59   ` Laszlo Ersek
2018-04-13 10:09     ` Zeng, Star
2018-04-18  0:55   ` Ni, Ruiyu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox