Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arch: coding guidelines: use bool param when data nature is boolean #72675

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DeHess
Copy link
Collaborator

@DeHess DeHess commented May 13, 2024

converted arch buffer validation param "write" to boolean because this param is used as a boolean in the method.

This PR is part of the enhancement issue #48002 which port the coding guideline fixes done by BUGSENG on the https://github.com/zephyrproject-rtos/zephyr/tree/v2.7-auditable-branch back to main

The commit in this PR is a subset of the original auditable-branch commit:
d03fa8d

@DeHess DeHess marked this pull request as draft May 13, 2024 10:05
@zephyrbot zephyrbot added area: X86 x86 Architecture (32-bit) area: Xtensa Xtensa Architecture platform: NXP Drivers NXP Semiconductors, drivers area: ARM ARM (32-bit) Architecture area: ARC ARC Architecture area: RISCV RISCV Architecture (32-bit & 64-bit) area: ARM64 ARM (64-bit) Architecture area: Architectures labels May 13, 2024
Copy link
Collaborator

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the change in the Arm part.
However, the doc for the API should most likely be updated to be consistent with the new data type.

converted buffer validation param "write" to boolean
because this param is used as a boolean.

Signed-off-by: Hess Nathan <nhess@baumer.com>
@DeHess DeHess force-pushed the misra_rules_essential_types_auditable_to_main_arch branch from aa08664 to c521d23 Compare May 13, 2024 10:38
@DeHess
Copy link
Collaborator Author

DeHess commented May 13, 2024

@ithinuel

You're right, updated the doc accordingly

@DeHess DeHess marked this pull request as ready for review May 13, 2024 12:42
@DeHess DeHess requested a review from ithinuel May 13, 2024 12:45
@@ -207,7 +207,7 @@ int arc_core_mpu_get_max_domain_partition_regions(void)
/**
* @brief validate the given buffer is user accessible or not
*/
int arc_core_mpu_buffer_validate(const void *addr, size_t size, int write)
int arc_core_mpu_buffer_validate(const void *addr, size_t size, bool write)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function calls _is_user_accessible_region, shouldn't that be updated as well?

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... this is an ancient function whose app-side responsibility has largely migrated to the mem_domain APIs. We don't actually call it anywhere in the tree except tests.

Might be worth considering removing it instead of needlessly churning its API...

@DeHess
Copy link
Collaborator Author

DeHess commented May 14, 2024

Hm... this is an ancient function whose app-side responsibility has largely migrated to the mem_domain APIs. We don't actually call it anywhere in the tree except tests.

Might be worth considering removing it instead of needlessly churning its API...

@andyross
I would love to remove these functions, but there is one place where it is called outside of tests:
syscall_handler.h
line 430

How would I deal with that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARC ARC Architecture area: Architectures area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Coding Guidelines Coding guidelines and style area: RISCV RISCV Architecture (32-bit & 64-bit) area: X86 x86 Architecture (32-bit) area: Xtensa Xtensa Architecture platform: NXP Drivers NXP Semiconductors, drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants