Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

use small lock in following files:

arch/arm/src/tiva/common/tiva_hciuart.c
arch/arm/src/tms570/tms570_lowputc.c
arch/arm/src/xmc4/xmc4_serial.c
arch/arm64/src/a64/a64_serial.c
arch/mips/src/pic32mx/pic32mx_serial.c
arch/mips/src/pic32mz/pic32mz_serial.c
arch/risc-v/src/c906/c906_serial.c
arch/risc-v/src/fe310/fe310_serial.c
arch/risc-v/src/hpm6750/hpm6750_serial.c
arch/risc-v/src/k210/k210_serial.c
arch/sparc/src/bm3803/bm3803-serial.c
arch/sparc/src/bm3823/bm3823-serial.c

Impact

arch/arm/src/tiva/common/tiva_hciuart.c
arch/arm/src/tms570/tms570_lowputc.c
arch/arm/src/xmc4/xmc4_serial.c
arch/arm64/src/a64/a64_serial.c
arch/mips/src/pic32mx/pic32mx_serial.c
arch/mips/src/pic32mz/pic32mz_serial.c
arch/risc-v/src/c906/c906_serial.c
arch/risc-v/src/fe310/fe310_serial.c
arch/risc-v/src/hpm6750/hpm6750_serial.c
arch/risc-v/src/k210/k210_serial.c
arch/sparc/src/bm3803/bm3803-serial.c
arch/sparc/src/bm3823/bm3823-serial.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 Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: sparc Issues related to the SPARC 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]

This PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information:

  • Summary: While it lists the files changed, it lacks crucial details. It doesn't explain why using a smaller lock is necessary. What problem does it solve? Is it a performance improvement? A bug fix? Reduced memory usage? How does the smaller lock work (e.g., is it a spinlock instead of a mutex? A byte-wide lock instead of a word-wide lock?).

  • Impact: Simply listing the changed files isn't sufficient. Address all the impact points explicitly:

    • Is new feature added? Is existing feature changed?: Likely "Existing feature changed" - explain which feature. Serial driver functionality.
    • Impact on user: Probably NO, but state this explicitly. If there are any subtle timing changes, mention them.
    • Impact on build: NO (unless you added any new configuration options).
    • Impact on hardware: YES (list the affected architectures/boards). Be specific.
    • Impact on documentation: Likely NO, but state it. If the smaller lock introduces any usage restrictions or considerations, document them.
    • Impact on security: Potentially YES if the lock change affects interrupt handling or concurrency. Analyze this carefully and justify your answer.
    • Impact on compatibility: Probably NO, but analyze and state explicitly.
  • Testing: "ci" is insufficient. Provide specific details:

    • Build Host(s): List the OS, CPU architecture, and compiler version used for testing.
    • Target(s): Specify the architectures and boards (and configurations) on which the changes were tested.
    • Testing logs before change: Include actual logs demonstrating the behavior before the change. If the change fixes a bug, show the bug occurring. If it's a performance improvement, show the performance metrics before the change.
    • Testing logs after change: Show the logs after the change demonstrating the fix or improvement. Include performance metrics if relevant.

How to Improve the PR Description:

  1. Expand the Summary: Clearly state the reason for the change. Quantify the benefits (e.g., "Reduces lock contention by X%," "Fixes deadlock scenario Y," "Reduces memory footprint by Z bytes"). Explain how the smaller lock achieves these benefits.

  2. Address all Impact points: Answer all the questions with "YES" or "NO" and provide specific details where necessary. Don't just list files.

  3. Provide Detailed Testing Information: Specify the build host and target environments. Include actual logs demonstrating the behavior before and after the change. If a specific test case was used, describe it or link to it. Don't just rely on CI passing. Show evidence that you've personally verified the change.

By providing this missing information, your PR will be much easier for reviewers to understand and approve. It also demonstrates that you have thoroughly considered the implications of your changes.

arch/arm/src/tiva/common/tiva_hciuart.c
arch/arm/src/tms570/tms570_lowputc.c
arch/arm/src/xmc4/xmc4_serial.c
arch/arm64/src/a64/a64_serial.c
arch/mips/src/pic32mx/pic32mx_serial.c
arch/mips/src/pic32mz/pic32mz_serial.c
arch/risc-v/src/c906/c906_serial.c
arch/risc-v/src/fe310/fe310_serial.c
arch/risc-v/src/hpm6750/hpm6750_serial.c
arch/risc-v/src/k210/k210_serial.c
arch/sparc/src/bm3803/bm3803-serial.c
arch/sparc/src/bm3823/bm3823-serial.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 798695a into apache:master Dec 23, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_51 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 Arch: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: sparc Issues related to the SPARC 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