* [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).