[PATCH v2 0/2] PlatformBootManagerLib: Don't update progress if Pcd is 0

[PATCH v2 0/2] PlatformBootManagerLib: Don't update progress if Pcd is 0

[Pete Batard]

Changes from previous version:
- Additional empty comment lines above and below, to keep with existing comment
  style
- Similar logic is applied to ArmVirtPkg/PlatformBootManagerLib, since the same
  issue may manifest itself here.

Pete Batard (2):
  OvmfPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0
  ArmVirtPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0

 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 14 +++++++++++---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 15 ++++++++++++---
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.21.0.windows.1

[PATCH v2 1/2] OvmfPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0

[Pete Batard]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2266

Independently of how we decide to address other aspects of the regression
introduced with commit 2de1f611be06ded3a59726a4052a9039be7d459b, it doesn't
make much sense to call for a progress update if PcdPlatformBootTimeOut is
zero.

PcdPlatformBootTimeOut 0, which is the cause of the bug (division by zero)
should be considered to indicate that a platform is not interested in
displaying a progress report, so we alter PlatformBootManagerWaitCallback
to behave that way.

We also change one variable name to make the code more explicit.

Signed-off-by: Pete Batard <pete@akeo.ie>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 70df6b841acc..8af9b71f18a3 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -1631,9 +1631,18 @@ PlatformBootManagerWaitCallback (
 {
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION Black;
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION White;
-  UINT16                              Timeout;
+  UINT16                              TimeoutInitial;
 
-  Timeout = PcdGet16 (PcdPlatformBootTimeOut);
+  TimeoutInitial = PcdGet16 (PcdPlatformBootTimeOut);
+
+  //
+  // If PcdPlatformBootTimeOut is set to zero, then we consider
+  // that no progress update should be enacted (since we'd only
+  // ever display a one-shot progress of either 0% or 100%).
+  //
+  if (TimeoutInitial == 0) {
+    return;
+  }
 
   Black.Raw = 0x00000000;
   White.Raw = 0x00FFFFFF;
@@ -1643,7 +1652,7 @@ PlatformBootManagerWaitCallback (
     Black.Pixel,
     L"Start boot option",
     White.Pixel,
-    (Timeout - TimeoutRemain) * 100 / Timeout,
+    (TimeoutInitial - TimeoutRemain) * 100 / TimeoutInitial,
     0
     );
 }
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH v2 1/2] OvmfPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0

[Philippe Mathieu-Daudé]

On 10/14/19 5:03 PM, Pete Batard wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2266
> 
> Independently of how we decide to address other aspects of the regression
> introduced with commit 2de1f611be06ded3a59726a4052a9039be7d459b, it doesn't
> make much sense to call for a progress update if PcdPlatformBootTimeOut is
> zero.
> 
> PcdPlatformBootTimeOut 0, which is the cause of the bug (division by zero)
> should be considered to indicate that a platform is not interested in
> displaying a progress report, so we alter PlatformBootManagerWaitCallback
> to behave that way.
> 
> We also change one variable name to make the code more explicit.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>

You can keep:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 70df6b841acc..8af9b71f18a3 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -1631,9 +1631,18 @@ PlatformBootManagerWaitCallback (
>   {
>     EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION Black;
>     EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION White;
> -  UINT16                              Timeout;
> +  UINT16                              TimeoutInitial;
>   
> -  Timeout = PcdGet16 (PcdPlatformBootTimeOut);
> +  TimeoutInitial = PcdGet16 (PcdPlatformBootTimeOut);
> +
> +  //
> +  // If PcdPlatformBootTimeOut is set to zero, then we consider
> +  // that no progress update should be enacted (since we'd only
> +  // ever display a one-shot progress of either 0% or 100%).
> +  //
> +  if (TimeoutInitial == 0) {
> +    return;
> +  }
>   
>     Black.Raw = 0x00000000;
>     White.Raw = 0x00FFFFFF;
> @@ -1643,7 +1652,7 @@ PlatformBootManagerWaitCallback (
>       Black.Pixel,
>       L"Start boot option",
>       White.Pixel,
> -    (Timeout - TimeoutRemain) * 100 / Timeout,
> +    (TimeoutInitial - TimeoutRemain) * 100 / TimeoutInitial,
>       0
>       );
>   }
> 

[PATCH v2 2/2] ArmVirtPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0

[Pete Batard]

Similar to what we now do for OVMF, we need to consider the possibility
that PlatformBootManagerWaitCallback () may be called with a
PcdPlatformBootTimeOut that was set to zero, in which case the call should
simply return.

We also change the initial timeout variable name to make the code explicit.

Signed-off-by: Pete Batard <pete@akeo.ie>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 30c015eec5b0..5f6cfe64daca 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -842,9 +842,17 @@ PlatformBootManagerWaitCallback (
 {
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION Black;
   EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION White;
-  UINT16                              Timeout;
+  UINT16                              TimeoutInitial;
 
-  Timeout = PcdGet16 (PcdPlatformBootTimeOut);
+  TimeoutInitial = PcdGet16 (PcdPlatformBootTimeOut);
+
+  //
+  // If PcdPlatformBootTimeOut is set to zero, then we consider
+  // that no progress update should be enacted.
+  //
+  if (TimeoutInitial == 0) {
+    return;
+  }
 
   Black.Raw = 0x00000000;
   White.Raw = 0x00FFFFFF;
@@ -854,7 +862,7 @@ PlatformBootManagerWaitCallback (
     Black.Pixel,
     L"Start boot option",
     White.Pixel,
-    (Timeout - TimeoutRemain) * 100 / Timeout,
+    (TimeoutInitial - TimeoutRemain) * 100 / TimeoutInitial,
     0
     );
 }
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH v2 2/2] ArmVirtPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0

[Philippe Mathieu-Daudé]

On 10/14/19 5:03 PM, Pete Batard wrote:
> Similar to what we now do for OVMF, we need to consider the possibility
> that PlatformBootManagerWaitCallback () may be called with a
> PcdPlatformBootTimeOut that was set to zero, in which case the call should
> simply return.

Oh I forgot this one, good catch.

> We also change the initial timeout variable name to make the code explicit.
> 
> Signed-off-by: Pete Batard <pete@akeo.ie>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 30c015eec5b0..5f6cfe64daca 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -842,9 +842,17 @@ PlatformBootManagerWaitCallback (
>   {
>     EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION Black;
>     EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION White;
> -  UINT16                              Timeout;
> +  UINT16                              TimeoutInitial;
>   
> -  Timeout = PcdGet16 (PcdPlatformBootTimeOut);
> +  TimeoutInitial = PcdGet16 (PcdPlatformBootTimeOut);
> +
> +  //
> +  // If PcdPlatformBootTimeOut is set to zero, then we consider
> +  // that no progress update should be enacted.
> +  //
> +  if (TimeoutInitial == 0) {
> +    return;
> +  }
>   
>     Black.Raw = 0x00000000;
>     White.Raw = 0x00FFFFFF;
> @@ -854,7 +862,7 @@ PlatformBootManagerWaitCallback (
>       Black.Pixel,
>       L"Start boot option",
>       White.Pixel,
> -    (Timeout - TimeoutRemain) * 100 / Timeout,
> +    (TimeoutInitial - TimeoutRemain) * 100 / TimeoutInitial,
>       0
>       );
>   }
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v2 0/2] PlatformBootManagerLib: Don't update progress if Pcd is 0

[Laszlo Ersek]

On 10/14/19 17:03, Pete Batard wrote:
> Changes from previous version:
> - Additional empty comment lines above and below, to keep with existing comment
>   style
> - Similar logic is applied to ArmVirtPkg/PlatformBootManagerLib, since the same
>   issue may manifest itself here.
> 
> Pete Batard (2):
>   OvmfPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0
>   ArmVirtPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0
> 
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 14 +++++++++++---
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 15 ++++++++++++---
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 

Thanks. I'll add the BZ reference to patch#2 too, and then push the
series tomorrow (with feedback tags from Phil and others picked up, of
course). Every set should spend 24h on the list, before being pushed.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 0/2] PlatformBootManagerLib: Don't update progress if Pcd is 0

[Laszlo Ersek]

On 10/14/19 20:36, Laszlo Ersek wrote:
> On 10/14/19 17:03, Pete Batard wrote:
>> Changes from previous version:
>> - Additional empty comment lines above and below, to keep with existing comment
>>   style
>> - Similar logic is applied to ArmVirtPkg/PlatformBootManagerLib, since the same
>>   issue may manifest itself here.
>>
>> Pete Batard (2):
>>   OvmfPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0
>>   ArmVirtPkg/PlatformBootManagerLib: Don't update progress if Pcd is 0
>>
>>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 14 +++++++++++---
>>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c   | 15 ++++++++++++---
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
> 
> Thanks. I'll add the BZ reference to patch#2 too, and then push the
> series tomorrow (with feedback tags from Phil and others picked up, of
> course). Every set should spend 24h on the list, before being pushed.

Pushed as commit range cd70b1a71d30..23ab8df01a2c.

Thanks!
Laszlo