Adding xmos granular lock

[chinglee-iot]

Description

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Comprehensive Review: XMOS Granular Locks Implementation

This PR introduces a significant architectural enhancement implementing granular locking for FreeRTOS SMP to reduce lock contention. While the overall approach is sound, several critical issues need to be addressed.

High-Level Architecture Assessment

The "Dual Spinlock With Data Group Locking" approach is well-designed, organizing data into logical groups (Kernel, Queue, Event Group, Stream Buffer, Timer) each with their own spinlock pairs. The hierarchical locking order prevents deadlocks and the backwards compatibility is maintained.

Critical Issues Found

1. MISRA C 2012 Compliance: Multiple violations detected by static analysis need resolution
2. Memory Safety: Potential race conditions in granular lock acquisition
3. API Consistency: Some naming and macro inconsistencies
4. Documentation: Critical sections and locking hierarchy need better documentation

Recommendation

This is a substantial improvement to FreeRTOS SMP performance, but the critical issues below must be resolved before merging. Consider splitting into smaller, focused PRs for easier review of the core changes.

Overall Status: Request Changes - Critical issues require resolution

[github-actions]

Detailed Technical Review: Critical Issues Identified

Based on static analysis results and code review, I've identified several critical issues that must be addressed:

MISRA C:2012 Compliance Issues (28 violations found)

• Rule 20.7: Macro parameter parentheses missing (8 violations)
• Rule 20.9: Undefined identifiers in preprocessor directives (17 violations)
• Rule 20.1: Misplaced include directives (1 violation)
• Rule 8.4: Missing external declarations (2 violations)

Critical Thread Safety Issues

1. Race condition in RP2040 spinlock implementation - Hardware/software lock state inconsistency window
2. Preemption disable counter overflow - No protection against wraparound in nested calls

Code Quality Issues

1. API inconsistency - Trailing semicolons in macro definitions causing double semicolons
2. Early return patterns - Violates FreeRTOS single exit point coding standard

Priority Classification

• P0 (Critical): Race condition, counter overflow - Must fix before merge
• P1 (High): MISRA violations, API inconsistency - Should fix for compliance
• P2 (Medium): Code style issues - Good to fix for maintainability

The granular locking architecture itself is excellent, but these implementation issues need resolution to ensure system stability and compliance.

[github-actions]

MISRA C:2012 Rule 20.7 Violation - Static analysis detected that macro parameters should be enclosed in parentheses.

The macro parameter should be enclosed in parentheses to prevent unintended operator precedence issues:

#define taskDATA_GROUP_ENTER_CRITICAL( pxTaskSpinlock, pxISRSpinlock )                \
do {                                                                                  \
    /* Disable preemption to avoid task state changes during the critical section. */ \
    vTaskPreemptionDisable( NULL );                                                   \
    {                                                                                 \
        const BaseType_t xCoreID = ( BaseType_t ) portGET_CORE_ID();                  \
        /* Task spinlock is always taken first */                                     \
        portGET_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxTaskSpinlock) );        \
        /* Disable interrupts */                                                      \
        portDISABLE_INTERRUPTS();                                                     \
        /* Take the ISR spinlock next */                                              \
        portGET_SPINLOCK( xCoreID, ( portSPINLOCK_TYPE * ) (pxISRSpinlock) );         \
        /* Increment the critical nesting count */                                    \
        portINCREMENT_CRITICAL_NESTING_COUNT( xCoreID );                              \
    }                                                                                 \
} while( 0 )

Apply similar fixes to all related macros in this file.

[github-actions]

MISRA C:2012 Rule 20.9 Violation - Static analysis detected that all identifiers used in preprocessing directives shall be #defined.

The macro conditions should ensure all identifiers are properly defined before use:

#if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) )

This ensures portUSING_GRANULAR_LOCKS is defined before evaluation, preventing potential issues with undefined identifiers in preprocessor expressions.

[github-actions]

MISRA C:2012 Rule 8.4 Violation - Static analysis detected that a compatible declaration shall be visible when an object or function with external linkage is defined.

The external variables need proper declarations. Add these to the appropriate header file (likely portmacro.h):

#if ( portCRITICAL_NESTING_IN_TCB == 0 )
    extern UBaseType_t uxCriticalNestings[ configNUMBER_OF_CORES ];
#endif

#if ( portUSING_GRANULAR_LOCKS == 1 )
    extern portSPINLOCK_TYPE xPortIsrSpinlock;
    extern portSPINLOCK_TYPE xPortTaskSpinlock;
#endif

This ensures external linkage objects have visible declarations before definition.

[github-actions]

MISRA C:2012 Rule 20.9 Violation & Potential Race Condition - Static analysis detected improper preprocessor directive usage, and there's a critical thread safety issue.

Issue 1: The #if condition evaluation needs proper identifier definition checking:

#if ( defined( portUSING_GRANULAR_LOCKS ) && ( portUSING_GRANULAR_LOCKS == 1 ) )

Issue 2: CRITICAL - The spinlock implementation has a race condition. The hardware spinlock is released before updating the software spinlock state, creating a window where another core can acquire the hardware lock while the software state is inconsistent.

Fix: Atomic operations or proper ordering:

void vPortSpinlockGet(BaseType_t xCoreID, portSPINLOCK_TYPE *pxSpinlock)
{
    BaseType_t xAcquired = pdFALSE;
    spin_lock_t *pxHardwareSpinLock = spin_lock_instance(configSMP_SPINLOCK_0);
    UBaseType_t ulState;

    while (xAcquired == pdFALSE)
    {
        ulState = portSET_INTERRUPT_MASK();
        
        if (spin_try_lock_unsafe(pxHardwareSpinLock) != false)
        {
            if ( pxSpinlock->xLockCount == 0 )
            {
                pxSpinlock->xOwnerCore = xCoreID;
                pxSpinlock->xLockCount = 1;
                xAcquired = pdTRUE;
                /* Release hardware lock AFTER updating software state */
                spin_unlock_unsafe(pxHardwareSpinLock);
            }
            else if( pxSpinlock->xOwnerCore == xCoreID )
            {
                pxSpinlock->xLockCount++;
                xAcquired = pdTRUE;
                spin_unlock_unsafe(pxHardwareSpinLock);
            }
            else
            {
                /* Release without updating state for contention case */
                spin_unlock_unsafe(pxHardwareSpinLock);
            }
        }
        
        portCLEAR_INTERRUPT_MASK(ulState);
    }
}

This critical race condition could cause system instability in SMP environments.

[github-actions]

MISRA C:2012 Rule 20.1 Violation - Static analysis detected that #include directives should only be preceded by preprocessor directives or comments.

The #include "swlock.h" directive appears after other code elements. Move it to the appropriate location with other includes:

/* Standard includes */
#include <...>

/* FreeRTOS specific includes */
#include "swlock.h"  // Move here with other includes

/* Then macro definitions follow */

This follows MISRA guidelines for proper include directive placement and improves code organization.

[github-actions]

MISRA C:2012 Rule 20.7 Violation & API Inconsistency - Multiple issues detected.

Issue 1: MISRA Rule 20.7 - Macro parameters need parentheses:

#define queueENTER_CRITICAL( pxQueue )    taskDATA_GROUP_ENTER_CRITICAL( &(pxQueue)->xTaskSpinlock, &(pxQueue)->xISRSpinlock )

Issue 2: API Inconsistency - The fallback macros have incorrect syntax with trailing semicolons:

// INCORRECT - has trailing semicolon causing double semicolon when used
#define queueENTER_CRITICAL( pxQueue )    taskENTER_CRITICAL();

// CORRECT - no trailing semicolon 
#define queueENTER_CRITICAL( pxQueue )    taskENTER_CRITICAL()

Fixed Version:

#if ( portUSING_GRANULAR_LOCKS == 1 )
    #define queueENTER_CRITICAL( pxQueue )                                      taskDATA_GROUP_ENTER_CRITICAL( &((pxQueue)->xTaskSpinlock), &((pxQueue)->xISRSpinlock) )
    #define queueENTER_CRITICAL_FROM_ISR( pxQueue, puxSavedInterruptStatus )    taskDATA_GROUP_ENTER_CRITICAL_FROM_ISR( &((pxQueue)->xISRSpinlock), (puxSavedInterruptStatus) )
    #define queueEXIT_CRITICAL( pxQueue )                                       taskDATA_GROUP_EXIT_CRITICAL( &((pxQueue)->xTaskSpinlock), &((pxQueue)->xISRSpinlock) )
    #define queueEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus, pxQueue )      taskDATA_GROUP_EXIT_CRITICAL_FROM_ISR( (uxSavedInterruptStatus), &((pxQueue)->xISRSpinlock) )
#else /* #if ( portUSING_GRANULAR_LOCKS == 1 ) */
    #define queueENTER_CRITICAL( pxQueue )                                      taskENTER_CRITICAL()
    #define queueENTER_CRITICAL_FROM_ISR( pxQueue, puxSavedInterruptStatus )    do { *( puxSavedInterruptStatus ) = taskENTER_CRITICAL_FROM_ISR(); } while( 0 )
    #define queueEXIT_CRITICAL( pxQueue )                                       taskEXIT_CRITICAL()
    #define queueEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus, pxQueue )      taskEXIT_CRITICAL_FROM_ISR( uxSavedInterruptStatus )
#endif

The same issues appear in similar macros throughout this file - apply consistent fixes.

[github-actions]

Early Return Pattern Concern - This macro violates the FreeRTOS coding standard discouraging early returns.

The current implementation:

#define event_groupsUNLOCK( pxEventBits )    prvUnlockEventGroupForTasks( pxEventBits );

Creates this usage pattern:

xAlreadyYielded = event_groupsUNLOCK( pxEventBits ); // Direct return

Issue: This creates multiple exit points in functions, contradicting FreeRTOS coding standards that prefer single exit points for maintainability.

Recommendation: Consider restructuring the implementation to maintain single exit points:

// In the calling function, instead of direct return:
BaseType_t xYieldResult;
event_groupsLOCK( pxEventBits );
{
    // ... critical section work ...
    xYieldResult = event_groupsUNLOCK( pxEventBits );
}

// Single exit point
xAlreadyYielded = xYieldResult;

This pattern should be applied consistently across all similar macros to maintain FreeRTOS coding standards.

[github-actions]

Critical Implementation Issue - Nested preemption disable counter without overflow protection.

Issue: The counter increment pxTCB->uxPreemptionDisable++ lacks overflow protection, which could cause undefined behavior in long-running systems.

Risk: If preemption is disabled/enabled unbalanced over time, the counter could overflow and wraparound, leading to:

1. Tasks becoming permanently non-preemptible
2. System deadlock scenarios
3. Real-time guarantee violations

Fix: Add overflow protection and validation:

pxTCB = prvGetTCBFromHandle( xTask );
configASSERT( pxTCB != NULL );

/* Protect against counter overflow */
configASSERT( pxTCB->uxPreemptionDisable < UBASE_TYPE_MAX );
if( pxTCB->uxPreemptionDisable < UBASE_TYPE_MAX )
{
    pxTCB->uxPreemptionDisable++;
}
else
{
    /* Handle overflow condition - could be assertion or error reporting */
    configASSERT( pdFALSE ); /* This should never happen in correct usage */
}

Alternative: Use a reasonable maximum limit:

/* Reasonable nesting limit to catch programming errors */
#define configMAX_PREEMPTION_DISABLE_NESTING    ( 255U )

if( pxTCB->uxPreemptionDisable < configMAX_PREEMPTION_DISABLE_NESTING )
{
    pxTCB->uxPreemptionDisable++;
}
else
{
    configASSERT( pdFALSE ); /* Likely programming error */
}

This prevents system corruption from unbalanced preemption control calls.