Skip to content

Conversation

@wangzhi16
Copy link
Contributor

Summary

use small lock in following files:

arch/arm/src/am335x/am335x_can.c
arch/arm/src/am335x/am335x_gpio.c
arch/arm/src/am335x/am335x_i2c.c
arch/arm/src/am335x/am335x_irq.c
arch/arm/src/am335x/am335x_serial.c
arch/arm64/src/imx9/imx9_gpio.c
arch/arm64/src/imx9/imx9_lpi2c.c
arch/arm64/src/imx9/imx9_lpspi.c
arch/arm64/src/imx9/imx9_usbdev.c
arch/x86_64/src/intel64/intel64_tsc_tickless.c

Impact

arch/arm/src/am335x/am335x_can.c
arch/arm/src/am335x/am335x_gpio.c
arch/arm/src/am335x/am335x_i2c.c
arch/arm/src/am335x/am335x_irq.c
arch/arm/src/am335x/am335x_serial.c
arch/arm64/src/imx9/imx9_gpio.c
arch/arm64/src/imx9/imx9_lpi2c.c
arch/arm64/src/imx9/imx9_lpspi.c
arch/arm64/src/imx9/imx9_usbdev.c
arch/x86_64/src/intel64/intel64_tsc_tickless.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: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium labels Jan 6, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 6, 2025

[Experimental Bot, please feedback here]

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

Missing Information in Summary:

  • Why is the change necessary? The PR mentions using a "small lock," but why is this change needed? Is it for performance improvement? Deadlock avoidance? Reduced memory usage? This needs to be explicitly stated.
  • What functional part of the code is being changed? Listing files is not enough. What functionality within those files is affected? (e.g., Interrupt handling, peripheral drivers, timing, etc.)
  • How does the change exactly work? What is the "small lock"? Is it a new lock type? A replacement of existing locking mechanisms? Be specific about the locking mechanism used (e.g., spinlock, mutex).

Missing Information in Impact:

Simply repeating the list of files does not describe the impact. Address each point explicitly:

  • New feature? Existing feature changed? This is likely an existing feature change (improved locking). State it clearly.
  • Impact on user: Will user applications need to be modified? Probably not, but say so explicitly (NO).
  • Impact on build: Are new configurations required? Likely NO, but confirm.
  • Impact on hardware: Are any specific architectures, boards, or drivers affected beyond those listed? Clarify. The listed files suggest ARM, ARM64, and x86_64 architectures are affected. Be explicit about which boards within these architectures are impacted.
  • Impact on documentation: Does any documentation need to be updated to reflect the locking changes? If not, state NO explicitly.
  • Impact on security: Are there any potential security implications of this change? Explain why or why not.
  • Impact on compatibility: Does this change break backward compatibility? Does it affect forward compatibility or interoperability? Explain.

Missing Information in Testing:

  • CI is not enough. While CI is important, you need to provide specific test results. "CI" doesn't tell reviewers anything about what was tested or the results.
  • Build Host(s): Be specific about the build environment used.
  • Target(s): Provide details about the target platform(s) where the changes were tested. "arch(sim,RISC-V,ARM,..), board:config" is too vague. Provide the actual architecture, board, and configuration used.
  • Testing logs before change: Include actual logs, even if they are just simple "hello world" tests or basic functionality verification. This demonstrates what worked before the change.
  • Testing logs after change: Show logs demonstrating the same functionality working after the change. Focus on the areas affected by the locking changes. Show that the functionality still works correctly and, if applicable, demonstrate the improvements (e.g., reduced latency, absence of deadlocks).

Example of Improved PR Description (fill in the specific details):

Summary

This change improves locking mechanisms in several drivers to reduce the risk of priority inversion and potentially improve performance. The existing global spinlocks in the CAN, GPIO, I2C, IRQ, and serial drivers for AM335x (ARM), the GPIO, LPI2C, LPSPI, and USBDEV drivers for i.MX9 (ARM64), and the tickless timer driver for x86_64 are replaced with more fine-grained mutexes. This allows for greater concurrency and reduces the likelihood of higher-priority tasks being blocked by lower-priority tasks holding a shared lock. This addresses potential deadlocks identified in [NuttX Issue #XXXX] (if applicable).

Impact

  • New feature: NO
  • Existing feature changed: YES, improved locking mechanisms in several peripheral drivers.
  • Impact on user: NO
  • Impact on build: NO
  • Impact on hardware: YES. This affects the following:
    • AM335x (ARM) - [List specific boards]
    • i.MX9 (ARM64) - [List specific boards]
    • x86_64 - [List specific boards/configurations]
  • Impact on documentation: NO
  • Impact on security: Potentially improved by reducing the risk of denial-of-service attacks through induced priority inversion. However, no known vulnerabilities are directly addressed by this change.
  • Impact on compatibility: Backward compatible. No changes to the driver interfaces are required.
  • Anything else to consider: None.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.2
  • Target(s):
    • QEMU-based AM335x simulator, nsh configuration
    • i.MX9 Evaluation Kit, configs/imx93-evk/nsh
    • QEMU-based x86_64 simulator, configs/qemu-i440fx/nsh

Testing logs before change (AM335x - GPIO example):

nsh> gpioinfo
... (output showing GPIO status before change)

Testing logs after change (AM335x - GPIO example):

nsh> gpioinfo
... (output showing GPIO status after change – should be the same functionality)

(Repeat testing logs for other affected architectures/drivers. Include tests that specifically exercise the locking mechanisms, if possible).

By providing this level of detail, reviewers can quickly understand the purpose, impact, and testing of your PR, leading to faster and more effective reviews. Remember to replace the bracketed placeholders with specific information relevant to your changes.

@wangzhi16 wangzhi16 force-pushed the dev branch 2 times, most recently from e6b303e to 88e20b4 Compare January 6, 2025 08:38
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium labels Jan 6, 2025
@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Jan 6, 2025
@wangzhi16 wangzhi16 force-pushed the dev branch 3 times, most recently from ff66e36 to fa9bd98 Compare January 7, 2025 06:13
@github-actions github-actions bot added Size: S The size of the change in this PR is small and removed Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium labels Jan 9, 2025
@wangzhi16 wangzhi16 closed this Jan 9, 2025
@github-actions github-actions bot added Size: XS The size of the change in this PR is very small and removed Size: S The size of the change in this PR is small labels Jan 9, 2025
@wangzhi16 wangzhi16 reopened this Jan 9, 2025
@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium and removed Size: XS The size of the change in this PR is very small labels Jan 9, 2025
@wangzhi16 wangzhi16 force-pushed the dev branch 2 times, most recently from f92f400 to 36e452c Compare January 9, 2025 09:01
@wangzhi16 wangzhi16 force-pushed the dev branch 4 times, most recently from 2c86bd6 to c7818b2 Compare January 10, 2025 04:04
    arch/arm/src/am335x/am335x_can.c
    arch/arm/src/am335x/am335x_gpio.c
    arch/arm/src/am335x/am335x_i2c.c
    arch/arm/src/am335x/am335x_irq.c
    arch/arm/src/am335x/am335x_serial.c
    arch/arm64/src/imx9/imx9_gpio.c
    arch/arm64/src/imx9/imx9_lpi2c.c
    arch/arm64/src/imx9/imx9_lpspi.c
    arch/arm64/src/imx9/imx9_usbdev.c
    arch/x86_64/src/intel64/intel64_tsc_tickless.c

Signed-off-by: wangzhi16 <wangzhi16@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit d84ba60 into apache:master Jan 10, 2025
39 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 Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: x86_64 Issues related to the x86_64 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