[PATCH 2/2] alpha: add a delay before serial port read

[PATCH 2/2] alpha: add a delay before serial port read

[Mikulas Patocka]

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 drivers/tty/serial/8250/8250_port.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
@@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+#ifdef CONFIG_ALPHA
+/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
+	ndelay(300);
+#endif
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }

Re: [PATCH 2/2] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Wed, May 06, 2020 at 07:23:31AM -0400, Mikulas Patocka wrote:
> The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> the Alpha Avanti platform.
> 
> The patch changes timing between accesses to the ISA bus, in particular,
> it reduces the time between "write" access and a subsequent "read" access.
> 
> This causes lock-up when accessing the real time clock and serial ports.
> 
> This patch fixes the serial ports by adding a small delay before the "inb"
> instruction.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  drivers/tty/serial/8250/8250_port.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> ===================================================================
> --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
> +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
> @@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
>  
>  static unsigned int io_serial_in(struct uart_port *p, int offset)
>  {
> +#ifdef CONFIG_ALPHA
> +/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
> +	ndelay(300);
> +#endif

We really do not like #ifdef in .c files, especially ones that cause a
coding style violation :)

Why can't you do this as a quirk for this specific chipset?  You should
tie it to the serial port hardware type, not to the CPU type.

thanks,

greg k-h

[PATCH 2/2 v2] alpha: add a delay before serial port read

[Mikulas Patocka]

On Wed, 6 May 2020, Greg Kroah-Hartman wrote:

> > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > ===================================================================
> > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
> > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
> > @@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
> >  
> >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> >  {
> > +#ifdef CONFIG_ALPHA
> > +/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
> > +	ndelay(300);
> > +#endif
> 
> We really do not like #ifdef in .c files, especially ones that cause a
> coding style violation :)
> 
> Why can't you do this as a quirk for this specific chipset?  You should
> tie it to the serial port hardware type, not to the CPU type.
> 
> thanks,
> 
> greg k-h

Do you want this patch? It enables the delay based on the specific PCI-ISA 
bridge.

There is still "#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)" because 
if we want to reference a variable defined by the PCI subsystem in the 
arch/alpha tree, we must do it conditionally. If you want to get rid of 
these #ifs, please describe how.

Mikulas


From: Mikulas Patocka <mpatocka@redhat.com>

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 arch/alpha/include/asm/pci.h        |    2 ++
 arch/alpha/kernel/pci.c             |    4 ++++
 drivers/tty/serial/8250/8250_core.c |    7 +++++++
 drivers/tty/serial/8250/8250_port.c |    3 +++
 include/linux/serial_core.h         |    1 +
 5 files changed, 17 insertions(+)

Index: linux-stable/include/linux/serial_core.h
===================================================================
--- linux-stable.orig/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
+++ linux-stable/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
@@ -154,6 +154,7 @@ struct uart_port {
 
 	/* quirks must be updated while holding port mutex */
 #define UPQ_NO_TXEN_TEST	BIT(0)
+#define UPQ_DELAY_BEFORE_READ	BIT(1)
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
Index: linux-stable/drivers/tty/serial/8250/8250_core.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
@@ -37,6 +37,9 @@
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif
+#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
+#include <linux/pci.h>
+#endif
 
 #include <asm/irq.h>
 
@@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
 static inline void serial8250_apply_quirks(struct uart_8250_port *up)
 {
 	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
+#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
+	if (alpha_serial_port_needs_delay)
+		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
+#endif
 }
 
 static void __init serial8250_isa_init_ports(void)
Index: linux-stable/arch/alpha/include/asm/pci.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/pci.h	2020-05-06 17:16:28.000000000 +0200
+++ linux-stable/arch/alpha/include/asm/pci.h	2020-05-06 17:16:28.000000000 +0200
@@ -97,4 +97,6 @@ extern void pci_adjust_legacy_attr(struc
 extern int pci_create_resource_files(struct pci_dev *dev);
 extern void pci_remove_resource_files(struct pci_dev *dev);
 
+extern int alpha_serial_port_needs_delay;
+
 #endif /* __ALPHA_PCI_H */
Index: linux-stable/arch/alpha/kernel/pci.c
===================================================================
--- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-06 17:16:28.000000000 +0200
+++ linux-stable/arch/alpha/kernel/pci.c	2020-05-06 17:16:28.000000000 +0200
@@ -61,9 +61,13 @@ struct pci_controller *pci_isa_hose;
  * Quirks.
  */
 
+int alpha_serial_port_needs_delay = 0;
+EXPORT_SYMBOL(alpha_serial_port_needs_delay);
+
 static void quirk_isa_bridge(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
+	alpha_serial_port_needs_delay = 1;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
 
Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 17:16:28.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 17:16:32.000000000 +0200
@@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
+		ndelay(300);
+
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }

Re: [PATCH 2/2 v2] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Wed, May 06, 2020 at 11:29:29AM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> 
> > > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > > ===================================================================
> > > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
> > > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
> > > @@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
> > >  
> > >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> > >  {
> > > +#ifdef CONFIG_ALPHA
> > > +/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
> > > +	ndelay(300);
> > > +#endif
> > 
> > We really do not like #ifdef in .c files, especially ones that cause a
> > coding style violation :)
> > 
> > Why can't you do this as a quirk for this specific chipset?  You should
> > tie it to the serial port hardware type, not to the CPU type.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Do you want this patch? It enables the delay based on the specific PCI-ISA 
> bridge.
> 
> There is still "#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)" because 
> if we want to reference a variable defined by the PCI subsystem in the 
> arch/alpha tree, we must do it conditionally. If you want to get rid of 
> these #ifs, please describe how.
> 
> Mikulas
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> the Alpha Avanti platform.
> 
> The patch changes timing between accesses to the ISA bus, in particular,
> it reduces the time between "write" access and a subsequent "read" access.
> 
> This causes lock-up when accessing the real time clock and serial ports.
> 
> This patch fixes the serial ports by adding a small delay before the "inb"
> instruction.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  arch/alpha/include/asm/pci.h        |    2 ++
>  arch/alpha/kernel/pci.c             |    4 ++++
>  drivers/tty/serial/8250/8250_core.c |    7 +++++++
>  drivers/tty/serial/8250/8250_port.c |    3 +++
>  include/linux/serial_core.h         |    1 +
>  5 files changed, 17 insertions(+)
> 
> Index: linux-stable/include/linux/serial_core.h
> ===================================================================
> --- linux-stable.orig/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> +++ linux-stable/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> @@ -154,6 +154,7 @@ struct uart_port {
>  
>  	/* quirks must be updated while holding port mutex */
>  #define UPQ_NO_TXEN_TEST	BIT(0)
> +#define UPQ_DELAY_BEFORE_READ	BIT(1)
>  
>  	unsigned int		read_status_mask;	/* driver specific */
>  	unsigned int		ignore_status_mask;	/* driver specific */
> Index: linux-stable/drivers/tty/serial/8250/8250_core.c
> ===================================================================
> --- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> +++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> @@ -37,6 +37,9 @@
>  #ifdef CONFIG_SPARC
>  #include <linux/sunserialcore.h>
>  #endif
> +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> +#include <linux/pci.h>
> +#endif

Don't need the #if for this.

>  
>  #include <asm/irq.h>
>  
> @@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
>  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
>  {
>  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> +	if (alpha_serial_port_needs_delay)
> +		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
> +#endif

Why is a #define needed here?

You can do this same type of change without any #ifdefs in any .c files
and that would be much nicer.

thanks,

greg k-h

Re: [PATCH 2/2 v2] alpha: add a delay before serial port read

[Mikulas Patocka]

On Wed, 6 May 2020, Greg Kroah-Hartman wrote:

> On Wed, May 06, 2020 at 11:29:29AM -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> > 
> > > > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > > > ===================================================================
> > > > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
> > > > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
> > > > @@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
> > > >  
> > > >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> > > >  {
> > > > +#ifdef CONFIG_ALPHA
> > > > +/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
> > > > +	ndelay(300);
> > > > +#endif
> > > 
> > > We really do not like #ifdef in .c files, especially ones that cause a
> > > coding style violation :)
> > > 
> > > Why can't you do this as a quirk for this specific chipset?  You should
> > > tie it to the serial port hardware type, not to the CPU type.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Do you want this patch? It enables the delay based on the specific PCI-ISA 
> > bridge.
> > 
> > There is still "#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)" because 
> > if we want to reference a variable defined by the PCI subsystem in the 
> > arch/alpha tree, we must do it conditionally. If you want to get rid of 
> > these #ifs, please describe how.
> > 
> > Mikulas
> > 
> > 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> > barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> > the Alpha Avanti platform.
> > 
> > The patch changes timing between accesses to the ISA bus, in particular,
> > it reduces the time between "write" access and a subsequent "read" access.
> > 
> > This causes lock-up when accessing the real time clock and serial ports.
> > 
> > This patch fixes the serial ports by adding a small delay before the "inb"
> > instruction.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> > Cc: stable@vger.kernel.org	# v4.17+
> > 
> > ---
> >  arch/alpha/include/asm/pci.h        |    2 ++
> >  arch/alpha/kernel/pci.c             |    4 ++++
> >  drivers/tty/serial/8250/8250_core.c |    7 +++++++
> >  drivers/tty/serial/8250/8250_port.c |    3 +++
> >  include/linux/serial_core.h         |    1 +
> >  5 files changed, 17 insertions(+)
> > 
> > Index: linux-stable/include/linux/serial_core.h
> > ===================================================================
> > --- linux-stable.orig/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> > +++ linux-stable/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> > @@ -154,6 +154,7 @@ struct uart_port {
> >  
> >  	/* quirks must be updated while holding port mutex */
> >  #define UPQ_NO_TXEN_TEST	BIT(0)
> > +#define UPQ_DELAY_BEFORE_READ	BIT(1)
> >  
> >  	unsigned int		read_status_mask;	/* driver specific */
> >  	unsigned int		ignore_status_mask;	/* driver specific */
> > Index: linux-stable/drivers/tty/serial/8250/8250_core.c
> > ===================================================================
> > --- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> > +++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> > @@ -37,6 +37,9 @@
> >  #ifdef CONFIG_SPARC
> >  #include <linux/sunserialcore.h>
> >  #endif
> > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > +#include <linux/pci.h>
> > +#endif
> 
> Don't need the #if for this.

You're right.

> >  #include <asm/irq.h>
> >  
> > @@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
> >  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
> >  {
> >  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > +	if (alpha_serial_port_needs_delay)
> > +		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
> > +#endif
> 
> Why is a #define needed here?
> 
> You can do this same type of change without any #ifdefs in any .c files
> and that would be much nicer.

Because alpha_serial_port_needs_delay is defined only on Alpha - so we 
need to guard accesses to it with #ifdef CONFIG_ALPHA - otherwise the 
kernel wouldn't link on non-Alpha platforms.

Should I make it defined for all architectures?

Mikulas

Re: [PATCH 2/2 v2] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Wed, May 06, 2020 at 11:57:08AM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> 
> > On Wed, May 06, 2020 at 11:29:29AM -0400, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> > > 
> > > > > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > > > > ===================================================================
> > > > > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 08:25:19.000000000 +0200
> > > > > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 09:04:17.000000000 +0200
> > > > > @@ -442,6 +442,10 @@ static unsigned int mem32be_serial_in(st
> > > > >  
> > > > >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> > > > >  {
> > > > > +#ifdef CONFIG_ALPHA
> > > > > +/* we need a small delay, the Alpha Avanti chipset locks up with back-to-back accesses */
> > > > > +	ndelay(300);
> > > > > +#endif
> > > > 
> > > > We really do not like #ifdef in .c files, especially ones that cause a
> > > > coding style violation :)
> > > > 
> > > > Why can't you do this as a quirk for this specific chipset?  You should
> > > > tie it to the serial port hardware type, not to the CPU type.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Do you want this patch? It enables the delay based on the specific PCI-ISA 
> > > bridge.
> > > 
> > > There is still "#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)" because 
> > > if we want to reference a variable defined by the PCI subsystem in the 
> > > arch/alpha tree, we must do it conditionally. If you want to get rid of 
> > > these #ifs, please describe how.
> > > 
> > > Mikulas
> > > 
> > > 
> > > From: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> > > barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> > > the Alpha Avanti platform.
> > > 
> > > The patch changes timing between accesses to the ISA bus, in particular,
> > > it reduces the time between "write" access and a subsequent "read" access.
> > > 
> > > This causes lock-up when accessing the real time clock and serial ports.
> > > 
> > > This patch fixes the serial ports by adding a small delay before the "inb"
> > > instruction.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> > > Cc: stable@vger.kernel.org	# v4.17+
> > > 
> > > ---
> > >  arch/alpha/include/asm/pci.h        |    2 ++
> > >  arch/alpha/kernel/pci.c             |    4 ++++
> > >  drivers/tty/serial/8250/8250_core.c |    7 +++++++
> > >  drivers/tty/serial/8250/8250_port.c |    3 +++
> > >  include/linux/serial_core.h         |    1 +
> > >  5 files changed, 17 insertions(+)
> > > 
> > > Index: linux-stable/include/linux/serial_core.h
> > > ===================================================================
> > > --- linux-stable.orig/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> > > +++ linux-stable/include/linux/serial_core.h	2020-05-06 17:16:28.000000000 +0200
> > > @@ -154,6 +154,7 @@ struct uart_port {
> > >  
> > >  	/* quirks must be updated while holding port mutex */
> > >  #define UPQ_NO_TXEN_TEST	BIT(0)
> > > +#define UPQ_DELAY_BEFORE_READ	BIT(1)
> > >  
> > >  	unsigned int		read_status_mask;	/* driver specific */
> > >  	unsigned int		ignore_status_mask;	/* driver specific */
> > > Index: linux-stable/drivers/tty/serial/8250/8250_core.c
> > > ===================================================================
> > > --- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> > > +++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-06 17:16:28.000000000 +0200
> > > @@ -37,6 +37,9 @@
> > >  #ifdef CONFIG_SPARC
> > >  #include <linux/sunserialcore.h>
> > >  #endif
> > > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > > +#include <linux/pci.h>
> > > +#endif
> > 
> > Don't need the #if for this.
> 
> You're right.
> 
> > >  #include <asm/irq.h>
> > >  
> > > @@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
> > >  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
> > >  {
> > >  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> > > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > > +	if (alpha_serial_port_needs_delay)
> > > +		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
> > > +#endif
> > 
> > Why is a #define needed here?
> > 
> > You can do this same type of change without any #ifdefs in any .c files
> > and that would be much nicer.
> 
> Because alpha_serial_port_needs_delay is defined only on Alpha - so we 
> need to guard accesses to it with #ifdef CONFIG_ALPHA - otherwise the 
> kernel wouldn't link on non-Alpha platforms.
> 
> Should I make it defined for all architectures?

Yes, it's not the first time we have had to do things like this :)

But, there is no other way to detect this based on hardware
signatures/types instead?  That is usually the best way to do it, right?

thanks,

greg k-h

[PATCH 2/2 v3] alpha: add a delay before serial port read

[Mikulas Patocka]

On Wed, 6 May 2020, Greg Kroah-Hartman wrote:

> On Wed, May 06, 2020 at 11:57:08AM -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> > 
> > > On Wed, May 06, 2020 at 11:29:29AM -0400, Mikulas Patocka wrote:
> > > > 
> > > > @@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
> > > >  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
> > > >  {
> > > >  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> > > > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > > > +	if (alpha_serial_port_needs_delay)
> > > > +		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
> > > > +#endif
> > > 
> > > Why is a #define needed here?
> > > 
> > > You can do this same type of change without any #ifdefs in any .c files
> > > and that would be much nicer.
> > 
> > Because alpha_serial_port_needs_delay is defined only on Alpha - so we 
> > need to guard accesses to it with #ifdef CONFIG_ALPHA - otherwise the 
> > kernel wouldn't link on non-Alpha platforms.
> > 
> > Should I make it defined for all architectures?
> 
> Yes, it's not the first time we have had to do things like this :)

I've created this patch that adds a global macro/variable 
serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
serial_port_needs_delay directly in io_serial_in, so that the compiler 
will optimize it out on non-alpha architectures.

> But, there is no other way to detect this based on hardware
> signatures/types instead?  That is usually the best way to do it, right?

It's hard to detect Alpha without using '#ifdef CONFIG_ALPHA' :) The ISA 
serial port hardware is simple, so I think that you can't distinguish it 
just based on its behavior.

> thanks,
> 
> greg k-h
> 


From: Mikulas Patocka <mpatocka@redhat.com>

alpha: add a delay before serial port read

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 arch/alpha/include/asm/pci.h        |    3 +++
 arch/alpha/kernel/pci.c             |    4 ++++
 drivers/tty/serial/8250/8250_port.c |    4 ++++
 include/linux/pci.h                 |    4 ++++
 4 files changed, 15 insertions(+)

Index: linux-stable/arch/alpha/include/asm/pci.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/pci.h	2020-05-06 18:54:24.000000000 +0200
+++ linux-stable/arch/alpha/include/asm/pci.h	2020-05-06 18:54:24.000000000 +0200
@@ -97,4 +97,7 @@ extern void pci_adjust_legacy_attr(struc
 extern int pci_create_resource_files(struct pci_dev *dev);
 extern void pci_remove_resource_files(struct pci_dev *dev);
 
+extern int serial_port_needs_delay;
+#define serial_port_needs_delay	serial_port_needs_delay
+
 #endif /* __ALPHA_PCI_H */
Index: linux-stable/arch/alpha/kernel/pci.c
===================================================================
--- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-06 18:54:24.000000000 +0200
+++ linux-stable/arch/alpha/kernel/pci.c	2020-05-06 18:54:24.000000000 +0200
@@ -61,9 +61,13 @@ struct pci_controller *pci_isa_hose;
  * Quirks.
  */
 
+int serial_port_needs_delay = 0;
+EXPORT_SYMBOL(serial_port_needs_delay);
+
 static void quirk_isa_bridge(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
+	serial_port_needs_delay = 1;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
 
Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
@@ -30,6 +30,7 @@
 #include <linux/uaccess.h>
 #include <linux/pm_runtime.h>
 #include <linux/ktime.h>
+#include <linux/pci.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -442,6 +443,9 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+	if (serial_port_needs_delay)
+		ndelay(300);
+
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }
Index: linux-stable/include/linux/pci.h
===================================================================
--- linux-stable.orig/include/linux/pci.h	2020-05-06 18:54:24.000000000 +0200
+++ linux-stable/include/linux/pci.h	2020-05-06 18:54:24.000000000 +0200
@@ -2384,6 +2384,10 @@ static inline bool pci_is_thunderbolt_at
 	return false;
 }
 
+#ifndef serial_port_needs_delay
+#define serial_port_needs_delay	0
+#endif
+
 #if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
 void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #endif

Re: [PATCH 2/2 v3] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Wed, May 06, 2020 at 01:04:38PM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> 
> > On Wed, May 06, 2020 at 11:57:08AM -0400, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> > > 
> > > > On Wed, May 06, 2020 at 11:29:29AM -0400, Mikulas Patocka wrote:
> > > > > 
> > > > > @@ -490,6 +493,10 @@ static void univ8250_rsa_support(struct
> > > > >  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
> > > > >  {
> > > > >  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> > > > > +#if defined(CONFIG_ALPHA) && defined(CONFIG_PCI)
> > > > > +	if (alpha_serial_port_needs_delay)
> > > > > +		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
> > > > > +#endif
> > > > 
> > > > Why is a #define needed here?
> > > > 
> > > > You can do this same type of change without any #ifdefs in any .c files
> > > > and that would be much nicer.
> > > 
> > > Because alpha_serial_port_needs_delay is defined only on Alpha - so we 
> > > need to guard accesses to it with #ifdef CONFIG_ALPHA - otherwise the 
> > > kernel wouldn't link on non-Alpha platforms.
> > > 
> > > Should I make it defined for all architectures?
> > 
> > Yes, it's not the first time we have had to do things like this :)
> 
> I've created this patch that adds a global macro/variable 
> serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
> serial_port_needs_delay directly in io_serial_in, so that the compiler 
> will optimize it out on non-alpha architectures.

That's not good, what about systems with hundreds of serial ports?

> > But, there is no other way to detect this based on hardware
> > signatures/types instead?  That is usually the best way to do it, right?
> 
> It's hard to detect Alpha without using '#ifdef CONFIG_ALPHA' :) The ISA 
> serial port hardware is simple, so I think that you can't distinguish it 
> just based on its behavior.

The ISA serial port hardware does not have a unique vendor/product id
somewhere?  Some other sort of definition that we can use to determine
exactly what type of system we are running on?



> 
> > thanks,
> > 
> > greg k-h
> > 
> 
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> alpha: add a delay before serial port read
> 
> The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> the Alpha Avanti platform.
> 
> The patch changes timing between accesses to the ISA bus, in particular,
> it reduces the time between "write" access and a subsequent "read" access.
> 
> This causes lock-up when accessing the real time clock and serial ports.
> 
> This patch fixes the serial ports by adding a small delay before the "inb"
> instruction.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  arch/alpha/include/asm/pci.h        |    3 +++
>  arch/alpha/kernel/pci.c             |    4 ++++
>  drivers/tty/serial/8250/8250_port.c |    4 ++++
>  include/linux/pci.h                 |    4 ++++
>  4 files changed, 15 insertions(+)
> 
> Index: linux-stable/arch/alpha/include/asm/pci.h
> ===================================================================
> --- linux-stable.orig/arch/alpha/include/asm/pci.h	2020-05-06 18:54:24.000000000 +0200
> +++ linux-stable/arch/alpha/include/asm/pci.h	2020-05-06 18:54:24.000000000 +0200
> @@ -97,4 +97,7 @@ extern void pci_adjust_legacy_attr(struc
>  extern int pci_create_resource_files(struct pci_dev *dev);
>  extern void pci_remove_resource_files(struct pci_dev *dev);
>  
> +extern int serial_port_needs_delay;
> +#define serial_port_needs_delay	serial_port_needs_delay
> +
>  #endif /* __ALPHA_PCI_H */
> Index: linux-stable/arch/alpha/kernel/pci.c
> ===================================================================
> --- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-06 18:54:24.000000000 +0200
> +++ linux-stable/arch/alpha/kernel/pci.c	2020-05-06 18:54:24.000000000 +0200
> @@ -61,9 +61,13 @@ struct pci_controller *pci_isa_hose;
>   * Quirks.
>   */
>  
> +int serial_port_needs_delay = 0;
> +EXPORT_SYMBOL(serial_port_needs_delay);
> +
>  static void quirk_isa_bridge(struct pci_dev *dev)
>  {
>  	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
> +	serial_port_needs_delay = 1;
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
>  
> Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> ===================================================================
> --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> @@ -30,6 +30,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/ktime.h>
> +#include <linux/pci.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -442,6 +443,9 @@ static unsigned int mem32be_serial_in(st
>  
>  static unsigned int io_serial_in(struct uart_port *p, int offset)
>  {
> +	if (serial_port_needs_delay)
> +		ndelay(300);

Again, this should be a per-port thing, not all ports in the system are
this broken, right?

thanks,

greg k-h

[PATCH 2/2 v4] alpha: add a delay before serial port read

[Mikulas Patocka]

On Wed, 6 May 2020, Greg Kroah-Hartman wrote:

> On Wed, May 06, 2020 at 01:04:38PM -0400, Mikulas Patocka wrote:
> > 
> > I've created this patch that adds a global macro/variable 
> > serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
> > serial_port_needs_delay directly in io_serial_in, so that the compiler 
> > will optimize it out on non-alpha architectures.
> 
> That's not good, what about systems with hundreds of serial ports?

I doubt that someone will conect hundreds of serial ports to such an old 
alpha machine :)

> > > But, there is no other way to detect this based on hardware
> > > signatures/types instead?  That is usually the best way to do it, right?
> > 
> > It's hard to detect Alpha without using '#ifdef CONFIG_ALPHA' :) The ISA 
> > serial port hardware is simple, so I think that you can't distinguish it 
> > just based on its behavior.
> 
> The ISA serial port hardware does not have a unique vendor/product id
> somewhere?  Some other sort of definition that we can use to determine
> exactly what type of system we are running on?

AFAIK it doesn't. You can only distinguish 8250, 16550 and 16550A - but 
not the vendor.

> > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > ===================================================================
> > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> > @@ -30,6 +30,7 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/ktime.h>
> > +#include <linux/pci.h>
> >  
> >  #include <asm/io.h>
> >  #include <asm/irq.h>
> > @@ -442,6 +443,9 @@ static unsigned int mem32be_serial_in(st
> >  
> >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> >  {
> > +	if (serial_port_needs_delay)
> > +		ndelay(300);
> 
> Again, this should be a per-port thing, not all ports in the system are
> this broken, right?
> 
> thanks,
> 
> greg k-h

Here is the patch that uses per-port flag UPQ_DELAY_BEFORE_READ. The flag 
is activated if we have the specific PCI-ISA bridge and if the serial port 
is an ISA port.

Mikulas



From: Mikulas Patocka <mpatocka@redhat.com>

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 arch/alpha/include/asm/pci.h        |    3 +++
 arch/alpha/kernel/pci.c             |    4 ++++
 drivers/tty/serial/8250/8250_core.c |   15 +++++++++------
 drivers/tty/serial/8250/8250_port.c |    3 +++
 include/linux/pci.h                 |    4 ++++
 include/linux/serial_core.h         |    1 +
 6 files changed, 24 insertions(+), 6 deletions(-)

Index: linux-stable/arch/alpha/include/asm/pci.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/pci.h	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/arch/alpha/include/asm/pci.h	2020-05-07 09:54:55.000000000 +0200
@@ -97,4 +97,7 @@ extern void pci_adjust_legacy_attr(struc
 extern int pci_create_resource_files(struct pci_dev *dev);
 extern void pci_remove_resource_files(struct pci_dev *dev);
 
+extern int serial_port_needs_delay;
+#define serial_port_needs_delay	serial_port_needs_delay
+
 #endif /* __ALPHA_PCI_H */
Index: linux-stable/arch/alpha/kernel/pci.c
===================================================================
--- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/arch/alpha/kernel/pci.c	2020-05-07 09:54:55.000000000 +0200
@@ -61,9 +61,13 @@ struct pci_controller *pci_isa_hose;
  * Quirks.
  */
 
+int serial_port_needs_delay = 0;
+EXPORT_SYMBOL(serial_port_needs_delay);
+
 static void quirk_isa_bridge(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
+	serial_port_needs_delay = 1;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
 
Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:54:55.000000000 +0200
@@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
+		ndelay(300);
+
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }
Index: linux-stable/include/linux/pci.h
===================================================================
--- linux-stable.orig/include/linux/pci.h	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/include/linux/pci.h	2020-05-07 09:54:55.000000000 +0200
@@ -2384,6 +2384,10 @@ static inline bool pci_is_thunderbolt_at
 	return false;
 }
 
+#ifndef serial_port_needs_delay
+#define serial_port_needs_delay	0
+#endif
+
 #if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
 void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #endif
Index: linux-stable/drivers/tty/serial/8250/8250_core.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:54:55.000000000 +0200
@@ -34,6 +34,7 @@
 #include <linux/uaccess.h>
 #include <linux/pm_runtime.h>
 #include <linux/io.h>
+#include <linux/pci.h>
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif
@@ -487,9 +488,17 @@ static void univ8250_rsa_support(struct
 #define univ8250_rsa_support(x)		do { } while (0)
 #endif /* CONFIG_SERIAL_8250_RSA */
 
+/*
+ * This "device" covers _all_ ISA 8250-compatible serial devices listed
+ * in the table in include/asm/serial.h
+ */
+static struct platform_device *serial8250_isa_devs;
+
 static inline void serial8250_apply_quirks(struct uart_8250_port *up)
 {
 	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
+	if (serial_port_needs_delay && serial8250_isa_devs && up->port.dev == &serial8250_isa_devs->dev)
+		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
 }
 
 static void __init serial8250_isa_init_ports(void)
@@ -903,12 +912,6 @@ static struct platform_driver serial8250
 };
 
 /*
- * This "device" covers _all_ ISA 8250-compatible serial devices listed
- * in the table in include/asm/serial.h
- */
-static struct platform_device *serial8250_isa_devs;
-
-/*
  * serial8250_register_8250_port and serial8250_unregister_port allows for
  * 16x50 serial ports to be configured at run-time, to support PCMCIA
  * modems and PCI multiport cards.
Index: linux-stable/include/linux/serial_core.h
===================================================================
--- linux-stable.orig/include/linux/serial_core.h	2020-05-07 09:54:55.000000000 +0200
+++ linux-stable/include/linux/serial_core.h	2020-05-07 09:54:55.000000000 +0200
@@ -154,6 +154,7 @@ struct uart_port {
 
 	/* quirks must be updated while holding port mutex */
 #define UPQ_NO_TXEN_TEST	BIT(0)
+#define UPQ_DELAY_BEFORE_READ	BIT(1)
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */

Re: [PATCH 2/2 v4] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Thu, May 07, 2020 at 04:18:49AM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 6 May 2020, Greg Kroah-Hartman wrote:
> 
> > On Wed, May 06, 2020 at 01:04:38PM -0400, Mikulas Patocka wrote:
> > > 
> > > I've created this patch that adds a global macro/variable 
> > > serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
> > > serial_port_needs_delay directly in io_serial_in, so that the compiler 
> > > will optimize it out on non-alpha architectures.
> > 
> > That's not good, what about systems with hundreds of serial ports?
> 
> I doubt that someone will conect hundreds of serial ports to such an old 
> alpha machine :)



> 
> > > > But, there is no other way to detect this based on hardware
> > > > signatures/types instead?  That is usually the best way to do it, right?
> > > 
> > > It's hard to detect Alpha without using '#ifdef CONFIG_ALPHA' :) The ISA 
> > > serial port hardware is simple, so I think that you can't distinguish it 
> > > just based on its behavior.
> > 
> > The ISA serial port hardware does not have a unique vendor/product id
> > somewhere?  Some other sort of definition that we can use to determine
> > exactly what type of system we are running on?
> 
> AFAIK it doesn't. You can only distinguish 8250, 16550 and 16550A - but 
> not the vendor.
> 
> > > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > > ===================================================================
> > > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> > > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-06 18:54:24.000000000 +0200
> > > @@ -30,6 +30,7 @@
> > >  #include <linux/uaccess.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/ktime.h>
> > > +#include <linux/pci.h>
> > >  
> > >  #include <asm/io.h>
> > >  #include <asm/irq.h>
> > > @@ -442,6 +443,9 @@ static unsigned int mem32be_serial_in(st
> > >  
> > >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> > >  {
> > > +	if (serial_port_needs_delay)
> > > +		ndelay(300);
> > 
> > Again, this should be a per-port thing, not all ports in the system are
> > this broken, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Here is the patch that uses per-port flag UPQ_DELAY_BEFORE_READ. The flag 
> is activated if we have the specific PCI-ISA bridge and if the serial port 
> is an ISA port.


Better, care to submit this in a format that it can be applied in?

thanks,

greg k-h

[PATCH 2/2 v4] alpha: add a delay before serial port read

[Mikulas Patocka]

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

We introduce a global variable serial_port_needs_delay - it is defined on
Alpha and zero on all other architectures. We also introduce a new
per-port flag UPQ_DELAY_BEFORE_READ, the flag is set if we have the
specific PCI-ISA bridge (i.e. if serial_port_needs_delay is ture) and if
the serial port is an ISA port.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 arch/alpha/include/asm/pci.h        |    3 +++
 arch/alpha/kernel/pci.c             |    4 ++++
 drivers/tty/serial/8250/8250_core.c |   16 ++++++++++------
 drivers/tty/serial/8250/8250_port.c |    3 +++
 include/linux/pci.h                 |    4 ++++
 include/linux/serial_core.h         |    1 +
 6 files changed, 25 insertions(+), 6 deletions(-)

Index: linux-stable/arch/alpha/include/asm/pci.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/pci.h	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/arch/alpha/include/asm/pci.h	2020-05-07 09:57:02.000000000 +0200
@@ -97,4 +97,7 @@ extern void pci_adjust_legacy_attr(struc
 extern int pci_create_resource_files(struct pci_dev *dev);
 extern void pci_remove_resource_files(struct pci_dev *dev);
 
+extern int serial_port_needs_delay;
+#define serial_port_needs_delay	serial_port_needs_delay
+
 #endif /* __ALPHA_PCI_H */
Index: linux-stable/arch/alpha/kernel/pci.c
===================================================================
--- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/arch/alpha/kernel/pci.c	2020-05-07 09:57:02.000000000 +0200
@@ -61,9 +61,13 @@ struct pci_controller *pci_isa_hose;
  * Quirks.
  */
 
+int serial_port_needs_delay = 0;
+EXPORT_SYMBOL(serial_port_needs_delay);
+
 static void quirk_isa_bridge(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
+	serial_port_needs_delay = 1;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
 
Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
@@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
+		ndelay(300);
+
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }
Index: linux-stable/include/linux/pci.h
===================================================================
--- linux-stable.orig/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
@@ -2384,6 +2384,10 @@ static inline bool pci_is_thunderbolt_at
 	return false;
 }
 
+#ifndef serial_port_needs_delay
+#define serial_port_needs_delay	0
+#endif
+
 #if defined(CONFIG_PCIEPORTBUS) || defined(CONFIG_EEH)
 void pci_uevent_ers(struct pci_dev *pdev, enum  pci_ers_result err_type);
 #endif
Index: linux-stable/drivers/tty/serial/8250/8250_core.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-07 12:37:59.000000000 +0200
@@ -34,6 +34,7 @@
 #include <linux/uaccess.h>
 #include <linux/pm_runtime.h>
 #include <linux/io.h>
+#include <linux/pci.h>
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif
@@ -487,9 +488,18 @@ static void univ8250_rsa_support(struct
 #define univ8250_rsa_support(x)		do { } while (0)
 #endif /* CONFIG_SERIAL_8250_RSA */
 
+/*
+ * This "device" covers _all_ ISA 8250-compatible serial devices listed
+ * in the table in include/asm/serial.h
+ */
+static struct platform_device *serial8250_isa_devs;
+
 static inline void serial8250_apply_quirks(struct uart_8250_port *up)
 {
 	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
+	if (serial_port_needs_delay &&
+	    serial8250_isa_devs && up->port.dev == &serial8250_isa_devs->dev)
+		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
 }
 
 static void __init serial8250_isa_init_ports(void)
@@ -903,12 +913,6 @@ static struct platform_driver serial8250
 };
 
 /*
- * This "device" covers _all_ ISA 8250-compatible serial devices listed
- * in the table in include/asm/serial.h
- */
-static struct platform_device *serial8250_isa_devs;
-
-/*
  * serial8250_register_8250_port and serial8250_unregister_port allows for
  * 16x50 serial ports to be configured at run-time, to support PCMCIA
  * modems and PCI multiport cards.
Index: linux-stable/include/linux/serial_core.h
===================================================================
--- linux-stable.orig/include/linux/serial_core.h	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/include/linux/serial_core.h	2020-05-07 09:57:02.000000000 +0200
@@ -154,6 +154,7 @@ struct uart_port {
 
 	/* quirks must be updated while holding port mutex */
 #define UPQ_NO_TXEN_TEST	BIT(0)
+#define UPQ_DELAY_BEFORE_READ	BIT(1)
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */

Re: [PATCH 2/2 v4] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Thu, May 07, 2020 at 06:53:55AM -0400, Mikulas Patocka wrote:
> Index: linux-stable/include/linux/pci.h
> ===================================================================

Is this coming from git?

> --- linux-stable.orig/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
> +++ linux-stable/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
> @@ -2384,6 +2384,10 @@ static inline bool pci_is_thunderbolt_at
>  	return false;
>  }
>  
> +#ifndef serial_port_needs_delay
> +#define serial_port_needs_delay	0
> +#endif

Anyway, why is this in pci.h?  It has nothing to do with the PCI core.

And the name needs a lot better description, something like:
	alpha_has_b0rken_serial_ports_and_needs_delay
or something to prevent anyone else from ever using this, right?

greg k-h

Re: [PATCH 2/2 v4] alpha: add a delay before serial port read

[Mikulas Patocka]

On Thu, 7 May 2020, Greg Kroah-Hartman wrote:

> On Thu, May 07, 2020 at 06:53:55AM -0400, Mikulas Patocka wrote:
> > Index: linux-stable/include/linux/pci.h
> > ===================================================================
> 
> Is this coming from git?

No - I use quilt.

> > --- linux-stable.orig/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
> > +++ linux-stable/include/linux/pci.h	2020-05-07 09:57:02.000000000 +0200
> > @@ -2384,6 +2384,10 @@ static inline bool pci_is_thunderbolt_at
> >  	return false;
> >  }
> >  
> > +#ifndef serial_port_needs_delay
> > +#define serial_port_needs_delay	0
> > +#endif
> 
> Anyway, why is this in pci.h?  It has nothing to do with the PCI core.
> 
> And the name needs a lot better description, something like:
> 	alpha_has_b0rken_serial_ports_and_needs_delay
> or something to prevent anyone else from ever using this, right?
> 
> greg k-h

OK - I moved it to asm/serial.h

Mikulas

[PATCH 2/2 v5] alpha: add a delay before serial port read

[Mikulas Patocka]

The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
the Alpha Avanti platform.

The patch changes timing between accesses to the ISA bus, in particular,
it reduces the time between "write" access and a subsequent "read" access.

This causes lock-up when accessing the real time clock and serial ports.

This patch fixes the serial ports by adding a small delay before the "inb"
instruction.

We introduce a global variable
alpha_has_broken_serial_ports_and_needs_delay - it is defined on Alpha and
it is set to 1 if we have the specific PCI-ISA bridge where this bug
occurs. We also introduce a new per-port flag UPQ_DELAY_BEFORE_READ, the
flag is set if alpha_has_broken_serial_ports_and_needs_delay is set and if
the serial port is an ISA port.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
Cc: stable@vger.kernel.org	# v4.17+

---
 arch/alpha/include/asm/serial.h     |    5 +++++
 arch/alpha/kernel/pci.c             |    5 +++++
 drivers/tty/serial/8250/8250_core.c |   17 +++++++++++------
 drivers/tty/serial/8250/8250_port.c |    3 +++
 include/linux/serial_core.h         |    1 +
 5 files changed, 25 insertions(+), 6 deletions(-)

Index: linux-stable/arch/alpha/kernel/pci.c
===================================================================
--- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/arch/alpha/kernel/pci.c	2020-05-07 14:30:07.000000000 +0200
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/syscalls.h>
 #include <asm/machvec.h>
+#include <asm/serial.h>
 
 #include "proto.h"
 #include "pci_impl.h"
@@ -61,9 +62,13 @@ struct pci_controller *pci_isa_hose;
  * Quirks.
  */
 
+int alpha_has_broken_serial_ports_and_needs_delay = 0;
+EXPORT_SYMBOL(alpha_has_broken_serial_ports_and_needs_delay);
+
 static void quirk_isa_bridge(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
+	alpha_has_broken_serial_ports_and_needs_delay = 1;
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
 
Index: linux-stable/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
@@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
 {
+	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
+		ndelay(300);
+
 	offset = offset << p->regshift;
 	return inb(p->iobase + offset);
 }
Index: linux-stable/drivers/tty/serial/8250/8250_core.c
===================================================================
--- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-07 14:34:16.000000000 +0200
@@ -487,9 +487,20 @@ static void univ8250_rsa_support(struct
 #define univ8250_rsa_support(x)		do { } while (0)
 #endif /* CONFIG_SERIAL_8250_RSA */
 
+/*
+ * This "device" covers _all_ ISA 8250-compatible serial devices listed
+ * in the table in include/asm/serial.h
+ */
+static struct platform_device *serial8250_isa_devs;
+
 static inline void serial8250_apply_quirks(struct uart_8250_port *up)
 {
 	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
+#ifdef alpha_has_broken_serial_ports_and_needs_delay
+	if (alpha_has_broken_serial_ports_and_needs_delay &&
+	    serial8250_isa_devs && up->port.dev == &serial8250_isa_devs->dev)
+		up->port.quirks |= UPQ_DELAY_BEFORE_READ;
+#endif
 }
 
 static void __init serial8250_isa_init_ports(void)
@@ -903,12 +914,6 @@ static struct platform_driver serial8250
 };
 
 /*
- * This "device" covers _all_ ISA 8250-compatible serial devices listed
- * in the table in include/asm/serial.h
- */
-static struct platform_device *serial8250_isa_devs;
-
-/*
  * serial8250_register_8250_port and serial8250_unregister_port allows for
  * 16x50 serial ports to be configured at run-time, to support PCMCIA
  * modems and PCI multiport cards.
Index: linux-stable/include/linux/serial_core.h
===================================================================
--- linux-stable.orig/include/linux/serial_core.h	2020-05-07 09:57:02.000000000 +0200
+++ linux-stable/include/linux/serial_core.h	2020-05-07 09:57:02.000000000 +0200
@@ -154,6 +154,7 @@ struct uart_port {
 
 	/* quirks must be updated while holding port mutex */
 #define UPQ_NO_TXEN_TEST	BIT(0)
+#define UPQ_DELAY_BEFORE_READ	BIT(1)
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
Index: linux-stable/arch/alpha/include/asm/serial.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/serial.h	2020-02-07 11:52:16.000000000 +0100
+++ linux-stable/arch/alpha/include/asm/serial.h	2020-05-07 14:48:21.000000000 +0200
@@ -28,3 +28,8 @@
 	{ 0, BASE_BAUD, 0x2F8, 3, STD_COM_FLAGS },	/* ttyS1 */	\
 	{ 0, BASE_BAUD, 0x3E8, 4, STD_COM_FLAGS },	/* ttyS2 */	\
 	{ 0, BASE_BAUD, 0x2E8, 3, STD_COM4_FLAGS },	/* ttyS3 */
+
+#ifdef CONFIG_PCI
+extern int alpha_has_broken_serial_ports_and_needs_delay;
+#define alpha_has_broken_serial_ports_and_needs_delay	alpha_has_broken_serial_ports_and_needs_delay
+#endif

Re: [PATCH 2/2 v5] alpha: add a delay before serial port read

[Greg Kroah-Hartman]

On Thu, May 07, 2020 at 08:57:09AM -0400, Mikulas Patocka wrote:
> The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> the Alpha Avanti platform.
> 
> The patch changes timing between accesses to the ISA bus, in particular,
> it reduces the time between "write" access and a subsequent "read" access.
> 
> This causes lock-up when accessing the real time clock and serial ports.
> 
> This patch fixes the serial ports by adding a small delay before the "inb"
> instruction.
> 
> We introduce a global variable
> alpha_has_broken_serial_ports_and_needs_delay - it is defined on Alpha and
> it is set to 1 if we have the specific PCI-ISA bridge where this bug
> occurs. We also introduce a new per-port flag UPQ_DELAY_BEFORE_READ, the
> flag is set if alpha_has_broken_serial_ports_and_needs_delay is set and if
> the serial port is an ISA port.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  arch/alpha/include/asm/serial.h     |    5 +++++
>  arch/alpha/kernel/pci.c             |    5 +++++
>  drivers/tty/serial/8250/8250_core.c |   17 +++++++++++------
>  drivers/tty/serial/8250/8250_port.c |    3 +++
>  include/linux/serial_core.h         |    1 +
>  5 files changed, 25 insertions(+), 6 deletions(-)
> 
> Index: linux-stable/arch/alpha/kernel/pci.c
> ===================================================================
> --- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-07 09:57:02.000000000 +0200
> +++ linux-stable/arch/alpha/kernel/pci.c	2020-05-07 14:30:07.000000000 +0200
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/syscalls.h>
>  #include <asm/machvec.h>
> +#include <asm/serial.h>
>  
>  #include "proto.h"
>  #include "pci_impl.h"
> @@ -61,9 +62,13 @@ struct pci_controller *pci_isa_hose;
>   * Quirks.
>   */
>  
> +int alpha_has_broken_serial_ports_and_needs_delay = 0;
> +EXPORT_SYMBOL(alpha_has_broken_serial_ports_and_needs_delay);
> +
>  static void quirk_isa_bridge(struct pci_dev *dev)
>  {
>  	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
> +	alpha_has_broken_serial_ports_and_needs_delay = 1;
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
>  
> Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> ===================================================================
> --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
> +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
> @@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
>  
>  static unsigned int io_serial_in(struct uart_port *p, int offset)
>  {
> +	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
> +		ndelay(300);
> +
>  	offset = offset << p->regshift;
>  	return inb(p->iobase + offset);
>  }
> Index: linux-stable/drivers/tty/serial/8250/8250_core.c
> ===================================================================
> --- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:57:02.000000000 +0200
> +++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-07 14:34:16.000000000 +0200
> @@ -487,9 +487,20 @@ static void univ8250_rsa_support(struct
>  #define univ8250_rsa_support(x)		do { } while (0)
>  #endif /* CONFIG_SERIAL_8250_RSA */
>  
> +/*
> + * This "device" covers _all_ ISA 8250-compatible serial devices listed
> + * in the table in include/asm/serial.h
> + */
> +static struct platform_device *serial8250_isa_devs;
> +
>  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
>  {
>  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> +#ifdef alpha_has_broken_serial_ports_and_needs_delay

That is a symbol, not a define, how does that work?

greg k-h

Re: [PATCH 2/2 v5] alpha: add a delay before serial port read

[Mikulas Patocka]

On Thu, 7 May 2020, Greg Kroah-Hartman wrote:

> On Thu, May 07, 2020 at 08:57:09AM -0400, Mikulas Patocka wrote:
> > The patch 92d7223a74235054f2aa7227d207d9c57f84dca0 ("alpha: io: reorder
> > barriers to guarantee writeX() and iowriteX() ordering #2") broke boot on
> > the Alpha Avanti platform.
> > 
> > The patch changes timing between accesses to the ISA bus, in particular,
> > it reduces the time between "write" access and a subsequent "read" access.
> > 
> > This causes lock-up when accessing the real time clock and serial ports.
> > 
> > This patch fixes the serial ports by adding a small delay before the "inb"
> > instruction.
> > 
> > We introduce a global variable
> > alpha_has_broken_serial_ports_and_needs_delay - it is defined on Alpha and
> > it is set to 1 if we have the specific PCI-ISA bridge where this bug
> > occurs. We also introduce a new per-port flag UPQ_DELAY_BEFORE_READ, the
> > flag is set if alpha_has_broken_serial_ports_and_needs_delay is set and if
> > the serial port is an ISA port.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> > Cc: stable@vger.kernel.org	# v4.17+
> > 
> > ---
> >  arch/alpha/include/asm/serial.h     |    5 +++++
> >  arch/alpha/kernel/pci.c             |    5 +++++
> >  drivers/tty/serial/8250/8250_core.c |   17 +++++++++++------
> >  drivers/tty/serial/8250/8250_port.c |    3 +++
> >  include/linux/serial_core.h         |    1 +
> >  5 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > Index: linux-stable/arch/alpha/kernel/pci.c
> > ===================================================================
> > --- linux-stable.orig/arch/alpha/kernel/pci.c	2020-05-07 09:57:02.000000000 +0200
> > +++ linux-stable/arch/alpha/kernel/pci.c	2020-05-07 14:30:07.000000000 +0200
> > @@ -24,6 +24,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/syscalls.h>
> >  #include <asm/machvec.h>
> > +#include <asm/serial.h>
> >  
> >  #include "proto.h"
> >  #include "pci_impl.h"
> > @@ -61,9 +62,13 @@ struct pci_controller *pci_isa_hose;
> >   * Quirks.
> >   */
> >  
> > +int alpha_has_broken_serial_ports_and_needs_delay = 0;
> > +EXPORT_SYMBOL(alpha_has_broken_serial_ports_and_needs_delay);
> > +
> >  static void quirk_isa_bridge(struct pci_dev *dev)
> >  {
> >  	dev->class = PCI_CLASS_BRIDGE_ISA << 8;
> > +	alpha_has_broken_serial_ports_and_needs_delay = 1;
> >  }
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82378, quirk_isa_bridge);
> >  
> > Index: linux-stable/drivers/tty/serial/8250/8250_port.c
> > ===================================================================
> > --- linux-stable.orig/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
> > +++ linux-stable/drivers/tty/serial/8250/8250_port.c	2020-05-07 09:57:02.000000000 +0200
> > @@ -442,6 +442,9 @@ static unsigned int mem32be_serial_in(st
> >  
> >  static unsigned int io_serial_in(struct uart_port *p, int offset)
> >  {
> > +	if (unlikely(p->quirks & UPQ_DELAY_BEFORE_READ))
> > +		ndelay(300);
> > +
> >  	offset = offset << p->regshift;
> >  	return inb(p->iobase + offset);
> >  }
> > Index: linux-stable/drivers/tty/serial/8250/8250_core.c
> > ===================================================================
> > --- linux-stable.orig/drivers/tty/serial/8250/8250_core.c	2020-05-07 09:57:02.000000000 +0200
> > +++ linux-stable/drivers/tty/serial/8250/8250_core.c	2020-05-07 14:34:16.000000000 +0200
> > @@ -487,9 +487,20 @@ static void univ8250_rsa_support(struct
> >  #define univ8250_rsa_support(x)		do { } while (0)
> >  #endif /* CONFIG_SERIAL_8250_RSA */
> >  
> > +/*
> > + * This "device" covers _all_ ISA 8250-compatible serial devices listed
> > + * in the table in include/asm/serial.h
> > + */
> > +static struct platform_device *serial8250_isa_devs;
> > +
> >  static inline void serial8250_apply_quirks(struct uart_8250_port *up)
> >  {
> >  	up->port.quirks |= skip_txen_test ? UPQ_NO_TXEN_TEST : 0;
> > +#ifdef alpha_has_broken_serial_ports_and_needs_delay
> 
> That is a symbol, not a define, how does that work?
> 
> greg k-h

That's both the symbol and a define - see this part of the patch:

Mikulas

Index: linux-stable/arch/alpha/include/asm/serial.h
===================================================================
--- linux-stable.orig/arch/alpha/include/asm/serial.h   2020-02-07 11:52:16.000000000 +0100
+++ linux-stable/arch/alpha/include/asm/serial.h        2020-05-07 14:48:21.000000000 +0200
@@ -28,3 +28,8 @@
        { 0, BASE_BAUD, 0x2F8, 3, STD_COM_FLAGS },      /* ttyS1 */     \
        { 0, BASE_BAUD, 0x3E8, 4, STD_COM_FLAGS },      /* ttyS2 */     \
        { 0, BASE_BAUD, 0x2E8, 3, STD_COM4_FLAGS },     /* ttyS3 */
+
+#ifdef CONFIG_PCI
+extern int alpha_has_broken_serial_ports_and_needs_delay;
+#define alpha_has_broken_serial_ports_and_needs_delay  alpha_has_broken_serial_ports_and_needs_delay
+#endif

Re: [PATCH 2/2 v4] alpha: add a delay before serial port read

[Maciej W. Rozycki]

On Thu, 7 May 2020, Mikulas Patocka wrote:

> > > I've created this patch that adds a global macro/variable 
> > > serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
> > > serial_port_needs_delay directly in io_serial_in, so that the compiler 
> > > will optimize it out on non-alpha architectures.
> > 
> > That's not good, what about systems with hundreds of serial ports?
> 
> I doubt that someone will conect hundreds of serial ports to such an old 
> alpha machine :)

 It would be good if PCI serial ports (on add-on cards) were unaffected.

> > > > But, there is no other way to detect this based on hardware
> > > > signatures/types instead?  That is usually the best way to do it, right?
> > > 
> > > It's hard to detect Alpha without using '#ifdef CONFIG_ALPHA' :) The ISA 
> > > serial port hardware is simple, so I think that you can't distinguish it 
> > > just based on its behavior.
> > 
> > The ISA serial port hardware does not have a unique vendor/product id
> > somewhere?  Some other sort of definition that we can use to determine
> > exactly what type of system we are running on?
> 
> AFAIK it doesn't. You can only distinguish 8250, 16550 and 16550A - but 
> not the vendor.

 You might be able to handle it as a platform device.  It's an onboard 
peripheral after all and wired permanently subject to further run-time 
configuration.

 Otherwise it's a generic off-the-shelf pre-LPC-bus PC Super I/O chip.  
Even if we can detect it it'll be there on some x86 machine.  And the 
issue is a problem that may well be anywhere between the CPU, the 
northbridge, the southbridge and the Super I/O, and the weak MMIO ordering 
of the Alpha CPU does not help narrowing it down.

 Let me see...  It's an NS PC87332 piece.  For Avanti technical spec see: 
<https://manx-docs.org/collections/mds-199909/cd1/alpha/pcdsatia.pdf> and 
for the National Semiconductor piece search for "PC87332.pdf" (no direct 
link, but you can download it indirectly):

2.5.8 SuperI/O Identification Register
(SID, Index = 08h)
The SID Register is accessed, like the other configuration
registers, through the Index Register.  This read-only register
is used to identify the PC87332 device.
  7    6    5    4    3    2    1    0
  0    0    0    1    X    X    X    X    Super I/O Identification
                                                      Reg. (SID)
                                                    Index = 08h

I'm not sure how reliable the uniqueness of the four bits in the SID 
register is across various PC Super I/O chips.  I doubt that the chip has 
any observable issues with our serial driver on x86 systems though.

 I'm not sure if the situation is fully understood here, but we have a 
regression and a working solution now is better than a perfect one in the
unspecified future.  We can always improve once we get to the bottom of 
the issue.

 I'm in lockdown away from my Alpha machine, but I can try verifying the 
solution, also with PCI/e serial ports once I am out of lockdown and back 
the right home sometime.

  Maciej

Re: [PATCH 2/2 v4] alpha: add a delay before serial port read

[Mikulas Patocka]

On Sun, 10 May 2020, Maciej W. Rozycki wrote:

> On Thu, 7 May 2020, Mikulas Patocka wrote:
> 
> > > > I've created this patch that adds a global macro/variable 
> > > > serial_port_needs_delay. I've also deleted UPQ_DELAY_BEFORE_READ and test 
> > > > serial_port_needs_delay directly in io_serial_in, so that the compiler 
> > > > will optimize it out on non-alpha architectures.
> > > 
> > > That's not good, what about systems with hundreds of serial ports?
> > 
> > I doubt that someone will conect hundreds of serial ports to such an old 
> > alpha machine :)
> 
> It would be good if PCI serial ports (on add-on cards) were unaffected.

After reading the Alpha specification, I am convinced that the issue is 
not timing, but reordering or merging of accesses to the MMIO space.

So, we need a barrier before a write (mandated by memory-barriers.txt), 
after a read (mandated by memory-barriers.txt) and between write and read 
(mandated by the alpha spec).

The performance of serial ports could be improved if we changed it to use 
read_relaxed and write_relaxed (the serial port never does DMA, so we 
don't have to deal with DMA ordering).

Mikulas