Skip to content

Conversation

@cvinayak
Copy link
Contributor

Fix address resolution wait to not be infinite loop and instead exit with a reasonable count down value.

Fixes #100101

Fix address resolution wait to not be infinite loop and
instead exit with a reasonable count down value.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@sonarqubecloud
Copy link

@cvinayak cvinayak requested a review from Copilot November 28, 2025 12:50
Copilot finished reviewing on behalf of cvinayak November 28, 2025 12:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an infinite loop issue in address resolution waiting logic for the nRF54LX Bluetooth controller by introducing a countdown-based timeout mechanism. The fix addresses issue #100101 where the code could hang indefinitely waiting for address resolution to complete.

Key Changes:

  • Introduces a new ar_end_wait() helper function with a countdown timeout (200 iterations) to replace infinite wait loops
  • Conditionally disables cpu_sleep() calls for NRF54LX SoC during the wait

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +2736 to +2739
#if !defined(CONFIG_SOC_SERIES_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_SERIES_NRF54LX */
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional compilation check uses CONFIG_SOC_SERIES_NRF54LX, but throughout the rest of this file, the consistent pattern is to use CONFIG_SOC_COMPATIBLE_NRF54LX (as seen in lines 2645, 2689, 2709, 2711, 2771, 2787, etc.). This inconsistency could lead to incorrect behavior where cpu_sleep() is called (or not called) on the wrong platforms.

Suggested fix:

#if !defined(CONFIG_SOC_COMPATIBLE_NRF54LX)
	/* We do not sleep on target SoC. */
	cpu_sleep();
#endif /* !CONFIG_SOC_COMPATIBLE_NRF54LX */
Suggested change
#if !defined(CONFIG_SOC_SERIES_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_SERIES_NRF54LX */
#if !defined(CONFIG_SOC_COMPATIBLE_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_COMPATIBLE_NRF54LX */

Copilot uses AI. Check for mistakes.
/* An arbitrary iteration which seems sufficient testing on target with one IRK to resolve.
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
*/
uint32_t countdown = 200U;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 200U for the countdown is arbitrary and lacks proper justification. While the comment mentions it "seems sufficient testing on target with one IRK to resolve," this doesn't provide guidance for:

  1. What happens if multiple IRKs need to be resolved
  2. How this value scales with different hardware configurations
  3. Whether timeout detection/handling is needed when countdown reaches 0

Consider either:

  • Defining this as a named constant (e.g., #define AAR_WAIT_COUNTDOWN_MAX 200U) with documentation explaining the expected timing
  • Adding timeout error handling/logging when countdown reaches 0
  • Documenting the expected maximum duration based on hardware specifications

Copilot uses AI. Check for mistakes.
static void ar_end_wait(void)
{
/* An arbitrary iteration which seems sufficient testing on target with one IRK to resolve.
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammatical error: "be not be waiting" should be "not be waiting".

Suggested change
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
* We should ideally not be waiting anyway, and in theory be resolved before we are here.

Copilot uses AI. Check for mistakes.
Comment on lines +2726 to +2741
static void ar_end_wait(void)
{
/* An arbitrary iteration which seems sufficient testing on target with one IRK to resolve.
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
*/
uint32_t countdown = 200U;

while ((NRF_AAR->EVENTS_END == 0U) && (countdown != 0U)) {
countdown--;

#if !defined(CONFIG_SOC_SERIES_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_SERIES_NRF54LX */
}
}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling or status return when the countdown expires. If NRF_AAR->EVENTS_END remains 0 after the countdown completes, the calling code has no way to detect this timeout condition. This could mask hardware issues or unexpected delays.

Consider:

  • Returning a status code from ar_end_wait() to indicate success/timeout
  • Adding an assertion or error log when the timeout occurs
  • Documenting the expected behavior when timeout happens

Copilot uses AI. Check for mistakes.
@cvinayak cvinayak marked this pull request as ready for review November 28, 2025 14:00
@cvinayak cvinayak added the backport v4.3-branch Request backport to the v4.3-branch label Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BLE device bricked after disconnect with CONFIG_BT_APP_PASSKEY=y on nRF54L15 (Zephyr 4.3.0)

2 participants