| Message ID | alpine.LRH.2.02.2005070407010.5006@file01.intranet.prod.int.rdu2.redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
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
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
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
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 */