* [PATCH 2/2] alpha: add a delay before serial port read
@ 2020-05-06 11:23 Mikulas Patocka
2020-05-06 11:47 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-06 11:23 UTC (permalink / raw)
To: Richard Henderson, Ivan Kokshaysky, Matt Turner, Greg Kroah-Hartman
Cc: linux-alpha, Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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);
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] alpha: add a delay before serial port read
2020-05-06 11:23 [PATCH 2/2] alpha: add a delay before serial port read Mikulas Patocka
@ 2020-05-06 11:47 ` Greg Kroah-Hartman
2020-05-06 15:29 ` [PATCH 2/2 v2] " Mikulas Patocka
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-06 11:47 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v2] alpha: add a delay before serial port read
2020-05-06 11:47 ` Greg Kroah-Hartman
@ 2020-05-06 15:29 ` Mikulas Patocka
2020-05-06 15:49 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-06 15:29 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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);
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v2] alpha: add a delay before serial port read
2020-05-06 15:29 ` [PATCH 2/2 v2] " Mikulas Patocka
@ 2020-05-06 15:49 ` Greg Kroah-Hartman
2020-05-06 15:57 ` Mikulas Patocka
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-06 15:49 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v2] alpha: add a delay before serial port read
2020-05-06 15:49 ` Greg Kroah-Hartman
@ 2020-05-06 15:57 ` Mikulas Patocka
2020-05-06 16:08 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-06 15:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v2] alpha: add a delay before serial port read
2020-05-06 15:57 ` Mikulas Patocka
@ 2020-05-06 16:08 ` Greg Kroah-Hartman
2020-05-06 17:04 ` [PATCH 2/2 v3] " Mikulas Patocka
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-06 16:08 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v3] alpha: add a delay before serial port read
2020-05-06 16:08 ` Greg Kroah-Hartman
@ 2020-05-06 17:04 ` Mikulas Patocka
2020-05-06 17:45 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-06 17:04 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3] alpha: add a delay before serial port read
2020-05-06 17:04 ` [PATCH 2/2 v3] " Mikulas Patocka
@ 2020-05-06 17:45 ` Greg Kroah-Hartman
2020-05-07 8:18 ` [PATCH 2/2 v4] " Mikulas Patocka
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-06 17:45 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Sinan Kaya, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-06 17:45 ` Greg Kroah-Hartman
@ 2020-05-07 8:18 ` Mikulas Patocka
2020-05-07 8:52 ` Greg Kroah-Hartman
2020-05-10 0:13 ` [PATCH 2/2 v4] " Maciej W. Rozycki
0 siblings, 2 replies; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-07 8:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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 */
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-07 8:18 ` [PATCH 2/2 v4] " Mikulas Patocka
@ 2020-05-07 8:52 ` Greg Kroah-Hartman
2020-05-07 10:53 ` Mikulas Patocka
2020-05-10 0:13 ` [PATCH 2/2 v4] " Maciej W. Rozycki
1 sibling, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07 8:52 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-07 8:52 ` Greg Kroah-Hartman
@ 2020-05-07 10:53 ` Mikulas Patocka
2020-05-07 11:10 ` Greg Kroah-Hartman
0 siblings, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-07 10:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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 */
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-07 10:53 ` Mikulas Patocka
@ 2020-05-07 11:10 ` Greg Kroah-Hartman
2020-05-07 12:53 ` Mikulas Patocka
2020-05-07 12:57 ` [PATCH 2/2 v5] " Mikulas Patocka
0 siblings, 2 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07 11:10 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-07 11:10 ` Greg Kroah-Hartman
@ 2020-05-07 12:53 ` Mikulas Patocka
2020-05-07 12:57 ` [PATCH 2/2 v5] " Mikulas Patocka
1 sibling, 0 replies; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-07 12:53 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2 v5] alpha: add a delay before serial port read
2020-05-07 11:10 ` Greg Kroah-Hartman
2020-05-07 12:53 ` Mikulas Patocka
@ 2020-05-07 12:57 ` Mikulas Patocka
2020-05-07 13:58 ` Greg Kroah-Hartman
1 sibling, 1 reply; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-07 12:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v5] alpha: add a delay before serial port read
2020-05-07 12:57 ` [PATCH 2/2 v5] " Mikulas Patocka
@ 2020-05-07 13:58 ` Greg Kroah-Hartman
2020-05-07 14:03 ` Mikulas Patocka
0 siblings, 1 reply; 18+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-07 13:58 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v5] alpha: add a delay before serial port read
2020-05-07 13:58 ` Greg Kroah-Hartman
@ 2020-05-07 14:03 ` Mikulas Patocka
0 siblings, 0 replies; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-07 14:03 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, linux-alpha,
Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-07 8:18 ` [PATCH 2/2 v4] " Mikulas Patocka
2020-05-07 8:52 ` Greg Kroah-Hartman
@ 2020-05-10 0:13 ` Maciej W. Rozycki
2020-05-23 10:37 ` Mikulas Patocka
1 sibling, 1 reply; 18+ messages in thread
From: Maciej W. Rozycki @ 2020-05-10 0:13 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Greg Kroah-Hartman, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v4] alpha: add a delay before serial port read
2020-05-10 0:13 ` [PATCH 2/2 v4] " Maciej W. Rozycki
@ 2020-05-23 10:37 ` Mikulas Patocka
0 siblings, 0 replies; 18+ messages in thread
From: Mikulas Patocka @ 2020-05-23 10:37 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Greg Kroah-Hartman, Richard Henderson, Ivan Kokshaysky,
Matt Turner, linux-alpha, Arnd Bergmann, linux-serial, linux-rtc
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-05-23 10:37 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 11:23 [PATCH 2/2] alpha: add a delay before serial port read Mikulas Patocka
2020-05-06 11:47 ` Greg Kroah-Hartman
2020-05-06 15:29 ` [PATCH 2/2 v2] " Mikulas Patocka
2020-05-06 15:49 ` Greg Kroah-Hartman
2020-05-06 15:57 ` Mikulas Patocka
2020-05-06 16:08 ` Greg Kroah-Hartman
2020-05-06 17:04 ` [PATCH 2/2 v3] " Mikulas Patocka
2020-05-06 17:45 ` Greg Kroah-Hartman
2020-05-07 8:18 ` [PATCH 2/2 v4] " Mikulas Patocka
2020-05-07 8:52 ` Greg Kroah-Hartman
2020-05-07 10:53 ` Mikulas Patocka
2020-05-07 11:10 ` Greg Kroah-Hartman
2020-05-07 12:53 ` Mikulas Patocka
2020-05-07 12:57 ` [PATCH 2/2 v5] " Mikulas Patocka
2020-05-07 13:58 ` Greg Kroah-Hartman
2020-05-07 14:03 ` Mikulas Patocka
2020-05-10 0:13 ` [PATCH 2/2 v4] " Maciej W. Rozycki
2020-05-23 10:37 ` Mikulas Patocka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).