[v1] ArmPkg: GenericWatchdogDxe fixes and improvements

[edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

Posted by Rebecca Cran via groups.io 1 year, 5 months ago

Fix the calculation of the timer period in GenericWatchdogDxe: we need
to multiply before dividing to keep the values as integers.

Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
---
 ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
index 05df101d5f4b..8f02f38c64e3 100644
--- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
+++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
@@ -119,7 +119,7 @@ WatchdogInterruptHandler (
   // the timer period plus 1.
   //
   if (mWatchdogNotify != NULL) {
-    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
     mWatchdogNotify (TimerPeriod + 1);
   }
 
@@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
     return EFI_INVALID_PARAMETER;
   }
 
-  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
+  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
 
   return EFI_SUCCESS;
 }
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113105): https://edk2.groups.io/g/devel/message/113105
Mute This Topic: https://groups.io/mt/103510102/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

Posted by Ard Biesheuvel 1 year, 5 months ago

Hi Rebecca,

On Wed, 3 Jan 2024 at 21:44, Rebecca Cran
<rebecca@os.amperecomputing.com> wrote:
>
> Fix the calculation of the timer period in GenericWatchdogDxe: we need
> to multiply before dividing to keep the values as integers.
>
> Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 05df101d5f4b..8f02f38c64e3 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -119,7 +119,7 @@ WatchdogInterruptHandler (
>    // the timer period plus 1.
>    //
>    if (mWatchdogNotify != NULL) {
> -    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
> +    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);

Could we just store the timer period in a global mTimerPeriod, and get
rid of mNumTimerTicks entirely? AFAICT, that would get rid of these
calculations as well.

>      mWatchdogNotify (TimerPeriod + 1);
>    }
>
> @@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
> +  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
>
>    return EFI_SUCCESS;
>  }
> --
> 2.34.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113108): https://edk2.groups.io/g/devel/message/113108
Mute This Topic: https://groups.io/mt/103510102/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] ArmPkg: Fix the calculation of the timer period in GenericWatchdogDxe

Posted by Rebecca Cran via groups.io 1 year, 5 months ago

Thanks, I've incorporated the changes into the v2 patch series.

-- 
Rebecca

On 1/3/2024 3:56 PM, Ard Biesheuvel wrote:
> Hi Rebecca,
> 
> On Wed, 3 Jan 2024 at 21:44, Rebecca Cran
> <rebecca@os.amperecomputing.com> wrote:
>>
>> Fix the calculation of the timer period in GenericWatchdogDxe: we need
>> to multiply before dividing to keep the values as integers.
>>
>> Signed-off-by: Rebecca Cran <rebecca@os.amperecomputing.com>
>> ---
>>   ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> index 05df101d5f4b..8f02f38c64e3 100644
>> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
>> @@ -119,7 +119,7 @@ WatchdogInterruptHandler (
>>     // the timer period plus 1.
>>     //
>>     if (mWatchdogNotify != NULL) {
>> -    TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
>> +    TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
> 
> Could we just store the timer period in a global mTimerPeriod, and get
> rid of mNumTimerTicks entirely? AFAICT, that would get rid of these
> calculations as well.
> 
>>       mWatchdogNotify (TimerPeriod + 1);
>>     }
>>
>> @@ -260,7 +260,7 @@ WatchdogGetTimerPeriod (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>
>> -  *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks);
>> +  *TimerPeriod = ((TIME_UNITS_PER_SECOND * mNumTimerTicks) / mTimerFrequencyHz);
>>
>>     return EFI_SUCCESS;
>>   }
>> --
>> 2.34.1
>>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113214): https://edk2.groups.io/g/devel/message/113214
Mute This Topic: https://groups.io/mt/103510102/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-