Re: [PATCH v2 01/25] fifo32: add peek function

[Octavian Purdila <tavip@google.com>]

On Tue, Oct 8, 2024 at 4:27 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 08/10/2024 02:18, Octavian Purdila wrote:
>
> > Add fifo32_peek() that returns the first element from the queue
> > without popping it.
> >
> > Signed-off-by: Octavian Purdila <tavip@google.com>
> > ---
> >   include/qemu/fifo32.h | 28 ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/include/qemu/fifo32.h b/include/qemu/fifo32.h
> > index 4e9fd1b5ef..9de1807375 100644
> > --- a/include/qemu/fifo32.h
> > +++ b/include/qemu/fifo32.h
> > @@ -140,6 +140,34 @@ static inline uint32_t fifo32_pop(Fifo32 *fifo)
> >       return ret;
> >   }
> >
> > +/**
> > + * fifo32_peek:
> > + * @fifo: fifo to peek at
> > + *
> > + * Returns the value from the FIFO's head without poping it. Behaviour
> > + * is undefined if the FIFO is empty. Clients are responsible for
> > + * checking for emptiness using fifo32_is_empty().
> > + *
> > + * Returns: the value from the FIFO's head
> > + */
> > +
> > +static inline uint32_t fifo32_peek(Fifo32 *fifo)
> > +{
> > +    uint32_t ret = 0, num;
> > +    const uint8_t *buf;
> > +
> > +    buf = fifo8_peek_bufptr(&fifo->fifo, 4, &num);
>
> Are you sure that you want to use fifo8_peek_bufptr() as opposed to fifo8_peek_buf()
> here? The reason for using the latter function (and why fifo8_*_bufptr() functions
> are not generally recommended) is that they will correctly handle the FIFO wraparound
> caused by the drifting head pointer which can occur if you don't empty the entire
> FIFO contents in a single *_pop() or *_pop_buf() operation.
>

I don't think that it matters in this case because the size of the
FIFO is always going to be a multiple of 4 and all push and pop
operations happen with 4 bytes as well. Am I missing something?

In any case, if it makes things more clear / consistent I can switch
to fifo8_peek_buf.

> > +    if (num != 4) {
> > +        return ret;
> > +    }
> > +
> > +    for (int i = 0; i < sizeof(uint32_t); i++) {
> > +        ret |= buf[i] << (i * 8);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> >   /**
> >    * There is no fifo32_pop_buf() because the data is not stored in the buffer
> >    * as a set of native-order words.
>
>
> ATB,
>
> Mark.
>