Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

use small lock in following files

arch/arm/src/efm32/efm32_leserial.c
arch/arm/src/efm32/efm32_serial.c
arch/arm/src/gd32f4/gd32f4xx_serial.c
arch/arm/src/imx9/imx9_edma.c
arch/arm/src/imxrt/imxrt_edma.c
arch/arm/src/imxrt/imxrt_hprtc.c
arch/arm/src/kinetis/kinetis_lpserial.c
arch/arm/src/lc823450/lc823450_gpio.c
arch/arm/src/s32k1xx/s32k1xx_edma.c
arch/arm/src/s32k3xx/s32k3xx_edma.c
arch/arm64/src/imx9/imx9_edma.c

Impact

arch/arm/src/efm32/efm32_leserial.c
arch/arm/src/efm32/efm32_serial.c
arch/arm/src/gd32f4/gd32f4xx_serial.c
arch/arm/src/imx9/imx9_edma.c
arch/arm/src/imxrt/imxrt_edma.c
arch/arm/src/imxrt/imxrt_hprtc.c
arch/arm/src/kinetis/kinetis_lpserial.c
arch/arm/src/lc823450/lc823450_gpio.c
arch/arm/src/s32k1xx/s32k1xx_edma.c
arch/arm/src/s32k3xx/s32k3xx_edma.c
arch/arm64/src/imx9/imx9_edma.c

Testing

ci

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: M The size of the change in this PR is medium labels Dec 23, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 23, 2024

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is the change necessary? Simply stating "use small lock" isn't sufficient. Explain the problem the larger lock was causing (e.g., performance bottleneck, potential deadlock, excessive interrupt latency). What are the benefits of using a smaller lock?
  • What functional part of the code is being changed? Listing files is not enough. Briefly describe the functionality within those files that is affected (e.g., serial driver interrupt handling, DMA transfer completion, GPIO interrupt handling, RTC).
  • How does the change exactly work? Be more specific than "use small lock." What type of smaller lock is being used (e.g., spinlock, mutex)? How is the locking strategy changed? Was the larger lock completely replaced or are there still cases where it's used?
  • Related Issues: Link any related NuttX or NuttX Apps issues.

Missing Information in Impact:

Instead of repeating the file list, address each impact point specifically:

  • New/Changed Feature: NO.
  • Impact on User: Likely NO, but explain why. If interrupt latency is improved, certain real-time applications might benefit, so consider mentioning that.
  • Impact on Build: NO, unless any Kconfig options were added or modified.
  • Impact on Hardware: YES. Specify the affected architectures (ARM, ARM64) and briefly mention the peripherals involved (serial, DMA, GPIO, RTC).
  • Impact on Documentation: Likely NO, but confirm. If the locking strategy is a significant change, documentation might need updating.
  • Impact on Security: Likely NO, but analyze if the change could introduce any vulnerabilities (e.g., race conditions).
  • Impact on Compatibility: Likely NO, but confirm. The change shouldn't break existing applications, but it's good to state that explicitly.

Missing Information in Testing:

  • Build Host(s): Specify the OS, CPU architecture, and compiler used for testing.
  • Target(s): List the specific architectures, boards, and configurations tested. "ci" is not enough. Did CI pass on all relevant platforms?
  • Testing Logs: "ci" isn't sufficient. Include relevant snippets from the logs that demonstrate the improvement (e.g., timing measurements, demonstration of reduced latency). If no quantitative data is available, at least describe the tests performed and their qualitative results.

Example of Improved Summary:

This PR replaces mutex locks with spinlocks in the interrupt handling routines of several drivers (serial, DMA, GPIO, RTC) on ARM and ARM64 architectures to reduce interrupt latency and improve real-time performance. The previous mutex locks could cause higher interrupt latency due to potential context switching. Spinlocks, being non-blocking, address this issue. The locking strategy was modified to acquire the spinlock before accessing shared resources within the interrupt handler and release it afterward. This change is expected to improve responsiveness in interrupt-driven applications. Relates to NuttX Issue #1234 (hypothetical).

By providing this level of detail, reviewers can quickly understand the change, its rationale, and its potential impact. This will significantly improve the review process and increase the likelihood of your PR being merged.

arch/arm/src/efm32/efm32_leserial.c
arch/arm/src/efm32/efm32_serial.c
arch/arm/src/gd32f4/gd32f4xx_serial.c
arch/arm/src/imx9/imx9_edma.c
arch/arm/src/imxrt/imxrt_edma.c
arch/arm/src/imxrt/imxrt_hprtc.c
arch/arm/src/kinetis/kinetis_lpserial.c
arch/arm/src/lc823450/lc823450_gpio.c
arch/arm/src/s32k1xx/s32k1xx_edma.c
arch/arm/src/s32k3xx/s32k3xx_edma.c
arch/arm64/src/imx9/imx9_edma.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 9aa5eda into apache:master Dec 23, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_47 branch February 5, 2025 02:58
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: arm64 Issues related to ARM64 (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.

3 participants