Skip to content

Use small lock to protect resources related to i2c. - #15648

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
wangzhi16:lock_16
Jan 23, 2025
Merged

Use small lock to protect resources related to i2c.#15648
xiaoxiang781216 merged 1 commit into
apache:masterfrom
wangzhi16:lock_16

Conversation

@wangzhi16

Copy link
Copy Markdown
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Use small lock to protect resources related to i2c.

Impact

logic of i2c in all arch.

Testing

CI

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-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: xtensa Issues related to the Xtensa architecture Size: L The size of the change in this PR is large labels Jan 22, 2025
@nuttxpr

nuttxpr commented Jan 22, 2025

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

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

Missing Information in Summary:

  • Why is this change necessary? Is there a bug being fixed? Is performance being improved? Is this a new feature to support a specific use case? This is the most crucial part of the summary. Just saying "Use small lock to protect resources related to i2c" doesn't explain the motivation behind the change.
  • What functional part of the code is being changed? Be specific. Mention the affected files, modules, or subsystems. For example: "The I2C driver implementation in drivers/i2c and the I2C bus initialization in architecture-specific code."
  • How does the change exactly work? What kind of lock is being used (mutex, semaphore, spinlock)? Was it unprotected before? How does this lock prevent race conditions or other issues? Provide more technical detail.
  • Related Issues: Link any related NuttX or NuttX Apps issues. This helps track the context and reason for the change.

Missing Information in Impact:

  • Is new feature added? Is existing feature changed? This needs a YES/NO answer followed by a description. In this case, it's likely "Existing feature changed" - explain how.
  • All other Impact sections need YES/NO answers followed by descriptions if YES. Simply stating "logic of i2c in all arch" isn't helpful. Does this change the API? Does it affect how users interact with I2C? Does it change build configurations? Does it add new dependencies? Be specific about the impact on each area. If the answer is NO, explicitly state NO.

Insufficient Testing Information:

  • "CI" is not enough. While CI passing is important, you need to provide more context about your local testing.
  • Build Host(s): List the operating system, CPU architecture, and compiler version used for building.
  • Target(s): Specify the target architecture (e.g., simulated, RISC-V, ARM), the board and configuration used for testing.
  • Testing logs before change: Include relevant logs demonstrating the problem this PR fixes (if applicable) or the behavior before the change.
  • Testing logs after change: Show logs demonstrating the corrected behavior or the new functionality after applying the change. Demonstrate that the I2C operations are now correctly protected by the lock.

Example of an Improved PR Description:

## Summary

This PR fixes a potential race condition in the I2C driver by introducing a small lock to protect access to shared I2C resources.  Previously, concurrent access to the I2C bus from multiple threads or interrupt handlers could lead to data corruption or bus errors.  This change implements a mutex lock within the `drivers/i2c` code and modifies the architecture-specific I2C initialization in `arch/<arch>/src/chip/<chip>/i2c_init.c` to acquire and release the lock during I2C operations.  This ensures exclusive access to the I2C bus, preventing race conditions.

Related [NuttX Issue](https://github.com/apache/nuttx/issues/XXXX) (If applicable)

## Impact

* Is new feature added? NO
* Is existing feature changed? YES.  The I2C driver now uses a mutex to protect against race conditions. This should not impact the user API directly, but improves the reliability of I2C communication in multi-threaded environments.
* Impact on user: NO
* Impact on build: NO
* Impact on hardware: NO
* Impact on documentation: YES. The I2C driver documentation will be updated to reflect the changes and explain the locking mechanism.
* Impact on security: YES. This fixes a potential vulnerability related to data corruption due to race conditions.
* Impact on compatibility: NO
* Anything else to consider:  None

## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target(s): sim:nsh, STM32F4DISCOVERY:nsh

Testing logs before change:

i2c_test: Transfer failed with error code -EINVAL
i2c_test: Data corruption detected


Testing logs after change:

i2c_test: All transfers successful

By providing more details and following the guidelines, you'll make it much easier for reviewers to understand and approve your PR. Remember to replace the placeholder information (e.g., issue number, specific files, test results) with your actual data.

Comment thread arch/arm/src/lc823450/lc823450_i2c.c Outdated
@wangzhi16
wangzhi16 force-pushed the lock_16 branch 4 times, most recently from 49072c7 to baf1747 Compare January 22, 2025 12:47
Comment thread arch/arm/src/xmc4/xmc4_i2c.c
Comment thread arch/arm/src/stm32/stm32_i2c.c Outdated
Signed-off-by: wangzhi16 <wangzhi16@xiaomi.com>
@github-actions github-actions Bot added Size: M The size of the change in this PR is medium and removed Size: L The size of the change in this PR is large labels Jan 23, 2025
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: mips Issues related to the MIPS architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa 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

4 participants