On Wed, 6 Aug 2008 11:53:13 +0100
"Will Newton" <will.newton@xxxxxxxxx> wrote:
> >From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
> From: Will Newton <will.newton@xxxxxxxxx>
> Date: Wed, 6 Aug 2008 11:48:29 +0100
> Subject: [PATCH] 8250: Improve workaround for UARTs that don't
> re-assert THRE correctly.
>
> Recent changes to tighten the check for UARTs that don't correctly
> re-assert THRE caused problems when such a UART was opened for the second
> time - the bug could only successfully be detected at first initialization.
> This patch stores the information about the bug in the bugs field of the
> port structure when the port is first started up so subsequent opens can
> check this bit even if the test for the bug fails.
>
> Signed-off-by: Will Newton <will.newton@xxxxxxxxx>
> Acked-by: Alex Williamson <alex.williamson@xxxxxx>
What are the "recent changes" to which you refer? I had a quick look
and didn't spot any THRE-related changes this year?
> ---
> drivers/serial/8250.c | 16 ++++++++++++----
> drivers/serial/8250.h | 1 +
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 342e12f..9ccc563 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1908,15 +1908,23 @@ static int serial8250_startup(struct uart_port *port)
> * kick the UART on a regular basis.
> */
> if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
> + up->bugs |= UART_BUG_THRE;
> pr_debug("ttyS%d - using backup timer\n", port->line);
> - up->timer.function = serial8250_backup_timeout;
> - up->timer.data = (unsigned long)up;
> - mod_timer(&up->timer, jiffies +
> - poll_timeout(up->port.timeout) + HZ / 5);
> }
> }
>
> /*
> + * The above check will only give an accurate result the first time
> + * the port is opened so this value needs to be preserved.
> + */
> + if (up->bugs & UART_BUG_THRE) {
> + up->timer.function = serial8250_backup_timeout;
> + up->timer.data = (unsigned long)up;
> + mod_timer(&up->timer, jiffies +
> + poll_timeout(up->port.timeout) + HZ / 5);
> + }
> +
> + /*
> * If the "interrupt" for this port doesn't correspond with any
> * hardware interrupt, we use a timer-based system. The original
> * driver used to do this with IRQ0.
> diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h
> index 78c0016..5202603 100644
> --- a/drivers/serial/8250.h
> +++ b/drivers/serial/8250.h
> @@ -47,6 +47,7 @@ struct serial8250_config {
> #define UART_BUG_QUOT (1 << 0) /* UART has buggy quot LSB */
> #define UART_BUG_TXEN (1 << 1) /* UART has buggy TX IIR status */
> #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */
> +#define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */
>
> #define PROBE_RSA (1 << 0)
> #define PROBE_ANY (~0)
Also, how serious is the problem which is being fixed here? It
_sounds_ like it's of the "fatal for people who have that hardware"
variety, in which case we should get this into 2.6.27 and probably
2.6.26.x. Not sure about 2.5.26.x though - the patch doesn't apply
there, but I didn't check whether this is due to functional changes.
Also2, we should officially use setup_timer(), like this:
--- a/drivers/serial/8250.c~serial-8250-tighten-test-for-using-backup-timer-fix
+++ a/drivers/serial/8250.c
@@ -1964,8 +1964,8 @@ static int serial8250_startup(struct uar
* the port is opened so this value needs to be preserved.
*/
if (up->bugs & UART_BUG_THRE) {
- up->timer.function = serial8250_backup_timeout;
- up->timer.data = (unsigned long)up;
+ setup_timer(&up->timer, serial8250_backup_timeout,
+ (unsigned long)up);
mod_timer(&up->timer, jiffies +
poll_timeout(up->port.timeout) + HZ / 5);
}
but that is a functional change - setup_timer() runs init_timer(),
whereas the code you have there does not.
We _do_ have an init_timer(&up->timer) all the way over in
serial8250_isa_init_ports(). It's all a bit weird. Could you please
double-check that we're being sensible about the initialisation of this
timer?
Thanks.
--
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
