[v1] ArmPkg: Arm GIC Library and Driver improvements

[edk2-devel] [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

Posted by Sami Mujawar 1 year, 4 months ago

According to edk2 coding standard specification, Non-Boolean
comparisons must use a compare operator (==, !=, >, < >=, <=).
See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
edk-ii-c-coding-standards-specification/5_source_files/
57_c_programming

Therefore, fix the comparison in ArmGicEnableDistributor()

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
--- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
+++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
@@ -26,7 +26,10 @@ ArmGicEnableDistributor (
   if (Revision == ARM_GIC_ARCH_REVISION_2) {
     MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
   } else {
-    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
+    if ((MmioRead32 (
+           GicDistributorBase + ARM_GIC_ICDDCR
+           ) & ARM_GIC_ICDDCR_ARE) != 0)
+    {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
     } else {
       MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'



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

Re: [edk2-devel] [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

Posted by Ard Biesheuvel 1 year, 4 months ago

On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> According to edk2 coding standard specification, Non-Boolean
> comparisons must use a compare operator (==, !=, >, < >=, <=).
> See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
> edk-ii-c-coding-standards-specification/5_source_files/
> 57_c_programming
>
> Therefore, fix the comparison in ArmGicEnableDistributor()
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
> --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
> @@ -26,7 +26,10 @@ ArmGicEnableDistributor (
>    if (Revision == ARM_GIC_ARCH_REVISION_2) {
>      MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>    } else {
> -    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
> +    if ((MmioRead32 (
> +           GicDistributorBase + ARM_GIC_ICDDCR
> +           ) & ARM_GIC_ICDDCR_ARE) != 0)
> +    {

This coding style is terrible. Mind using a temporary variable for the
result of the MmioRead32() instead?

>        MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
>      } else {
>        MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>


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

Re: [edk2-devel] [PATCH v1 04/12] ArmPkg: Fix Non-Boolean comparison in ArmGicEnableDistributor

Posted by Sami Mujawar 1 year, 4 months ago

Hi Ard,

Thank you for the feedback.

I will fix this in the v2 series.

Regards,

Sami Mujawar

On 23/05/2023 02:32 pm, Ard Biesheuvel wrote:
> On Tue, 23 May 2023 at 15:04, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> According to edk2 coding standard specification, Non-Boolean
>> comparisons must use a compare operator (==, !=, >, < >=, <=).
>> See Section 5.7.2.1 at https://edk2-docs.gitbook.io/
>> edk-ii-c-coding-standards-specification/5_source_files/
>> 57_c_programming
>>
>> Therefore, fix the comparison in ArmGicEnableDistributor()
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>>   ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> index c17cbe041e8a9ceb8c8c3a6b953ff88b75f6f206..1ca66a40940434d6d89e243650f3e81aa3f588b5 100644
>> --- a/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> +++ b/ArmPkg/Drivers/ArmGic/ArmGicNonSecLib.c
>> @@ -26,7 +26,10 @@ ArmGicEnableDistributor (
>>     if (Revision == ARM_GIC_ARCH_REVISION_2) {
>>       MmioWrite32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>>     } else {
>> -    if (MmioRead32 (GicDistributorBase + ARM_GIC_ICDDCR) & ARM_GIC_ICDDCR_ARE) {
>> +    if ((MmioRead32 (
>> +           GicDistributorBase + ARM_GIC_ICDDCR
>> +           ) & ARM_GIC_ICDDCR_ARE) != 0)
>> +    {
> This coding style is terrible. Mind using a temporary variable for the
> result of the MmioRead32() instead?
>
>>         MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x2);
>>       } else {
>>         MmioOr32 (GicDistributorBase + ARM_GIC_ICDDCR, 0x1);
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>


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