[edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Henz, Patrick]

From: Patrick Henz <patrick.henz@hpe.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578

The implementation of XhcGetElapsedTicks did not account for
non-zero start and stop values for the performance counter
timer, potentially resulting in an incorrect elapsed tick
count getting returned to the caller. Account for non-zero
start and stop values when calculating the elapsed tick
count.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
Reviewed-by:
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index f4e61d223c..ff641d0130 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
     // Counter counts upwards, check for an overflow condition
     //
     if (*PreviousTick > CurrentTick) {
-      Delta = (mXhciPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
+      Delta = (CurrentTick - mXhciPerformanceCounterStartValue) + (mXhciPerformanceCounterEndValue - *PreviousTick);
     } else {
       Delta = CurrentTick - *PreviousTick;
     }
@@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
     // Counter counts downwards, check for an underflow condition
     //
     if (*PreviousTick < CurrentTick) {
-      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
+      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mXhciPerformanceCounterEndValue);
     } else {
       Delta = *PreviousTick - CurrentTick;
     }
-- 
2.34.1

Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Laszlo Ersek]

Hi Patrick,

(CC Liming, Mike)

On 2/6/24 16:29, Henz, Patrick wrote:
> From: Patrick Henz <patrick.henz@hpe.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
> 
> The implementation of XhcGetElapsedTicks did not account for
> non-zero start and stop values for the performance counter
> timer, potentially resulting in an incorrect elapsed tick
> count getting returned to the caller. Account for non-zero
> start and stop values when calculating the elapsed tick
> count.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

How does this patch differ from your v1, from October last year:

https://edk2.groups.io/g/devel/message/110434
message-id:
<c3038878c4d30c54e60cce7192cf1aa60c30ad2e.1698770394.git.patrick.henz@hpe.com>

That version was:

Acked-by: Hao A Wu <hao.a.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

If this patch is the same (effectively a repost), then I have two comments:

- the Reviewed-by line on v2 is incomplete, plus the patch is missing
Hao's Acked-by

- I'm really sorry that your v1 patch didn't get merged. I think we
should just apply v1, with the feedback tags given back then. IMO the
patch certainly qualifies for the feature freeze (even the hard one),
because it is a bugfix.

At this development phase, I can't merge your v1, but Liming or Mike
could. Mike, Liming: can you git-am Patrick's v1, and pick up the
feedback tags, for merging?

Thanks!
Laszlo


> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index f4e61d223c..ff641d0130 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>      // Counter counts upwards, check for an overflow condition
>      //
>      if (*PreviousTick > CurrentTick) {
> -      Delta = (mXhciPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> +      Delta = (CurrentTick - mXhciPerformanceCounterStartValue) + (mXhciPerformanceCounterEndValue - *PreviousTick);
>      } else {
>        Delta = CurrentTick - *PreviousTick;
>      }
> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>      // Counter counts downwards, check for an underflow condition
>      //
>      if (*PreviousTick < CurrentTick) {
> -      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> +      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mXhciPerformanceCounterEndValue);
>      } else {
>        Delta = *PreviousTick - CurrentTick;
>      }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115253): https://edk2.groups.io/g/devel/message/115253
Mute This Topic: https://groups.io/mt/104199605/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Henz, Patrick]

Hi Laszlo,

> How does this patch differ from your v1, from October last year:

There's a conflict with my previous changes and those found in commit ff4c49a5ee38d613384fb2e318d891a800d32999, some global variables in Xhci.c were renamed, adding an Xhci prefix. My V2 patch is the same as V1, just with the updated globals.

> I'm really sorry that your v1 patch didn't get merged.

No worries! 

Thanks,
Patrick Henz

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Wednesday, February 7, 2024 3:00 PM
To: devel@edk2.groups.io; Henz, Patrick (IMC Firmware Engineering) <patrick.henz@hpe.com>
Cc: hao.a.wu@intel.com; ray.ni@intel.com; Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft address) <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

Hi Patrick,

(CC Liming, Mike)

On 2/6/24 16:29, Henz, Patrick wrote:
> From: Patrick Henz <patrick.henz@hpe.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
> 
> The implementation of XhcGetElapsedTicks did not account for non-zero 
> start and stop values for the performance counter timer, potentially 
> resulting in an incorrect elapsed tick count getting returned to the 
> caller. Account for non-zero start and stop values when calculating 
> the elapsed tick count.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

How does this patch differ from your v1, from October last year:

https://edk2.groups.io/g/devel/message/110434
message-id:
<c3038878c4d30c54e60cce7192cf1aa60c30ad2e.1698770394.git.patrick.henz@hpe.com>

That version was:

Acked-by: Hao A Wu <hao.a.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

If this patch is the same (effectively a repost), then I have two comments:

- the Reviewed-by line on v2 is incomplete, plus the patch is missing Hao's Acked-by

- I'm really sorry that your v1 patch didn't get merged. I think we should just apply v1, with the feedback tags given back then. IMO the patch certainly qualifies for the feature freeze (even the hard one), because it is a bugfix.

At this development phase, I can't merge your v1, but Liming or Mike could. Mike, Liming: can you git-am Patrick's v1, and pick up the feedback tags, for merging?

Thanks!
Laszlo


> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c 
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index f4e61d223c..ff641d0130 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>      // Counter counts upwards, check for an overflow condition
>      //
>      if (*PreviousTick > CurrentTick) {
> -      Delta = (mXhciPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> +      Delta = (CurrentTick - mXhciPerformanceCounterStartValue) + 
> + (mXhciPerformanceCounterEndValue - *PreviousTick);
>      } else {
>        Delta = CurrentTick - *PreviousTick;
>      }
> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>      // Counter counts downwards, check for an underflow condition
>      //
>      if (*PreviousTick < CurrentTick) {
> -      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> +      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + 
> + (*PreviousTick - mXhciPerformanceCounterEndValue);
>      } else {
>        Delta = *PreviousTick - CurrentTick;
>      }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115256): https://edk2.groups.io/g/devel/message/115256
Mute This Topic: https://groups.io/mt/104199605/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Laszlo Ersek]

On 2/7/24 22:07, Henz, Patrick (IMC Firmware Engineering) wrote:
> Hi Laszlo,
> 
>> How does this patch differ from your v1, from October last year:
> 
> There's a conflict with my previous changes and those found in commit ff4c49a5ee38d613384fb2e318d891a800d32999, some global variables in Xhci.c were renamed, adding an Xhci prefix. My V2 patch is the same as V1, just with the updated globals.
> 
>> I'm really sorry that your v1 patch didn't get merged.
> 
> No worries! 

OK, I guess the feedback tags given for v1 should carry over such a
context change / rename. Do you want to post a v3 with the v1 feedback
tags correctly picked up? (This is a question I could be asking for
Liming's or Ray's sake, because I believe they're supposed to merge the
patch.)

Thanks
Laszlo

> 
> Thanks,
> Patrick Henz
> 
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com> 
> Sent: Wednesday, February 7, 2024 3:00 PM
> To: devel@edk2.groups.io; Henz, Patrick (IMC Firmware Engineering) <patrick.henz@hpe.com>
> Cc: hao.a.wu@intel.com; ray.ni@intel.com; Michael Kinney <michael.d.kinney@intel.com>; Liming Gao (Byosoft address) <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks
> 
> Hi Patrick,
> 
> (CC Liming, Mike)
> 
> On 2/6/24 16:29, Henz, Patrick wrote:
>> From: Patrick Henz <patrick.henz@hpe.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
>>
>> The implementation of XhcGetElapsedTicks did not account for non-zero 
>> start and stop values for the performance counter timer, potentially 
>> resulting in an incorrect elapsed tick count getting returned to the 
>> caller. Account for non-zero start and stop values when calculating 
>> the elapsed tick count.
>>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>> Reviewed-by:
>> ---
>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> How does this patch differ from your v1, from October last year:
> 
> https://edk2.groups.io/g/devel/message/110434
> message-id:
> <c3038878c4d30c54e60cce7192cf1aa60c30ad2e.1698770394.git.patrick.henz@hpe.com>
> 
> That version was:
> 
> Acked-by: Hao A Wu <hao.a.wu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> If this patch is the same (effectively a repost), then I have two comments:
> 
> - the Reviewed-by line on v2 is incomplete, plus the patch is missing Hao's Acked-by
> 
> - I'm really sorry that your v1 patch didn't get merged. I think we should just apply v1, with the feedback tags given back then. IMO the patch certainly qualifies for the feature freeze (even the hard one), because it is a bugfix.
> 
> At this development phase, I can't merge your v1, but Liming or Mike could. Mike, Liming: can you git-am Patrick's v1, and pick up the feedback tags, for merging?
> 
> Thanks!
> Laszlo
> 
> 
>>
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c 
>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> index f4e61d223c..ff641d0130 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>>      // Counter counts upwards, check for an overflow condition
>>      //
>>      if (*PreviousTick > CurrentTick) {
>> -      Delta = (mXhciPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
>> +      Delta = (CurrentTick - mXhciPerformanceCounterStartValue) + 
>> + (mXhciPerformanceCounterEndValue - *PreviousTick);
>>      } else {
>>        Delta = CurrentTick - *PreviousTick;
>>      }
>> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>>      // Counter counts downwards, check for an underflow condition
>>      //
>>      if (*PreviousTick < CurrentTick) {
>> -      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
>> +      Delta = (mXhciPerformanceCounterStartValue - CurrentTick) + 
>> + (*PreviousTick - mXhciPerformanceCounterEndValue);
>>      } else {
>>        Delta = *PreviousTick - CurrentTick;
>>      }
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115292): https://edk2.groups.io/g/devel/message/115292
Mute This Topic: https://groups.io/mt/104199605/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-