Re: [4/4] ntdll: Properly set flag which indicates buffer empty state.

[Wolfgang Walter]

Hello Dimitry,

I think I now found the difference between my patches and yours and what makes 
that one application fail (with or without [4/4] ntdll: Properly set flag 
which indicates buffer empty state.)

My patch removes the attempt to flag EV_TXEMPTY only once.

In check_events() EV_TXEMPTY is set:

if (mask & EV_TXEMPTY)
    {
        if (!old->temt && new->temt)
            ret |= EV_TXEMPTY;
    }

I think that happens:

* application writes data to com port.
* all is written, serial buffer is empty
* application calls  WaitCommEvent()
* wait_on() is called
* wait_on() calls get_irq_info()
* get_irq_info() sets commio->irq_info->temt = 1
* wait_on() calls check_events() and uses sets commio->irq_info for old an new
* so old->temt == new->temt and EV_TXEMPTY is not set
* if there are no other events (in real world basically EV_RXCHAR):
* wait_for_event() is startet with commio->irq_info->temt set to one
* wait_for_event() calls get_irq_info() with new_irq_info()
* get_irq_info() sets new_irq_info->temt = 1 because the buffer is still empty
* wait_for_event() calls check_events() with new_irq_info and commio->irq_info
* again check_events() will not set EV_TXEMPTY as both have temt == 1

It seems that we do not recover from that hang.

Please correct me if I misread the code and I'm wrong.

I think it's better if WaitCommEvent() returns with EV_TXEMPTY even if there 
has no new data been sent in between:

* if an application waits only for EV_TXEMPTY then there is no problem.
* if it waits for i.e. EV_TXEMPTY|EV_RXEMPTY it may busy loops. Still better 
then not waiting forever.

Sending EV_TXEMPTY only once after the write buffer got empty is not easy to 
implement I think. One way would be: block a write until it finished and 
buffer is empty, the set a flag.

Am Mittwoch, 28. August 2013, 12:41:00 schrieb Dmitry Timoshkov:
> Dmitry Timoshkov  wrote:
> > Alexandre Julliard  wrote:
> > > It doesn't work here:
> > >
> > > ../../../tools/runtest -q -P wine -M kernel32.dll -T ../../.. -p
> > > kernel32_test.exe.so comm.c && touch comm.ok comm.c:835: Test failed:
> > > WriteFile took 1 ms to write 17 Bytes at 150 Baud comm.c:848: Test
> > > failed: WaitCommEvent failed with a timeout
> > > comm.c:859: Test failed: WaitCommEvent error 997
> > > comm.c:860: Test failed: WaitCommEvent: expected EV_TXEMPTY, got 0
> > > comm.c:865: Test failed: WaitCommEvent used 1000 ms for waiting
> > > comm.c:1895: Tests skipped: interactive tests (set
> > > WINETEST_INTERACTIVE=1)
> > > make: *** [comm.ok] Error 5
> >
> > I assume it's a real hardware and not a VM? Is this with a real COM port,
> > or USB-serial cable? If the latter one what driver is it using?
>
> Looking at the failure messages above once again, I can say that WriteFile
> failure is definitely not caused by this patch. Does running the test
> several times cause the same failures? In any case it would be interesting
> to see the +comm log with "ntdll: Add a trace for transmitter's buffer
> empty flag." applied.
>
> And current logic of setting the empty buffer flag is really broken, it
> always sets the flag to a not zero value if ioctl(TIOCOUTQ) succeeds.

Regards,