[Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptyness check

[Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptyness check

[Peter Crosthwaite]

This was guarding against a full fifo rather than an empty fifo when
popping. Fix.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 27dab7d..6d3b5af 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -225,7 +225,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 
     if (s->tsr_retry <= 0) {
         if (s->fcr & UART_FCR_FE) {
-            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
+            s->tsr = fifo8_is_empty(&s->xmit_fifo) ?
                         0 : fifo8_pop(&s->xmit_fifo);
             if (!s->xmit_fifo.num) {
                 s->lsr |= UART_LSR_THRE;
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptyness check

[Dr. David Alan Gilbert]

* Peter Crosthwaite (peter.crosthwaite@xilinx.com) wrote:
> This was guarding against a full fifo rather than an empty fifo when
> popping. Fix.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I think this brings it back to how it was on the old FIFO code.

> 
>  hw/char/serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 27dab7d..6d3b5af 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -225,7 +225,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>  
>      if (s->tsr_retry <= 0) {
>          if (s->fcr & UART_FCR_FE) {
> -            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
> +            s->tsr = fifo8_is_empty(&s->xmit_fifo) ?
>                          0 : fifo8_pop(&s->xmit_fifo);
>              if (!s->xmit_fifo.num) {
>                  s->lsr |= UART_LSR_THRE;
> -- 
> 1.8.5.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptynesscheckk

[Alex Bennée]

Dr. David Alan Gilbert <dgilbert@redhat.com> writes:

> * Peter Crosthwaite (peter.crosthwaite@xilinx.com) wrote:
>> This was guarding against a full fifo rather than an empty fifo when
>> popping. Fix.
>> 
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> I think this brings it back to how it was on the old FIFO code.

This does make me wonder why it was changed? Was there another crashing
case it fixed?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptynesscheckk

[Dr. David Alan Gilbert]

* Alex Benn?e (alex.bennee@linaro.org) wrote:
> 
> Dr. David Alan Gilbert <dgilbert@redhat.com> writes:
> 
> > * Peter Crosthwaite (peter.crosthwaite@xilinx.com) wrote:
> >> This was guarding against a full fifo rather than an empty fifo when
> >> popping. Fix.
> >> 
> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> >> ---
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > I think this brings it back to how it was on the old FIFO code.
> 
> This does make me wonder why it was changed? Was there another crashing
> case it fixed?

The old serial code had it's own FIFO implementation, and that was
removed so that we had one less FIFO in the codebase; the problem
was that the old one in the serial code just returned 0 when empty,
the shared one aborts.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptyness check

[Martin Kletzander]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Mon, Feb 10, 2014 at 10:49:35PM -0800, Peter Crosthwaite wrote:
> This was guarding against a full fifo rather than an empty fifo when
> popping. Fix.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

With this patch qemu doesn't crash in my use-case.

>
>  hw/char/serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 27dab7d..6d3b5af 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -225,7 +225,7 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
>
>      if (s->tsr_retry <= 0) {
>          if (s->fcr & UART_FCR_FE) {
> -            s->tsr = fifo8_is_full(&s->xmit_fifo) ?
> +            s->tsr = fifo8_is_empty(&s->xmit_fifo) ?
>                          0 : fifo8_pop(&s->xmit_fifo);
>              if (!s->xmit_fifo.num) {
>                  s->lsr |= UART_LSR_THRE;
> --
> 1.8.5.4
>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH bugfix v1 1/1] char/serial: Fix emptyness check

[Michael Tokarev]

11.02.2014 10:49, Peter Crosthwaite wrote:
> This was guarding against a full fifo rather than an empty fifo when
> popping. Fix.

I've applied this to qemu-trivial for now, if no one will pick it up
before the next -trivial pull, -- because the original change were
merged using -trivial too.

I also compared this with the previous code in this area, and it looks
like this is the right fix.  So you can add my

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

too.

Thank you!

/mjt