On Mon, Aug 5, 2019 at 10:22 AM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Mon, Aug 05, 2019 at 09:49:24AM -0400, Robert Middleton wrote: > > Thanks for testing, my comments below. > > > Unfortunately this will re-introduce the bug that it was attempting to > > solve, that is ensuring that the buffer in the kernel and the buffer > > on the chip are clear before going into shutdown on the chip. > > Breaking at the beginning of the loop means that the kernel has > > written everything to the internal buffer on the chip, but until the > > LSR bits are clear the bytes have not been transmitted yet. > > So, the difference here, that you have a long delay with mdelay(1) which > "fixes" your issue. The important fix is really this part: lsr = serial_in(up, UART_LSR); if (lsr & (UART_LSR_TEMT | UART_LSR_THRE)) Both of those bits must be set before the shutdown can occur. The msleep(1) is so that it is not a busy loop(maybe usleep_range would be better?). If the transmitter is not empty and the transmitter FIFO is not empty, when the shutdown occurs the remaining data will be discarded by the exar chip. Assuming the FIFO on the exar is full(256 bytes to transmit), it would take ~250ms to transmit all of the bytes at 9600 baud, so our check would happen ~250 times(assuming mdelay(1) sleeps for 1ms and not up to 20). This is based off of the reference driver from Max Linear: https://github.com/rm5248/exar-driver/blob/5dcbac6aa564cf5604512ecd6c9e02c577b70fa2/xr17v35x.c#L1282 > > I guess the third patch in this series makes it again not-working. > Can you check and confirm that? I don't see a third patch. > > Or even better, replace entire loop with one usleep_range() call and play with > numbers there, like (10, 20), (100, 150), (1000, 1100). Probably you can start > with udelay(2) followed up by above list. > > If my theory is correct you will see at some point the problem will disappear. It would probably disappear, but if you remove the entire loop and LSR check you can't be sure that the data has actually been transmitted out of the exar chip. Likely this would result in a different number of bytes being dropped depending on the baud rate and how large the buffer was when shutdown occurs. > > > I'm not positive that the uart_circ_empty needs to be checked in the > > first place; I had put it in because the serial8250_tx_chars does that > > before stopping the tx, and I assume that there could be a potential > > race condition where the kernel has not yet written all the data to > > the exar, but the exar has finished transmitting all the data in its > > transmit buffer(I am not sure how likely this is to happen). > > tty gets uninitialized before ->shutdown() happen, it also set's TTY IO error > condition, which has been checked in tty_write(). I'm sure new data will not > come at this point. That makes sense; thanks for the information. The TTY layer is rather complicated for somebody like me who doesn't look at it a lot. -Robert Middleton
