[PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

[PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

[Jiri Kosina]

The patch below has been lost several times (the last submission happened 
probably on [1]). I am currently aware of a real hardware which triggers 
this problem quite reliably, so we should rather have that really fixed.

[1] http://lkml.org/lkml/2009/3/12/379




From: Ian Jackson <ian.jackson@eu.citrix.com>

Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

Reading the IIR clears some oustanding interrupts so it is not safe.
Instead, simply transmit immediately if the buffer is empty without
regard to IIR.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Jiri Kosina <jkosina@suse.cz>

---
 drivers/serial/8250.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 737b4c9..807042b 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1339,14 +1339,12 @@ static void serial8250_start_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 
 		if (up->bugs & UART_BUG_TXEN) {
-			unsigned char lsr, iir;
+			unsigned char lsr;
 			lsr = serial_in(up, UART_LSR);
 			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-			iir = serial_in(up, UART_IIR) & 0x0f;
 			if ((up->port.type == PORT_RM9000) ?
-				(lsr & UART_LSR_THRE &&
-				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
-				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
+				(lsr & UART_LSR_THRE) :
+				(lsr & UART_LSR_TEMT))
 				transmit_chars(up);
 		}
 	}

Re: [PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

[Jiri Kosina]

On Wed, 18 Nov 2009, Jiri Kosina wrote:

> The patch below has been lost several times (the last submission happened 
> probably on [1]). I am currently aware of a real hardware which triggers 
> this problem quite reliably, so we should rather have that really fixed.
> 
> [1] http://lkml.org/lkml/2009/3/12/379
> 
> 
> 
> 
> From: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Do not read IIR in serial8250_start_tx when UART_BUG_TXEN
> 
> Reading the IIR clears some oustanding interrupts so it is not safe.
> Instead, simply transmit immediately if the buffer is empty without
> regard to IIR.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> 
> ---
>  drivers/serial/8250.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 737b4c9..807042b 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1339,14 +1339,12 @@ static void serial8250_start_tx(struct uart_port *port)
>  		serial_out(up, UART_IER, up->ier);
>  
>  		if (up->bugs & UART_BUG_TXEN) {
> -			unsigned char lsr, iir;
> +			unsigned char lsr;
>  			lsr = serial_in(up, UART_LSR);
>  			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> -			iir = serial_in(up, UART_IIR) & 0x0f;
>  			if ((up->port.type == PORT_RM9000) ?
> -				(lsr & UART_LSR_THRE &&
> -				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
> -				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
> +				(lsr & UART_LSR_THRE) :
> +				(lsr & UART_LSR_TEMT))
>  				transmit_chars(up);
>  		}
>  	}

Does anyone have any comments about this please?

In a nutshell -- this is needed so that we make the UART_BUG_TXEN really 
harmless (which I guess it originally inteded to be, but reading IIR has 
some unwanted sideeffects and is in fact not needed).

We need to have this in to handle properly the cases in which BUG_TXEN is 
misdetected, and we can't blacklist such systems as we do for some SoL 
hardware (see commit b6adea334c6c (" 8250: fix boot hang with serial 
console when using with Serial Over Lan port"). Also there doesn't seem to 
be any straightforward way to workaround the misdetection, so this seems 
to be proper fix, unbreaking all the possible scenarios.

Alan, Greg, any comments?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

[Greg KH]

On Thu, Nov 19, 2009 at 05:38:55PM +0100, Jiri Kosina wrote:
> On Wed, 18 Nov 2009, Jiri Kosina wrote:
> 
> > The patch below has been lost several times (the last submission happened 
> > probably on [1]). I am currently aware of a real hardware which triggers 
> > this problem quite reliably, so we should rather have that really fixed.
> > 
> > [1] http://lkml.org/lkml/2009/3/12/379
> > 
> > 
> > 
> > 
> > From: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Do not read IIR in serial8250_start_tx when UART_BUG_TXEN
> > 
> > Reading the IIR clears some oustanding interrupts so it is not safe.
> > Instead, simply transmit immediately if the buffer is empty without
> > regard to IIR.
> > 
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > 
> > ---
> >  drivers/serial/8250.c |    8 +++-----
> >  1 files changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> > index 737b4c9..807042b 100644
> > --- a/drivers/serial/8250.c
> > +++ b/drivers/serial/8250.c
> > @@ -1339,14 +1339,12 @@ static void serial8250_start_tx(struct uart_port *port)
> >  		serial_out(up, UART_IER, up->ier);
> >  
> >  		if (up->bugs & UART_BUG_TXEN) {
> > -			unsigned char lsr, iir;
> > +			unsigned char lsr;
> >  			lsr = serial_in(up, UART_LSR);
> >  			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> > -			iir = serial_in(up, UART_IIR) & 0x0f;
> >  			if ((up->port.type == PORT_RM9000) ?
> > -				(lsr & UART_LSR_THRE &&
> > -				(iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
> > -				(lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
> > +				(lsr & UART_LSR_THRE) :
> > +				(lsr & UART_LSR_TEMT))
> >  				transmit_chars(up);
> >  		}
> >  	}
> 
> Does anyone have any comments about this please?
> 
> In a nutshell -- this is needed so that we make the UART_BUG_TXEN really 
> harmless (which I guess it originally inteded to be, but reading IIR has 
> some unwanted sideeffects and is in fact not needed).
> 
> We need to have this in to handle properly the cases in which BUG_TXEN is 
> misdetected, and we can't blacklist such systems as we do for some SoL 
> hardware (see commit b6adea334c6c (" 8250: fix boot hang with serial 
> console when using with Serial Over Lan port"). Also there doesn't seem to 
> be any straightforward way to workaround the misdetection, so this seems 
> to be proper fix, unbreaking all the possible scenarios.
> 
> Alan, Greg, any comments?

At first glance, it looks acceptable to me.  It's in my "to-apply" queue
that has gotten a bit big due to "real work" and a vacation I took last
week.

thanks,

greg k-h

Re: [PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN

[Ian Jackson]

Jiri Kosina writes ("Re: [PATCH] Do not read IIR in serial8250_start_tx when UART_BUG_TXEN"):
> Does anyone have any comments about this please?
> 
> In a nutshell -- this is needed so that we make the UART_BUG_TXEN really 
> harmless (which I guess it originally inteded to be, but reading IIR has 
> some unwanted sideeffects and is in fact not needed).

Yes.

> We need to have this in to handle properly the cases in which BUG_TXEN is 
> misdetected, and we can't blacklist such systems as we do for some SoL 
> hardware (see commit b6adea334c6c (" 8250: fix boot hang with serial 
> console when using with Serial Over Lan port"). Also there doesn't seem to 
> be any straightforward way to workaround the misdetection, so this seems 
> to be proper fix, unbreaking all the possible scenarios.

Unless the code has changed, the UART_BUG_TXEN detection is
fundamentally broken and racy.  So fixing that too would be good (and
possible, when I looked at it).

But even without fixing the UART_BUG_TXEN detection, the IIR read is
absolutely and definitely wrong.  The only purpose of the IIR read is
to avoid an unnecessary but harmless call to transmit_chars.

It seems to me that the author of the UART_BUG_TXEN patch did not
appreciate that reading the IIR was not side effect free, and that
they were just trying a minor (and perhaps ill-advised) optimisation.

In any case we don't have any useful information (in comments or
commit logs) explaining why they did it this way and it is (a) easily
seen to be buggy and (b) causes actual real problems.

So my patch should be applied.  The last time we discussed this we had
some kind of FUD along the lines of "but real serial ports often have
bugs so you can't reason about them properly".  I don't think that's a
sensible basis for discussion.  How can it be falsified ?

If there is some specific bug that real ports have that might be
relevant, sure, take it into account.  But we should not have the
situation where we hear the mere assertion that there often are bugs;
therefore reasoning doesn't show change to be safe; therefore no
change can be made.

Ian.