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

gpio: use small lock to protect configgpio #15219

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

hujun260
Copy link
Contributor

Summary

gpio: use small lock to protect configgpio

reason:
We would like to replace the critical section with a small lock.

Impact

gpio

Testing

ci

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium labels Dec 17, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 17, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: It mentions what is being changed (replacing a critical section with a small lock) but not why this is necessary. What problem does this solve? What are the benefits (e.g., performance improvement, reduced interrupt latency)? It also lacks specifics on how the change is implemented. Which functions are modified? Are new functions introduced? Are there any algorithm changes? Finally, issue references are missing.

  • Impact: The single word "gpio" is insufficient. It needs to explicitly answer all the impact questions (user, build, hardware, documentation, security, compatibility) with "YES" or "NO" and provide descriptions where necessary. Even if the answer is "NO," it should still explicitly state "NO." For example, "Impact on user: NO" or "Impact on hardware: YES (affects all platforms using GPIO)."

  • Testing: "ci" is not acceptable. The PR needs to specify the specific build hosts and targets tested. OS, CPU architecture, compiler, board, and configuration details are all required. Critically, it's missing the actual testing logs before and after the change, which are essential for demonstrating that the change works as intended and doesn't introduce regressions.

In short, the PR needs substantially more detail to be acceptable. It should clearly articulate the motivation, implementation, and impact of the change, along with thorough testing evidence.

reason:
We would like to replace the critical section with a small lock.

Signed-off-by: hujun5 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 188a7ce into apache:master Dec 17, 2024
27 checks passed
@anchao
Copy link
Contributor

anchao commented Dec 17, 2024

@hujun260 @xiaoxiang781216

Emm ... Do you think this code works properly?

image

hujun260 added a commit to hujun260/nuttx that referenced this pull request Dec 17, 2024
fix regression from apache#15219

Signed-off-by: hujun5 <[email protected]>
@hujun260
Copy link
Contributor Author

@hujun260 @xiaoxiang781216

Emm ... Do you think this code works properly?

image

I missed some replacements. #15238

anchao pushed a commit that referenced this pull request Dec 17, 2024
fix regression from #15219

Signed-off-by: hujun5 <[email protected]>
@lupyuen
Copy link
Member

lupyuen commented Dec 17, 2024

Sorry @hujun260 wonder if we missed out something for Tiva Spin Lock?
https://github.com/NuttX/nuttx/actions/runs/12370858664/job/34525845581#step:7:595

Configuration/Tool: dk-tm4c129x/ipv6,CONFIG_ARM_TOOLCHAIN_GNU_EABI
chip/tm4c/tm4c_gpio.c: In function 'tiva_configgpio':
Error: chip/tm4c/tm4c_gpio.c:793:11: error: implicit declaration of function 'spin_lock_irqsave' [-Werror=implicit-function-declaration]
  793 |   flags = spin_lock_irqsave(&g_configgpio_lock);
      |           ^~~~~~~~~~~~~~~~~
Error: chip/tm4c/tm4c_gpio.c:844:3: error: implicit declaration of function 'spin_unlock_irqrestore' [-Werror=implicit-function-declaration]
  844 |   spin_unlock_irqrestore(&g_configgpio_lock, flags);
      |   ^~~~~~~~~~~~~~~~~~~~~~

More Tiva Spin Lock errors at NuttX Dashboard:
https://nuttx-dashboard.org/d/fe2bqg6uk7nr4a/build-logs-from-nuttx-build-farm?from=now-2d&to=now&timezone=browser&var-arch=$__all&var-subarch=$__all&var-board=$__all&var-config=$__all&var-group=arm-14&var-Filters=&__cf_chl_tk=iMcyDBki9HdNDQL8aX3EaEb47.pJs79A1d8oQT.A5Pc-1733459484-1.0.1.1-sNb9kVpXOg3kOHZEvTfrf9Z6j_CMoUsQnuwOU31abEE
Screenshot 2024-12-17 at 8 50 02 PM

@hujun260
Copy link
Contributor Author

Sorry @hujun260 wonder if we missed out something for Tiva Spin Lock? https://github.com/NuttX/nuttx/actions/runs/12370858664/job/34525845581#step:7:595

Configuration/Tool: dk-tm4c129x/ipv6,CONFIG_ARM_TOOLCHAIN_GNU_EABI
chip/tm4c/tm4c_gpio.c: In function 'tiva_configgpio':
Error: chip/tm4c/tm4c_gpio.c:793:11: error: implicit declaration of function 'spin_lock_irqsave' [-Werror=implicit-function-declaration]
  793 |   flags = spin_lock_irqsave(&g_configgpio_lock);
      |           ^~~~~~~~~~~~~~~~~
Error: chip/tm4c/tm4c_gpio.c:844:3: error: implicit declaration of function 'spin_unlock_irqrestore' [-Werror=implicit-function-declaration]
  844 |   spin_unlock_irqrestore(&g_configgpio_lock, flags);
      |   ^~~~~~~~~~~~~~~~~~~~~~

More Tiva Spin Lock errors at NuttX Dashboard: https://nuttx-dashboard.org/d/fe2bqg6uk7nr4a/build-logs-from-nuttx-build-farm?from=now-2d&to=now&timezone=browser&var-arch=$__all&var-subarch=$__all&var-board=$__all&var-config=$__all&var-group=arm-14&var-Filters=&__cf_chl_tk=iMcyDBki9HdNDQL8aX3EaEb47.pJs79A1d8oQT.A5Pc-1733459484-1.0.1.1-sNb9kVpXOg3kOHZEvTfrf9Z6j_CMoUsQnuwOU31abEE Screenshot 2024-12-17 at 8 50 02 PM

#15240

jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 18, 2024
fix regression from apache#15219

Signed-off-by: hujun5 <[email protected]>
xiaoxiang781216 pushed a commit that referenced this pull request Dec 18, 2024
fix regression from #15219

Signed-off-by: hujun5 <[email protected]>
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
fix regression from apache#15219

Signed-off-by: hujun5 <[email protected]>
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 Arch: hc Issues related to HC architecture Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-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.

Crtical section should be replaced with spinlock as much as we can to improve SMP performance
5 participants