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

use small lock to protect register about l2cc in arch "arm" #15430

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

wangzhi-art
Copy link
Contributor

@wangzhi-art wangzhi-art commented Jan 6, 2025

Summary

Use small lock to protect register about l2cc, involving the following files:

arch/arm/src/armv7-a/arm_l2cc_pl310.c
arch/arm/src/armv7-r/arm_l2cc_pl310.c
arch/arm/src/armv8-r/arm_l2cc_pl310.c

Impact

arch/arm/src/armv7-a/arm_l2cc_pl310.c
arch/arm/src/armv7-r/arm_l2cc_pl310.c
arch/arm/src/armv8-r/arm_l2cc_pl310.c

Testing

CI

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Jan 6, 2025
@wangzhi-art wangzhi-art changed the title use small lock to protect register use small lock to protect register in arch "arm" Jan 6, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 6, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes and lists impacted files, it lacks crucial information. Here's what's missing:

  • Summary: Needs to explain why the lock is necessary. What problem does it solve? What was the previous behavior, and why was it incorrect/inefficient/unsafe? How does the small lock mechanism work specifically? Are there any related NuttX issues?
  • Impact: The listed files are the changed files, but the impact section needs to describe the effects of those changes. Specifically address all the points (user impact, build impact, hardware impact, documentation, security, compatibility). For example, does this change improve performance? Does it fix a bug? Does it introduce any new limitations? Just saying "NO" or "YES" isn't sufficient; explain the impact.
  • Testing: "CI" is not enough. Provide actual test logs before and after the change demonstrating the issue being fixed or the new functionality working. What specific tests were run? What platforms were they run on (Build Host and Target details)? What were the results? The logs should clearly show a difference in behavior. If the change is related to performance, provide benchmarks.

Essentially, the PR needs to provide enough information for a reviewer to understand the context, assess the impact, and verify the effectiveness of the changes. The current version is too vague and lacks evidence to support the claim that it "works as intended."

arch/arm/src/armv7-a/arm_l2cc_pl310.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-r/arm_l2cc_pl310.c Outdated Show resolved Hide resolved
@wangzhi-art wangzhi-art changed the title use small lock to protect register in arch "arm" use small lock to protect register about l2cc in arch "arm" Jan 7, 2025
arch/arm/src/armv7-r/arm_l2cc_pl310.c Outdated Show resolved Hide resolved
arch/arm/src/armv7-r/arm_l2cc_pl310.c Outdated Show resolved Hide resolved
arch/arm/src/armv8-r/l2cc.h Outdated Show resolved Hide resolved
arch/arm/src/armv8-r/l2cc.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-r/l2cc.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-r/l2cc.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/l2cc.h Outdated Show resolved Hide resolved
arch/arm/src/armv7-a/l2cc.h Outdated Show resolved Hide resolved
…ng files:

arch/arm/src/armv7-a/arm_l2cc_pl310.c
arch/arm/src/armv7-r/arm_l2cc_pl310.c
arch/arm/src/armv8-r/arm_l2cc_pl310.c

Signed-off-by: wangzhi16 <wangzhi16@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit f179cb8 into apache:master Jan 10, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants