It is super useful 😊
-----Original Message-----
From: David Rowe <da...@rowetel.com>
Sent: June 30, 2018 12:41 AM
To: freetel-codec2@lists.sourceforge.net
Subject: Re: [Freetel-codec2] Possible Issue With Fifo.c
Hi Marco,
Well actually that's good news. Thank you for the comprehensive review!
I've used this FIFO design for 20 years as an interface between
processes/threads and even two separate physical CPUs that have shared physical
memory. Like your use case a common application is between interrupt service
routines to the main process.
The design doesn't require any synchronisation primitives (like flags, locks or
mutexes) which is very handy.
Cheers,
David
On 30/06/18 13:59, Marko Milutinovic wrote:
> Hello,
>
> I'm sorry I didn't read the code closely enough. You don't update the 'pin'
> and 'pout' pointers in the fifo struct until after all data is transferred ,
> only the local copies are being modified while actually copying data. None of
> the comments I made earlier apply ...
>
> Please disregard previous emails.
>
> Cheers,
> Marko
>
> -----Original Message-----
> From: Marko Milutinovic <mmilutino...@blacklinesafety.com>
> Sent: June 29, 2018 9:26 PM
> To: freetel-codec2@lists.sourceforge.net
> Subject: Re: [Freetel-codec2] Possible Issue With Fifo.c
>
> It would be very subtle, I only ran into an issue in my code when I switched
> from the STM32F4 line to the STM32H7 line. I use Fifos like this for
> transferring data from interrupts to code.
>
> I'll write a unit test to try and produce this. Two threads at different
> priorities should do it eventually.
>
> -----Original Message-----
> From: David Rowe <da...@rowetel.com>
> Sent: June 29, 2018 4:53 PM
> To: freetel-codec2@lists.sourceforge.net
> Subject: Re: [Freetel-codec2] Possible Issue With Fifo.c
>
> Hi Marko,
>
> Thank you for that analysis. I used this fifo design a lot, e.g in FreeDV
> GUI and the SM1000. No current issues that I am aware of but they could be
> subtle.
>
> What specific changes to fifo.c would you suggest?
>
> Is there a unit test that can demonstrate the problem?
>
> Thanks,
>
> David
>
> On 29/06/18 13:56, Marko Milutinovic wrote:
>> 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:
>> 1. *pin* pointer value will be loaded to a register from memory
>> 2. The actual value in memory will be incremented. The pointer
>> increment is essentially done before storing data
>> 3. 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).
>>
>> Â
>>
>> 2. 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
>>
>
> ----------------------------------------------------------------------
> -------- 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
>
> ----------------------------------------------------------------------
> -------- 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
>
> ----------------------------------------------------------------------
> -------- 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
>
------------------------------------------------------------------------------
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
------------------------------------------------------------------------------
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
