AT client fix only: remove asyncRunningMutex in uAtClientIgnoreAsync(…

…). (#530)

The function uAtClientIgnoreAsync() had a mutex, asyncRunningMutex, which it would lock while doing its work in order to guarantee that, if it were called while an asynchronous task was currently in progress, it would wait for that asynchronous event to complete before returning, ensuring that none were even partially executing. However that was a mistake: the things called by the asynchronous event will have their own mutex protection and so, for instance in the case of the cellular API and URC-related events, the following sequence could occur during shut-down of the cellular API:

- task (A): calls one of the cellular API shut-down functions (uCellRemove() or uCellDeinit()), which locks the cellular API mutex gUCellPrivateMutex.
- task (B): starts running an asynchronous event and so asyncRunningMutex is locked while executing it.
- task (A): as part of the cellular API shutdown, calls uAtClientIgnoreAsync() which wants to lock asyncRunningMutex but is blocked as task (B) has it.
- task (B): the function called by the asynchronous event is in the cellular API and so wants to lock gUCellPrivateMutex but is blocked as task (A) has it: WE ARE LOCKED.

With this commit the asyncRunningMutex in uAtClientIgnoreAsync() is removed; it is up to the things being called asynchronously to provide their own mutex protection (which they do) and that will already ensure that such calls complete in an organised way; asyncRunningMutex is not actually necessary.

common/at_client/api/u_at_client.h

@@ -461,13 +461,18 @@ uAtClientHandle_t uAtClientAdd(int32_t streamHandle,
 /** Tell the given AT client to throw away asynchronous events; use NULL
  * as the parameter to apply this to all AT clients.  This function
  * is useful when the application that is using the AT client is shutting
- * things down but may have issues callbacks, either for URCs
+ * things down but may have issued callbacks, either for URCs
  * directly or for callbacks via uAtClientCallback().  Such asynchronous
  * callbacks may have been given pointers to context data which will
  * become invalid, yet they may still be sitting in a queue waiting
  * to be processed and so could access out of range memory if they are
  * allowed to run; calling this function before invalidating such pointers,
  * then later calling uAtClientRemove(), avoids any surprises.
+ * Note that this function stops any future asynchronous events but
+ * an asynchronous event currently running will, of course, continue
+ * to execute; hence it is important that any APIs called from an
+ * asynchronous event have their own mutex protection against such
+ * shut-down situations.
  *
  * @param atHandle the handle of the AT client the should ignore
  *                 asynchronous events.

common/at_client/src/u_at_client.c

@@ -372,7 +372,6 @@ typedef struct uAtClientInstance_t {
     uPortMutexHandle_t mutex; /** Mutex for threadsafeness. */
     uPortMutexHandle_t streamMutex; /** Mutex for the data stream. */
     uPortMutexHandle_t urcPermittedMutex; /** Mutex that we can use to avoid trampling on a URC. */
-    uPortMutexHandle_t asyncRunningMutex; /** Mutex indicating an uAtClientCallback() is executing. */
     uAtClientReceiveBuffer_t *pReceiveBuffer; /** Pointer to the receive buffer structure. */
     bool debugOn; /** Whether general debug is on or off. */
     bool printAtOn; /** Whether printing of AT commands and responses is on or off. */
@@ -779,11 +778,6 @@ static void removeClient(uAtClientInstance_t *pClient)
     U_PORT_MUTEX_UNLOCK(pClient->urcPermittedMutex);
     uPortMutexDelete(pClient->urcPermittedMutex);
 
-    // Delete the async running mutex
-    U_PORT_MUTEX_LOCK(pClient->asyncRunningMutex);
-    U_PORT_MUTEX_UNLOCK(pClient->asyncRunningMutex);
-    uPortMutexDelete(pClient->asyncRunningMutex);
-
     // And finally free the client context.
     free(pClient);
 }
@@ -2359,20 +2353,13 @@ static void urcCallback(int32_t streamHandle, uint32_t eventBitmask,
 // Callback for the event queue.
 static void eventQueueCallback(void *pParameters, size_t paramLength)
 {
-    uAtClientInstance_t *pClient;
     uAtClientCallback_t *pCb = (uAtClientCallback_t *) pParameters;
 
     (void) paramLength;
 
     if ((pCb != NULL) && (pCb->pFunction != NULL) &&
         processAsync(pCb->atClientMagicNumber)) {
-        pClient = (uAtClientInstance_t *) pCb->atHandle;
-
-        U_PORT_MUTEX_LOCK(pClient->asyncRunningMutex);
-
         pCb->pFunction(pCb->atHandle, pCb->pParam);
-
-        U_PORT_MUTEX_UNLOCK(pClient->asyncRunningMutex);
     }
 }
 
@@ -2513,8 +2500,7 @@ uAtClientHandle_t uAtClientAdd(int32_t streamHandle,
                     // Create the mutexes
                     if ((uPortMutexCreate(&(pClient->mutex)) == 0) &&
                         (uPortMutexCreate(&(pClient->streamMutex)) == 0) &&
-                        (uPortMutexCreate(&(pClient->urcPermittedMutex)) == 0) &&
-                        (uPortMutexCreate(&(pClient->asyncRunningMutex)) == 0)) {
+                        (uPortMutexCreate(&(pClient->urcPermittedMutex)) == 0)) {
                         // Set all the non-zero initial values before we set
                         // the event handlers which might call us
                         pClient->streamHandle = streamHandle;
@@ -2565,9 +2551,6 @@ uAtClientHandle_t uAtClientAdd(int32_t streamHandle,
 
                 if (errorCode != 0) {
                     // Clean up on failure
-                    if (pClient->asyncRunningMutex != NULL) {
-                        uPortMutexDelete(pClient->asyncRunningMutex);
-                    }
                     if (pClient->urcPermittedMutex != NULL) {
                         uPortMutexDelete(pClient->urcPermittedMutex);
                     }
@@ -2599,22 +2582,14 @@ void uAtClientIgnoreAsync(uAtClientHandle_t atHandle)
 
     U_PORT_MUTEX_LOCK(gMutex);
 
-    // As well as locking gMutex we also lock
-    // asyncRunningMutex so that we can guarantee
-    // that no asynchronous call will land once this
-    // function returns
     if (pClient == NULL) {
         pClient = gpAtClientList;
         while (pClient != NULL) {
-            U_PORT_MUTEX_LOCK(pClient->asyncRunningMutex);
             ignoreAsync(pClient);
-            U_PORT_MUTEX_UNLOCK(pClient->asyncRunningMutex);
             pClient = pClient->pNext;
         }
     } else {
-        U_PORT_MUTEX_LOCK(pClient->asyncRunningMutex);
         ignoreAsync(pClient);
-        U_PORT_MUTEX_UNLOCK(pClient->asyncRunningMutex);
     }
 
     U_PORT_MUTEX_UNLOCK(gMutex);