On Wed, Jan 29, 2014 at 4:48 AM, Beniamino Galvani <b.galv...@gmail.com> wrote:
> On Tue, Jan 28, 2014 at 10:43:28AM +0000, Peter Maydell wrote:
>> On 28 January 2014 00:04, Peter Crosthwaite
>> <peter.crosthwa...@xilinx.com> wrote:
>> > On Tue, Jan 28, 2014 at 4:32 AM, Peter Maydell <peter.mayd...@linaro.org>
>> > wrote:
>> >>> void fifo8_reset(Fifo8 *fifo)
>> >>> {
>> >>> fifo->num = 0;
>> >>> + fifo->head = 0;
>> >>
>> >> This is a bug fix, right? It should go in its own patch.
>> >>
>> >
>> > No bug - where the ring buffer starts following a reset is undefined
>> > and need not be defined. But it improves the predicatability of the
>> > newly added pop_buf fn as you can now following a reset, guarantee
>> > that a single pop_buf will take all contents if its the first pop
>> > (which is how its being used in P2).
>>
>> True. I still think it should have its own patch (and
>> indeed it would be worth saying what you just did as
>> part of the commit message for that patch...)
>
> Ok, I will move the change to a new patch.
>
>> I think it's also nicer for any state that gets migrated
>> to be reset cleanly.
>
> Do you mean also the buffer content? In the emac the tx fifo gets
> reset after each transmission. Isn't this too costly?
>
I would say yes, it is too costly. Out-of-range buffer content is
undefined and there is no good reason that I can see to memset to 0 at
the cost of performance. If we are worried about stale information
leak via VMSD images or anything wierd like that then it should be
handled pre-save and post-load - not here in reset() which is
potentially a fast path.
However resetting the head pointer as you have done in this change is
a win-win, as it's both cleaner (as per Peter's argument) as well as
increasing performance for your use case.
Regards.
Peter
> Beniamino
>
