Re: kern/46780: tty TIOCSQSIZE can free clist buffers which drivers are still using for output

[Dennis Ferguson <dennis%mistimed.com@localhost>]

On 12 Aug, 2012, at 08:25 , Christos Zoulas wrote:

> | While I still think it is possible to do a _flush() entry point I
> | think it is going to take more driver-specific code then you have.
> | The alternative of deferring the free of a non-empty output buffer's
> | memory (the bookkeeping needn't be kept, just the ring buffer memory
> | allocation) until the driver indicates that the hardware/interrupt
> | is inactive by calling ndqb() or ndflush() is probably easier even
> | if it does require some checks to be added to the 'normal' code
> | path.
> 
> Well, we could always wait for ndflush() but I will need to verify that
> this happens. We could also make the drivers voluntarily call a function
> to get the new pointers once they consume all they bytes from the old ones.
> Or we can make the ioctl fail with ebysy if the buffer is not empty.

I only know this because I've been working on that driver, but the
protocol most drivers seem to follow works like this:

- call ndqb() to get the number of bytes available in the clist.  If
  there are some, start the tx interrupt/DMA to send those bytes.

- when the above completes, call ndflush() to remove the bytes sent
  from the clist.

When the driver calls ndqb() or ndflush(), or when the clist byte
count is zero, it should be quite safe to assume there is no tx output
in progress.

Given this I'm pretty sure this might work:

(1) Change all drivers which call ndqb() to copy the tp->t_outq.c_cf
    pointer only after the ndqb() call (or, better, make ndqb() return
    the pointer as well as the count, or something).  This avoids a
    potential race when changing what tp->t_outq.c_cf points to.

(2) When replacing the output queue memory, if the byte count in the clist
    is non-zero save a reference to the old buffer memory (no need to keep the
    bookkeeping, just the buffer itself) rather than freeing it.

(3) If the driver calls ndqb() check to see if an old buffer is present and,
    if it is, free it (before returning information from the current clist,
    as normal).  If the driver is calling ndqb() it is safe to dump the old
    memory.

(4) If the driver calls ndflush() when an old buffer is present, free the old
    buffer and do not flush anything from the current buffer (i.e. ignore the
    byte count).  If the driver is calling ndflush() whatever output it was
    doing has completed, so it is safe to free the old buffer, but if the old
    buffer is present the byte count it is giving you must be from the
    previous buffer and shouldn't be subtracted from the current clist.

(5) While most drivers use the ndqb()/ndflush(), there may be a single
    character _get() routine as well (I'm doing this from memory since
    I'm traveling and away from my sources).  Any code which reads bytes
    from the outq should similarly check for and free the old buffer.

If that's too much work, however, returning EBUSY when the output queue
has bytes in it should also be safe.

Dennis Ferguson