Re: [Qemu-devel] [PATCH v2 1/1] char/serial: Fix emptyness handling

[Peter Crosthwaite]

On Tue, Mar 18, 2014 at 5:29 AM, Don Slutz <address@hidden> wrote:
> On 03/16/14 22:21, Peter Crosthwaite wrote:
>>
>> On Thu, Feb 27, 2014 at 12:48 PM, Don Slutz <address@hidden> wrote:
>>>
>>> The commit 88c1ee73d3231c74ff90bcfc084a7589670ec244
>>> char/serial: Fix emptyness check
>>>
>>> Still causes extra NULL byte(s) to be sent.
>>>
>>> So if the fifo is empty, do not send an extra NULL byte.
>>> Do full state change on fifo8_is_empty.
>>>
>>> Signed-off-by: Don Slutz <address@hidden>
>>> ---
>>> v1 to v2:  Do all the state changes that would have been done sending the
>>> NULL byte.
>>>
>>>   hw/char/serial.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>>> index 6d3b5af..7af3c1b 100644
>>> --- a/hw/char/serial.c
>>> +++ b/hw/char/serial.c
>>> @@ -225,8 +225,13 @@ 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_empty(&s->xmit_fifo) ?
>>> -                        0 : fifo8_pop(&s->xmit_fifo);
>>> +            if (fifo8_is_empty(&s->xmit_fifo)) {
>>> +                s->lsr |= UART_LSR_THRE | UART_LSR_TEMT;
>>> +                s->thr_ipending = 1;
>>> +                serial_update_irq(s);
>>> +                return FALSE;
>>> +            }
>>
>> This is copy paste of the UART_LSR_THRE handler code below. The
>> implementation is now somewhat inconsistent with non-fifo mode with
>> the mixed stage handling of empty fifo/hold-reg. Perhaps you could:
>>
>> if (s->fcr & UART_FCR_FE && fifo8_is_empty(&s->xmit_fifo)) {
>>     s->lsr |= UART_LSR_THRE;
>> }
>>
>> after a a successful transmit? Then let the existing check of
>> UART_LSR_THRE catch for your irq updates etc.
>
>
> If I am understanding correctly, you would like v1 of this patch
> (02/19/2014)
> which just does a "return FALSE;" in this case.
>
> At the posting of this version, I was still unsure of the steps that can get
> you
> into this case.  Since then I have come up with:
>
> If I have the right steps leading to this case, you need to have at least 2
> (
> maybe more) xmit overruns just before xmit starts working.  At which time
> there are more calls outstanding via qemu_chr_fe_add_watch() then there
> are bytes in the FIFO when all the timing lines up.

Is this perhaps a bug? Fifo overrun is known to the code via some if
logic already:

            if(s->fcr & UART_FCR_FE) {
                /* xmit overruns overwrite data, so make space if needed */
                if (fifo8_is_full(&s->xmit_fifo)) {
                    fifo8_pop(&s->xmit_fifo);
                }

Should we just use this same condition to inhibit the call to
serial_xmit ensuring that there are number more watches than FIFO
bytes?

Then again, it's correct for callbacks to check that they are still
relevant once they happen (any guest driven side effects could happen
in that time, making the call back unwanted) so perhaps we should just
do both checks. A misbehaving guest could create a flood of these
watches.

> So when the last byte
> is
> successfully transmitted, at least 1 more call will be done to serial_xmit
> with
> an empty FIFO.
>
> So now v1 looks to be the better fix.
>

Yeh, so I'm thinking the existing code should still work for the
interrupt side effects etc for successful last-byte transmit. I have
put an RB to V1. These extraneous watches should have 0 side effects.

> With no input, I decided that doing what the code would have done on a
> successful transmit of an extra NULL was better.
>
>
>
>>> +            s->tsr = fifo8_pop(&s->xmit_fifo);
>>>               if (!s->xmit_fifo.num) {
>>>                   s->lsr |= UART_LSR_THRE;
>>>               }
>>
>> I think this is this now dead code. I think (!s->xmit_fifo.num) is the
>> same condition as fifo8_is_empty().
>
>
> This is not dead code.  And yes (!s->xmit_fifo.num) is the same
>  condition.  However, the pop 1 line before can change the state
> to empty.
>

Yeh, my bad.

> I am happy to also use fifo8_is_empty() here if a v3 is needed.  Just going
> for the minimal change.  I can also make it a separate patch if wanted.
>

If you would like. V1 as-is however self standing and addresses the
core issue completely for me. Thanks for the debug.

Regards,
Peter

>
>
>>
>> Sorry about the delayed response. Thanks for the patch.
>
>
> No problem.
>
>     -Don Slutz
>
>> Regards,
>> Peter
>>
>>> --
>>> 1.8.4
>>>
>>>
>
>