Skip to content

prevent NFC driver crash on repeated deinitialization#6844

Open
TychoVrahe wants to merge 1 commit intomainfrom
tychovrahe/nfc/deinit_fix
Open

prevent NFC driver crash on repeated deinitialization#6844
TychoVrahe wants to merge 1 commit intomainfrom
tychovrahe/nfc/deinit_fix

Conversation

@TychoVrahe
Copy link
Copy Markdown
Contributor

Spi deinit causes the crash when receiving NULL instance.

@TychoVrahe TychoVrahe self-assigned this Apr 28, 2026
@TychoVrahe TychoVrahe requested a review from kopecdav April 28, 2026 14:12
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f23f1948-cf77-4b6a-9540-d2822a554611

📥 Commits

Reviewing files that changed from the base of the PR and between 972dd51 and 7b271ae.

📒 Files selected for processing (1)
  • core/embed/io/nfc/st25/nfc.c

Walkthrough

The nfc_deinit() function in the NFC driver was changed to guard SPI de-initialization. Instead of unconditionally calling HAL_SPI_DeInit() on the driver's SPI handle, it now checks that drv->hspi.Instance is non-NULL before invoking HAL_SPI_DeInit(), avoiding de-initialization when the SPI peripheral was never initialized or its instance was cleared.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description is minimal and vague. While it mentions the core issue (SPI deinit crash with NULL instance), it lacks the structured information recommended by the template. Enhance the description with more context: explain the issue scenario, the fix approach, testing steps, and whether QA testing is needed per the template guidelines.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preventing NFC driver crash on repeated deinitialization, which matches the code change that adds a NULL check before SPI deinitialization.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tychovrahe/nfc/deinit_fix

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 28, 2026

en main(all)

model device_test click_test persistence_test
T2T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3B1 test(all) main(all) test(all) main(all) test(all) main(all)
T3T1 test(all) main(all) test(all) main(all) test(all) main(all)
T3W1 test(all) main(all) test(all) main(all) test(all) main(all)

Latest CI run: 25058975892

@github-project-automation github-project-automation Bot moved this to 🔎 Needs review in Firmware Apr 28, 2026
@TychoVrahe TychoVrahe force-pushed the tychovrahe/nfc/deinit_fix branch from 972dd51 to 7b271ae Compare April 28, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎 Needs review

Development

Successfully merging this pull request may close these issues.

1 participant