Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

use small lock in following files:

arch/arm/src/rp2040/rp2040_usbdev.c
arch/arm/src/rp23xx/rp23xx_uart.c
arch/arm/src/s32k1xx/s32k1xx_enet.c
arch/arm/src/sam34/sam_lowputc.c
arch/arm/src/sama5/sam_lowputc.c
arch/arm/src/samd2l2/sam_serial.c
arch/arm/src/samd5e5/sam_serial.c
arch/arm/src/samv7/sam_lowputc.c
arch/arm/src/stm32/stm32_hciuart.c
arch/arm/src/stm32/stm32_qencoder.c
arch/arm/src/stm32/stm32_serial.c
arch/arm/src/stm32f0l0g0/stm32_serial_v1.c

Impact

arch/arm/src/rp2040/rp2040_usbdev.c
arch/arm/src/rp23xx/rp23xx_uart.c
arch/arm/src/s32k1xx/s32k1xx_enet.c
arch/arm/src/sam34/sam_lowputc.c
arch/arm/src/sama5/sam_lowputc.c
arch/arm/src/samd2l2/sam_serial.c
arch/arm/src/samd5e5/sam_serial.c
arch/arm/src/samv7/sam_lowputc.c
arch/arm/src/stm32/stm32_hciuart.c
arch/arm/src/stm32/stm32_qencoder.c
arch/arm/src/stm32/stm32_serial.c
arch/arm/src/stm32f0l0g0/stm32_serial_v1.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 Dec 23, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 23, 2024

[Experimental Bot, please feedback here]

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

Summary:

  • Insufficient explanation: "use small lock in following files" is vague. Why are these locks being changed? What problem does this solve (e.g., performance bottleneck, deadlock potential, unnecessary overhead)? What kind of "small lock" is being used (e.g., spinlock, mutex)?
  • Missing functional context: What subsystems are affected by these changes (USB, UART, Ethernet, etc.)? Be more specific than just listing file paths.
  • No "how it works": Explain the mechanism of the change. Did you replace a larger lock with a smaller one? Did you introduce fine-grained locking? Did you eliminate locking altogether in some cases?
  • Missing issue references: If this PR addresses a reported issue, link to it.

Impact:

  • All "Impact" sections are simply repeating the list of files. This provides no useful information. Address each point directly:
    • New Feature/Changed Feature: Briefly describe the functional change.
    • Impact on User: Will the user experience any changes in behavior or API? Likely no, but state that explicitly.
    • Impact on Build: Likely no, but state it.
    • Impact on Hardware: Specify the affected architectures (ARM, specifically the listed sub-families). If specific boards are impacted, list them.
    • Impact on Documentation: Likely no, but confirm.
    • Impact on Security: Does changing the locking mechanism have any security implications? (e.g., race conditions). Carefully consider this.
    • Impact on Compatibility: Are there any backward compatibility concerns?
    • Anything else: Anything unusual or noteworthy?

Testing:

  • Insufficient information: "ci" is not enough. Which CI systems? What tests were run?
  • No logs: The template specifically requests "Testing logs before change" and "Testing logs after change." Provide relevant snippets showing the improvement (e.g., performance benchmarks, demonstration that a deadlock is resolved). If you don't have comparative logs, explain why. Even a simple statement like "No noticeable performance difference observed during manual testing" is better than nothing.
  • Target details: Be specific about the target(s) you tested on. "arch(sim,RISC-V,ARM,..), board:config" is a placeholder. Fill it in with the actual architectures and board configurations. For example: ARM: STM32F4 Discovery, config: default.

Example of an improved Summary:

This PR replaces mutexes with spinlocks in the serial driver, UART driver, Ethernet driver, and USB driver for several ARM architectures (rp2040, rp23xx, s32k1xx, sam34, sama5, samd2l2, samd5e5, samv7, stm32, stm32f0l0g0) to reduce overhead in interrupt contexts and improve responsiveness. Profiling revealed that these mutexes were a performance bottleneck in high-throughput scenarios. The spinlocks are used to protect short critical sections within the drivers, ensuring data integrity. This change addresses issue #[Insert Issue Number if applicable].

By providing specific details and addressing each point in the template, you'll create a much stronger and more informative PR that is more likely to be accepted.

arch/arm/src/rp2040/rp2040_usbdev.c
arch/arm/src/rp23xx/rp23xx_uart.c
arch/arm/src/s32k1xx/s32k1xx_enet.c
arch/arm/src/sam34/sam_lowputc.c
arch/arm/src/sama5/sam_lowputc.c
arch/arm/src/samd2l2/sam_serial.c
arch/arm/src/samd5e5/sam_serial.c
arch/arm/src/samv7/sam_lowputc.c
arch/arm/src/stm32/stm32_hciuart.c
arch/arm/src/stm32/stm32_qencoder.c
arch/arm/src/stm32/stm32_serial.c
arch/arm/src/stm32f0l0g0/stm32_serial_v1.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit b66c1b9 into apache:master Dec 23, 2024
25 checks passed
@hujun260 hujun260 deleted the apache_49 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 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