Hello,
I was just going through the code and noticed a possible issue with the fifo
code. I think there will be instances where it is not two process safe. I have
similar code which I got burned on recently. It applies to both read and write.
int fifo_write(struct FIFO *fifo, short data[], int n) {
int i;
short *pdata;
short *pin = fifo->pin;
assert(fifo != NULL);
assert(data != NULL);
if (n > fifo_free(fifo)) {
return -1;
}
else {
/* This could be made more efficient with block copies
using memcpy */
pdata = data;
for(i=0; i<n; i++) {
1. -> *pin++ = *pdata++;
2. -> if (pin == (fifo->buf + fifo->nshort))
pin = fifo->buf;
}
fifo->pin = pin;
}
return 0;
}
1. Depending on compiler/optimizations it is possible that the following
happens:
* pin pointer value will be loaded to a register from memory
* The actual value in memory will be incremented. The pointer increment
is essentially done before storing data
* The value stored in the register is used to store pdata.
The intent of the C code is not violated by this sequence, but the compiler has
no idea this is being used as a 2 process safe code. If this happens and the
reading process runs at just the right time it could read out invalid data. To
get around this make pin volatile and do the increment after the data transfer
(the volatile will ensure the compiler maintains the order you want).
1. At this point it's possible for pin to point to invalid memory. If the
reading process gets context at this instant there could be a lot of invalid
data read out. This one is harder to fix. Typically I use free running offsets
instead of pointers for this type of code. Also my code requires modulus, but
to make it more efficient I make the Fifo size be a power of 2 so that I can
use the and operator (ulMyBufferMask is fifo size -1)
ULONG
Length()
{
return (((ULONG)(ulMyHead - ulMyTail)) & ulMyBufferMask);
}
ULONG
FreeSpace()
{
return (ulMyBufferMask - Length());
}
BOOL
Put(
UCHAR* pucBuf_, //The buffer to store
USHORT usLen_) //Number of elements in the passed in buffer that will be
stored
{
//Check there is enough room for the entire buffer
if (usLen_ > FreeSpace())
return FALSE;
//Store buffer
for (USHORT i = 0; i < usLen_; i++)
{
pucMyBuffer[ulMyHead & ulMyBufferMask] = pucBuf_[i];
ulMyHead++;
}
return TRUE;
}
If I misunderstood the code and there are no issues please ignore my long
blurb. Just bringing this up as the comments indicate the Fifo is meant to be 2
process safe. Data corruption created by these types of bugs will be very
timing dependent and infuriating to debug so hopefully this helps.
Cheers,
Marko
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Freetel-codec2 mailing list
Freetel-codec2@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freetel-codec2
