Should prvNotifyQueueSetContainer check if the queue is locked before editing event lists?

Kernel
1

cman12345 wrote on Thursday, February 19, 2015:

In queue.c, I noticed the following description about prvLock/UnlockQueue:

/*
 * Unlocks a queue locked by a call to prvLockQueue.  Locking a queue does not
 * prevent an ISR from adding or removing items to the queue, but does prevent
 * an ISR from removing tasks from the queue event lists.  If an ISR finds a
 * queue is locked it will instead increment the appropriate queue lock count
 * to indicate that a task may require unblocking.  When the queue in unlocked
 * these lock counts are inspected, and the appropriate action taken.
 */
static void prvUnlockQueue( Queue_t * const pxQueue ) PRIVILEGED_FUNCTION;

Does that mean it’s dangerous to edit the queue event list while running from an ISR?

If so, I’m trying to understand why there is no check to see if the queue is locked in prvNotifyQueueSetContainer where the following path can be reached:

xQueueGenericSendFromISR() -> prvNotifyQueueSetContainer() -> xTaskRemoveFromEventList()

Thanks for any help!

2

rtel wrote on Thursday, February 19, 2015:

I’ve been looking at this for the last half hour, and on first inspection you could be onto something here. From the scenarios I have drawn out it does look like a race condition could occur. Also I’m running the coverage tests as I type and we don’t seem to hit that condition. It will take some time to investigate further - but if there is an issue it can be corrected with a simple update that checks the queue lock before calling prvNotifyQueueSetContainer(), and does nothing but increment the lock if it is found that the lock is set.

Regards.

3

rtel wrote on Thursday, February 19, 2015:

It look to me like the code should be like the following (test markets and asserts removed for clarity). Do you agree:

EDIT - the formatting was unreadable so instead I have attached a small file that contains the update.

Regards.

4

cman12345 wrote on Thursday, February 19, 2015:

Yes, that aligns with what I would expect. Convention wise, I suppose a prvNotifyQueueSetContainerFromISR() routine that does this would line up with how the other code is organized.

Looking closer, I think this patch is definitely needed because otherwise you would have race conditions in xQueueGenericReceive if you are waiting on a queue set and xQueueGenericSendFromISR is called on a queue that is part of the queue set. In this case, the task list could get edited incorrectly.

Thanks for the response, much appreciated!

5

rtel wrote on Thursday, February 19, 2015:

Yes - that is the conclusion I came to too. The code has been updated
but is still running tests so not checked in yet. However the tests are
not hitting that line - so highlighting an issue in the tests.

Regards.