I've been trying to get Prasad's "don't block Rx during emergency Tx"
feature into a form that Peter would be happy with, and the big
challenge is finding somewhere to put the received characters without
taking any locks.
This has led me into figuring out the whole tty_buffer system,
and unless someone objects, I'm going to submit a comment patch
for that because the locking rules are not entirely obvious.
It's also not clear that "used" and "read" have to be in the tty_buffer
at all. The only instances that are ever used are bufhead->tail->used,
and bufhead->head->read, so they could be moved to the bufhead structure.
And the sentinel is a bit strange and probably unnecessary.
Anyway, the steady state will typically be two 512-byte (size = 256)
buffers that are ping-ponged. So there should always be at least
256 bytes of buffer space available without needing to allocate anything.
I'll have to add some features to the tty_buffer interface to support a
no-allocation interface that reports if space is available and uses it
if it is. A bit invasive, but it seems like the only clean way.
A more immediate and specific concern I've been having is the temporary
unlock
spin_unlock(&port->lock);
tty_flip_buffer_push(&port->state->port);
spin_lock(&port->lock);
at the end of serial8250_rx_chars(). I think there's actually a bug
there.
The unlock/lock itself is not the bug. I think it's unnecessary and
could be deleted, but that wouldn't affect the bug: the failure to set
lsr_saved_flags if --max_count expires.
At that point in the code, up->lsr_saved_flags has been set to zero,
but if the character on top of the FIFO has a parity or framing error
when max_count expires, the error will be lost and the character received
as normal.
serial8250_rx_chars does return the LSR bits, but serial8250_handle_irq
doesn't do anything with the returned LSR except decide if it should
call serial8250_tx_chars. The other LSR bits aren't saved. So if the
max_count expires leaving a character with a parity or framing error on
top of the FIFO, the error will get lost.
This could be fixed fairly easily by having serial8250_rx_chars save the
unprocessed LSR bits before unlocking.
Another possible fix would be to test --max_count *before*
re-reading the LSR, so the errors would stay in the FIFO.
(Looking for other users, fsl8250_handle_irq sets lsr_saved_flags based
on the orig_lsr before serial8250_rx_chars was called.... and I think
it's wrong.)
Regarding the need for the unlock in the first place, I'm really
not seeing it. tty_flip_buffer_push() is just an alias for
tty_schedule_flip(), which just fills in port->buf.tail->commit and
calls schedule_work(port->buf.work).
So not only is nothing done immediately, but even the eventual
call to flush_to_ldisc() doesn't take port->lock.
Although it wastes time, I don't think the unlock/lock
introduces any bugs.
A serial8250_console_write could sneak in during the unlock, but
only the transmit bits matter, and the consle_write code is careful
to leave them both set, which the hardware is allowed to do
asynchronously anyway, so worst case is the interrupt handler
will be immediately re-invoked after it returns.
Anyway, can a second pair of eyes confirm that my analysis here
is correct?
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
