[PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Andy Shevchenko]

The loop can be refactored by using ARRAY_SIZE() instead of NULL terminator.
This reduces code base and makes it easier to read and understand.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 02985cf90ef2..b9138bd08b7f 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -897,37 +897,31 @@ static int pci_netmos_init(struct pci_dev *dev)
 /* enable IO_Space bit */
 #define ITE_887x_POSIO_ENABLE		(1 << 31)
 
+/* inta_addr are the configuration addresses of the ITE */
+static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0, 0x200, 0x280, };
 static int pci_ite887x_init(struct pci_dev *dev)
 {
-	/* inta_addr are the configuration addresses of the ITE */
-	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
-							0x200, 0x280, 0 };
 	int ret, i, type;
 	struct resource *iobase = NULL;
 	u32 miscr, uartbar, ioport;
 
 	/* search for the base-ioport */
-	i = 0;
-	while (inta_addr[i] && iobase == NULL) {
-		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
-								"ite887x");
+	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
+		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");
 		if (iobase != NULL) {
 			/* write POSIO0R - speed | size | ioport */
 			pci_write_config_dword(dev, ITE_887x_POSIO0,
 				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
 				ITE_887x_POSIO_IOSIZE_32 | inta_addr[i]);
 			/* write INTCBAR - ioport */
-			pci_write_config_dword(dev, ITE_887x_INTCBAR,
-								inta_addr[i]);
+			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);
 			ret = inb(inta_addr[i]);
 			if (ret != 0xff) {
 				/* ioport connected */
 				break;
 			}
 			release_region(iobase->start, ITE_887x_IOSIZE);
-			iobase = NULL;
 		}
-		i++;
 	}
 
 	if (!inta_addr[i]) {
-- 
2.30.2

[PATCH v1 2/4] serial: 8250_pci: Get rid of redundant 'else' keyword

[Andy Shevchenko]

The 'else' keyword is not needed when previous conditional branch returns
to the upper layer. Get rid of redundant 'else' keyword in such cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index b9138bd08b7f..937861327aca 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -515,7 +515,7 @@ static int pci_siig_init(struct pci_dev *dev)
 
 	if (type == 0x1000)
 		return pci_siig10x_init(dev);
-	else if (type == 0x2000)
+	if (type == 0x2000)
 		return pci_siig20x_init(dev);
 
 	moan_device("Unknown SIIG card", dev);
@@ -792,9 +792,9 @@ static int pci_netmos_9900_setup(struct serial_private *priv,
 		bar = 3 * idx;
 
 		return setup_port(priv, port, bar, 0, board->reg_shift);
-	} else {
-		return pci_default_setup(priv, board, port, idx);
 	}
+
+	return pci_default_setup(priv, board, port, idx);
 }
 
 /* the 99xx series comes with a range of device IDs and a variety
@@ -1361,9 +1361,10 @@ pericom_do_set_divisor(struct uart_port *port, unsigned int baud,
 			serial_port_out(port, 2, 16 - scr);
 			serial_port_out(port, UART_LCR, lcr);
 			return;
-		} else if (baud > actual_baud) {
-			break;
 		}
+
+		if (baud > actual_baud)
+			break;
 	}
 	serial8250_do_set_divisor(port, baud, quot, quot_frac);
 }
-- 
2.30.2

[PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

There is no need to try MSI/MSI-X only on selected devices.
If MSI is not supported while neing advertised it means device
is broken and we rather introduce a list of such devices which
hopefully will be small or never appear.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 937861327aca..02825c8c5f84 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -58,18 +58,6 @@ struct serial_private {
 
 #define PCI_DEVICE_ID_HPE_PCI_SERIAL	0x37e
 
-static const struct pci_device_id pci_use_msi[] = {
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9900,
-			 0xA000, 0x1000) },
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9912,
-			 0xA000, 0x1000) },
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9922,
-			 0xA000, 0x1000) },
-	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_HP_3PAR, PCI_DEVICE_ID_HPE_PCI_SERIAL,
-			 PCI_ANY_ID, PCI_ANY_ID) },
-	{ }
-};
-
 static int pci_default_setup(struct serial_private*,
 	  const struct pciserial_board*, struct uart_8250_port *, int);
 
@@ -3994,14 +3982,9 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
 	if (board->flags & FL_NOIRQ) {
 		uart.port.irq = 0;
 	} else {
-		if (pci_match_id(pci_use_msi, dev)) {
-			dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
-			pci_set_master(dev);
-			rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
-		} else {
-			dev_dbg(&dev->dev, "Using legacy interrupts\n");
-			rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
-		}
+		pci_set_master(dev);
+
+		rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
 		if (rc < 0) {
 			kfree(priv);
 			priv = ERR_PTR(rc);
@@ -4009,6 +3992,11 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
 		}
 
 		uart.port.irq = pci_irq_vector(dev, 0);
+
+		if (pci_dev_msi_enabled(dev))
+			dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
+		else
+			dev_dbg(&dev->dev, "Using legacy interrupts\n");
 	}
 
 	uart.port.dev = &dev->dev;
-- 
2.30.2

[PATCH v1 4/4] serial: 8250_pci: Replace dev_*() by pci_*() macros

[Andy Shevchenko]

PCI subsystem provides convenient shortcut macros for message printing.
Use those macros instead of dev_*().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_pci.c | 55 +++++++++++++-----------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 02825c8c5f84..0a51e44de4cd 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -63,14 +63,13 @@ static int pci_default_setup(struct serial_private*,
 
 static void moan_device(const char *str, struct pci_dev *dev)
 {
-	dev_err(&dev->dev,
-	       "%s: %s\n"
+	pci_err(dev, "%s\n"
 	       "Please send the output of lspci -vv, this\n"
 	       "message (0x%04x,0x%04x,0x%04x,0x%04x), the\n"
 	       "manufacturer and name of serial board or\n"
 	       "modem board to <linux-serial@vger.kernel.org>.\n",
-	       pci_name(dev), str, dev->vendor, dev->device,
-	       dev->subsystem_vendor, dev->subsystem_device);
+	       str,
+	       dev->vendor, dev->device, dev->subsystem_vendor, dev->subsystem_device);
 }
 
 static int
@@ -226,7 +225,7 @@ static int pci_inteli960ni_init(struct pci_dev *dev)
 	/* is firmware started? */
 	pci_read_config_dword(dev, 0x44, &oldval);
 	if (oldval == 0x00001000L) { /* RESET value */
-		dev_dbg(&dev->dev, "Local i960 firmware missing\n");
+		pci_dbg(dev, "Local i960 firmware missing\n");
 		return -ENODEV;
 	}
 	return 0;
@@ -576,9 +575,8 @@ static int pci_timedia_probe(struct pci_dev *dev)
 	 * (0,2,3,5,6: serial only -- 7,8,9: serial + parallel)
 	 */
 	if ((dev->subsystem_device & 0x00f0) >= 0x70) {
-		dev_info(&dev->dev,
-			"ignoring Timedia subdevice %04x for parport_serial\n",
-			dev->subsystem_device);
+		pci_info(dev, "ignoring Timedia subdevice %04x for parport_serial\n",
+			 dev->subsystem_device);
 		return -ENODEV;
 	}
 
@@ -815,8 +813,7 @@ static int pci_netmos_9900_numports(struct pci_dev *dev)
 		if (sub_serports > 0)
 			return sub_serports;
 
-		dev_err(&dev->dev,
-			"NetMos/Mostech serial driver ignoring port on ambiguous config.\n");
+		pci_err(dev, "NetMos/Mostech serial driver ignoring port on ambiguous config.\n");
 		return 0;
 	}
 
@@ -913,7 +910,7 @@ static int pci_ite887x_init(struct pci_dev *dev)
 	}
 
 	if (!inta_addr[i]) {
-		dev_err(&dev->dev, "ite887x: could not find iobase\n");
+		pci_err(dev, "could not find iobase\n");
 		return -ENODEV;
 	}
 
@@ -1008,9 +1005,7 @@ static int pci_endrun_init(struct pci_dev *dev)
 	/* EndRun device */
 	if (deviceID == 0x07000200) {
 		number_uarts = ioread8(p + 4);
-		dev_dbg(&dev->dev,
-			"%d ports detected on EndRun PCI Express device\n",
-			number_uarts);
+		pci_dbg(dev, "%d ports detected on EndRun PCI Express device\n", number_uarts);
 	}
 	pci_iounmap(dev, p);
 	return number_uarts;
@@ -1040,9 +1035,7 @@ static int pci_oxsemi_tornado_init(struct pci_dev *dev)
 	/* Tornado device */
 	if (deviceID == 0x07000200) {
 		number_uarts = ioread8(p + 4);
-		dev_dbg(&dev->dev,
-			"%d ports detected on Oxford PCI Express device\n",
-			number_uarts);
+		pci_dbg(dev, "%d ports detected on Oxford PCI Express device\n", number_uarts);
 	}
 	pci_iounmap(dev, p);
 	return number_uarts;
@@ -1102,15 +1095,15 @@ static struct quatech_feature quatech_cards[] = {
 	{ 0, }
 };
 
-static int pci_quatech_amcc(u16 devid)
+static int pci_quatech_amcc(struct pci_dev *dev)
 {
 	struct quatech_feature *qf = &quatech_cards[0];
 	while (qf->devid) {
-		if (qf->devid == devid)
+		if (qf->devid == dev->device)
 			return qf->amcc;
 		qf++;
 	}
-	pr_err("quatech: unknown port type '0x%04X'.\n", devid);
+	pci_err(dev, "unknown port type '0x%04X'.\n", dev->device);
 	return 0;
 };
 
@@ -1273,7 +1266,7 @@ static int pci_quatech_rs422(struct uart_8250_port *port)
 
 static int pci_quatech_init(struct pci_dev *dev)
 {
-	if (pci_quatech_amcc(dev->device)) {
+	if (pci_quatech_amcc(dev)) {
 		unsigned long base = pci_resource_start(dev, 0);
 		if (base) {
 			u32 tmp;
@@ -1297,7 +1290,7 @@ static int pci_quatech_setup(struct serial_private *priv,
 	port->port.uartclk = pci_quatech_clock(port);
 	/* For now just warn about RS422 */
 	if (pci_quatech_rs422(port))
-		pr_warn("quatech: software control of RS422 features not currently supported.\n");
+		pci_warn(priv->dev, "software control of RS422 features not currently supported.\n");
 	return pci_default_setup(priv, board, port, idx);
 }
 
@@ -1508,7 +1501,7 @@ static int pci_fintek_setup(struct serial_private *priv,
 	/* Get the io address from configuration space */
 	pci_read_config_word(pdev, config_base + 4, &iobase);
 
-	dev_dbg(&pdev->dev, "%s: idx=%d iobase=0x%x", __func__, idx, iobase);
+	pci_dbg(pdev, "idx=%d iobase=0x%x", idx, iobase);
 
 	port->port.iotype = UPIO_PORT;
 	port->port.iobase = iobase;
@@ -1672,7 +1665,7 @@ static int skip_tx_en_setup(struct serial_private *priv,
 			struct uart_8250_port *port, int idx)
 {
 	port->port.quirks |= UPQ_NO_TXEN_TEST;
-	dev_dbg(&priv->dev->dev,
+	pci_dbg(priv->dev,
 		"serial8250: skipping TxEn test for device [%04x:%04x] subsystem [%04x:%04x]\n",
 		priv->dev->vendor, priv->dev->device,
 		priv->dev->subsystem_vendor, priv->dev->subsystem_device);
@@ -3994,9 +3987,9 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
 		uart.port.irq = pci_irq_vector(dev, 0);
 
 		if (pci_dev_msi_enabled(dev))
-			dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
+			pci_dbg(dev, "Using MSI(-X) interrupts\n");
 		else
-			dev_dbg(&dev->dev, "Using legacy interrupts\n");
+			pci_dbg(dev, "Using legacy interrupts\n");
 	}
 
 	uart.port.dev = &dev->dev;
@@ -4005,12 +3998,12 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
 		if (quirk->setup(priv, board, &uart, i))
 			break;
 
-		dev_dbg(&dev->dev, "Setup PCI port: port %lx, irq %d, type %d\n",
+		pci_dbg(dev, "Setup PCI port: port %lx, irq %d, type %d\n",
 			uart.port.iobase, uart.port.irq, uart.port.iotype);
 
 		priv->line[i] = serial8250_register_8250_port(&uart);
 		if (priv->line[i] < 0) {
-			dev_err(&dev->dev,
+			pci_err(dev,
 				"Couldn't register serial port %lx, irq %d, type %d, error %d\n",
 				uart.port.iobase, uart.port.irq,
 				uart.port.iotype, priv->line[i]);
@@ -4106,8 +4099,7 @@ pciserial_init_one(struct pci_dev *dev, const struct pci_device_id *ent)
 	}
 
 	if (ent->driver_data >= ARRAY_SIZE(pci_boards)) {
-		dev_err(&dev->dev, "invalid driver_data: %ld\n",
-			ent->driver_data);
+		pci_err(dev, "invalid driver_data: %ld\n", ent->driver_data);
 		return -EINVAL;
 	}
 
@@ -4147,8 +4139,7 @@ pciserial_init_one(struct pci_dev *dev, const struct pci_device_id *ent)
 		       sizeof(struct pciserial_board));
 		rc = serial_pci_guess_board(dev, &tmp);
 		if (rc == 0 && serial_pci_matches(board, &tmp))
-			moan_device("Redundant entry in serial pci_table.",
-				    dev);
+			moan_device("Redundant entry in serial pci_table.", dev);
 	}
 
 	priv = pciserial_init_ports(dev, board);
-- 
2.30.2

Re: [PATCH v1 4/4] serial: 8250_pci: Replace dev_*() by pci_*() macros

[Joe Perches]

On Tue, 2021-07-13 at 13:40 +0300, Andy Shevchenko wrote:
> PCI subsystem provides convenient shortcut macros for message printing.
> Use those macros instead of dev_*().
[]
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
[]
> @@ -4147,8 +4139,7 @@ pciserial_init_one(struct pci_dev *dev, const struct pci_device_id *ent)
>  		       sizeof(struct pciserial_board));
>  		rc = serial_pci_guess_board(dev, &tmp);
>  		if (rc == 0 && serial_pci_matches(board, &tmp))
> -			moan_device("Redundant entry in serial pci_table.",
> -				    dev);
> +			moan_device("Redundant entry in serial pci_table.", dev);

Unassociated change.

Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Jiri Slaby]

On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> There is no need to try MSI/MSI-X only on selected devices.
> If MSI is not supported while neing advertised it means device

being

> is broken and we rather introduce a list of such devices which
> hopefully will be small or never appear.

Hmm, have you checked the commit which introduced the whitelist?

     Nevertheless, this needs to handled with care: while many 8250 devices
     actually claim to support MSI(-X) interrupts it should not be 
enabled be
     default. I had at least one device in my hands with broken MSI
     implementation.

     So better introduce a whitelist with devices that are known to support
     MSI(-X) interrupts. I tested all devices mentioned in the patch.


You should have at least CCed the author for an input.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/tty/serial/8250/8250_pci.c | 28 ++++++++--------------------
>   1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 937861327aca..02825c8c5f84 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -58,18 +58,6 @@ struct serial_private {
>   
>   #define PCI_DEVICE_ID_HPE_PCI_SERIAL	0x37e
>   
> -static const struct pci_device_id pci_use_msi[] = {
> -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9900,
> -			 0xA000, 0x1000) },
> -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9912,
> -			 0xA000, 0x1000) },
> -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9922,
> -			 0xA000, 0x1000) },
> -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_HP_3PAR, PCI_DEVICE_ID_HPE_PCI_SERIAL,
> -			 PCI_ANY_ID, PCI_ANY_ID) },
> -	{ }
> -};
> -
>   static int pci_default_setup(struct serial_private*,
>   	  const struct pciserial_board*, struct uart_8250_port *, int);
>   
> @@ -3994,14 +3982,9 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
>   	if (board->flags & FL_NOIRQ) {
>   		uart.port.irq = 0;
>   	} else {
> -		if (pci_match_id(pci_use_msi, dev)) {
> -			dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
> -			pci_set_master(dev);
> -			rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
> -		} else {
> -			dev_dbg(&dev->dev, "Using legacy interrupts\n");
> -			rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
> -		}
> +		pci_set_master(dev);

But bus mastering is not about MSIs. I *think* it's still OK, but you 
need to document that in the commit log too.

Actually, why the commit which added this code turns on bus mastering?

> +
> +		rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
>   		if (rc < 0) {
>   			kfree(priv);
>   			priv = ERR_PTR(rc);
> @@ -4009,6 +3992,11 @@ pciserial_init_ports(struct pci_dev *dev, const struct pciserial_board *board)
>   		}
>   
>   		uart.port.irq = pci_irq_vector(dev, 0);
> +
> +		if (pci_dev_msi_enabled(dev))
> +			dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
> +		else
> +			dev_dbg(&dev->dev, "Using legacy interrupts\n");
>   	}
>   
>   	uart.port.dev = &dev->dev;
> 

thanks,
-- 
js
suse labs

Re: [PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Jiri Slaby]

On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> The loop can be refactored by using ARRAY_SIZE() instead of NULL terminator.
> This reduces code base and makes it easier to read and understand.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/tty/serial/8250/8250_pci.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 02985cf90ef2..b9138bd08b7f 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -897,37 +897,31 @@ static int pci_netmos_init(struct pci_dev *dev)
>   /* enable IO_Space bit */
>   #define ITE_887x_POSIO_ENABLE		(1 << 31)
>   
> +/* inta_addr are the configuration addresses of the ITE */
> +static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0, 0x200, 0x280, };
>   static int pci_ite887x_init(struct pci_dev *dev)
>   {
> -	/* inta_addr are the configuration addresses of the ITE */
> -	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
> -							0x200, 0x280, 0 };
>   	int ret, i, type;
>   	struct resource *iobase = NULL;
>   	u32 miscr, uartbar, ioport;
>   
>   	/* search for the base-ioport */
> -	i = 0;
> -	while (inta_addr[i] && iobase == NULL) {
> -		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
> -								"ite887x");
> +	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
> +		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");

Irrelevant whitespace change.

>   		if (iobase != NULL) {
>   			/* write POSIO0R - speed | size | ioport */
>   			pci_write_config_dword(dev, ITE_887x_POSIO0,
>   				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
>   				ITE_887x_POSIO_IOSIZE_32 | inta_addr[i]);
>   			/* write INTCBAR - ioport */
> -			pci_write_config_dword(dev, ITE_887x_INTCBAR,
> -								inta_addr[i]);
> +			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);

detto

>   			ret = inb(inta_addr[i]);
>   			if (ret != 0xff) {
>   				/* ioport connected */
>   				break;
>   			}
>   			release_region(iobase->start, ITE_887x_IOSIZE);
> -			iobase = NULL;
>   		}
> -		i++;
>   	}
>   
>   	if (!inta_addr[i]) {

OOB access?

regards,
-- 
js
suse labs

Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Jiri Slaby]

On 14. 07. 21, 8:54, Jiri Slaby wrote:
>> @@ -3994,14 +3982,9 @@ pciserial_init_ports(struct pci_dev *dev, const 
>> struct pciserial_board *board)
>>       if (board->flags & FL_NOIRQ) {
>>           uart.port.irq = 0;
>>       } else {
>> -        if (pci_match_id(pci_use_msi, dev)) {
>> -            dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
>> -            pci_set_master(dev);
>> -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
>> -        } else {
>> -            dev_dbg(&dev->dev, "Using legacy interrupts\n");
>> -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
>> -        }
>> +        pci_set_master(dev);
> 
> But bus mastering is not about MSIs. I *think* it's still OK, but you 
> need to document that in the commit log too.
> 
> Actually, why the commit which added this code turns on bus mastering?

Forget about this line, I wasn't woken enough. Of course, MSI (writes) 
to bus need bus mastering.

In any case, I'm still not sure what happens to devices which do not 
support MSI if we enable mastering on them?

-- 
js
suse labs

Re: [PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Dan Carpenter]

Hi Andy,

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/serial-8250_pci-Refactor-the-loop-in-pci_ite887x_init/20210713-184225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: x86_64-randconfig-m001-20210713 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/tty/serial/8250/8250_pci.c:927 pci_ite887x_init() error: buffer overflow 'inta_addr' 7 <= 7 (assuming for loop doesn't break)

vim +927 drivers/tty/serial/8250/8250_pci.c

97f2398f0f6a89 drivers/tty/serial/8250/8250_pci.c Andy Shevchenko    2021-07-13  901  static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0, 0x200, 0x280, };
f79abb828e1d85 drivers/serial/8250_pci.c          Ralf Baechle       2007-08-30  902  static int pci_ite887x_init(struct pci_dev *dev)
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  903  {
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  904  	int ret, i, type;
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  905  	struct resource *iobase = NULL;
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  906  	u32 miscr, uartbar, ioport;
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  907  
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  908  	/* search for the base-ioport */
97f2398f0f6a89 drivers/tty/serial/8250/8250_pci.c Andy Shevchenko    2021-07-13  909  	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
                                                                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^

97f2398f0f6a89 drivers/tty/serial/8250/8250_pci.c Andy Shevchenko    2021-07-13  910  		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  911  		if (iobase != NULL) {
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  912  			/* write POSIO0R - speed | size | ioport */
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  913  			pci_write_config_dword(dev, ITE_887x_POSIO0,
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  914  				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  915  				ITE_887x_POSIO_IOSIZE_32 | inta_addr[i]);
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  916  			/* write INTCBAR - ioport */
97f2398f0f6a89 drivers/tty/serial/8250/8250_pci.c Andy Shevchenko    2021-07-13  917  			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  918  			ret = inb(inta_addr[i]);
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  919  			if (ret != 0xff) {
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  920  				/* ioport connected */
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  921  				break;
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  922  			}
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  923  			release_region(iobase->start, ITE_887x_IOSIZE);
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  924  		}
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  925  	}
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  926  
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22 @927  	if (!inta_addr[i]) {

Should this be changed to if (i == ARRAY_SIZE(inta_addr)) {?

af8c5b8debb046 drivers/tty/serial/8250/8250_pci.c Greg Kroah-Hartman 2013-09-28  928  		dev_err(&dev->dev, "ite887x: could not find iobase\n");
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  929  		return -ENODEV;
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  930  	}
84f8c6fc0e3b6e drivers/serial/8250_pci.c          Niels de Vos       2007-08-22  931  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 9:55 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> > There is no need to try MSI/MSI-X only on selected devices.
> > If MSI is not supported while neing advertised it means device
>
> being
>
> > is broken and we rather introduce a list of such devices which
> > hopefully will be small or never appear.
>
> Hmm, have you checked the commit which introduced the whitelist?

Nope, my bad.

>      Nevertheless, this needs to handled with care: while many 8250 devices
>      actually claim to support MSI(-X) interrupts it should not be
> enabled be
>      default. I had at least one device in my hands with broken MSI
>      implementation.
>
>      So better introduce a whitelist with devices that are known to support
>      MSI(-X) interrupts. I tested all devices mentioned in the patch.

Thanks, but I still think that blacklisting is better. All drivers I
have split (or participated in splitting) from 8250_pci have enabled
MSI for the entire subset they serve for.

> You should have at least CCed the author for an input.

Thanks. I also added Randy, who extended the list.

...

> > +             pci_set_master(dev);
>
> But bus mastering is not about MSIs.

Strictly speaking it's not, but MSI won't work without DMA.

>  I *think* it's still OK, but you
> need to document that in the commit log too.
>
> Actually, why the commit which added this code turns on bus mastering?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 09:58:52AM +0200, Jiri Slaby wrote:
> On 14. 07. 21, 8:54, Jiri Slaby wrote:
> > > @@ -3994,14 +3982,9 @@ pciserial_init_ports(struct pci_dev *dev,
> > > const struct pciserial_board *board)
> > >       if (board->flags & FL_NOIRQ) {
> > >           uart.port.irq = 0;
> > >       } else {
> > > -        if (pci_match_id(pci_use_msi, dev)) {
> > > -            dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
> > > -            pci_set_master(dev);
> > > -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
> > > -        } else {
> > > -            dev_dbg(&dev->dev, "Using legacy interrupts\n");
> > > -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
> > > -        }
> > > +        pci_set_master(dev);
> > 
> > But bus mastering is not about MSIs. I *think* it's still OK, but you
> > need to document that in the commit log too.
> > 
> > Actually, why the commit which added this code turns on bus mastering?
> 
> Forget about this line, I wasn't woken enough. Of course, MSI (writes) to
> bus need bus mastering.

Yes.

> In any case, I'm still not sure what happens to devices which do not support
> MSI if we enable mastering on them?

If they support bus mastering, it means that device may be an arbiter on the
bus and initiate messages over it by its own. I'm not sure any of the existing
UARTs take advantage of bus mastering for anything than MSI delivery.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Joe Perches]

On Tue, 2021-07-13 at 13:40 +0300, Andy Shevchenko wrote:
> The loop can be refactored by using ARRAY_SIZE() instead of NULL terminator.
> This reduces code base and makes it easier to read and understand.
[]
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
[]
> @@ -897,37 +897,31 @@ static int pci_netmos_init(struct pci_dev *dev)
>  /* enable IO_Space bit */
>  #define ITE_887x_POSIO_ENABLE		(1 << 31)
>  
> 
> +/* inta_addr are the configuration addresses of the ITE */
> +static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0, 0x200, 0x280, };

Why move this outside the only function it's used in?
The trailing comma isn't necessary/useful and possibly confusing too.

>  static int pci_ite887x_init(struct pci_dev *dev)
>  {
> -	/* inta_addr are the configuration addresses of the ITE */
> -	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
> -							0x200, 0x280, 0 };
>  	int ret, i, type;
>  	struct resource *iobase = NULL;
>  	u32 miscr, uartbar, ioport;
> 
>  	/* search for the base-ioport */
> -	i = 0;
> -	while (inta_addr[i] && iobase == NULL) {
> -		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
> -								"ite887x");
> +	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
> +		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");
>  		if (iobase != NULL) {

continue and unindent the block below?

>  			/* write POSIO0R - speed | size | ioport */
>  			pci_write_config_dword(dev, ITE_887x_POSIO0,
>  				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
>  				ITE_887x_POSIO_IOSIZE_32 | inta_addr[i]);
>  			/* write INTCBAR - ioport */
> -			pci_write_config_dword(dev, ITE_887x_INTCBAR,
> -								inta_addr[i]);
> +			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);
>  			ret = inb(inta_addr[i]);
>  			if (ret != 0xff) {
>  				/* ioport connected */
>  				break;
>  			}
>  			release_region(iobase->start, ITE_887x_IOSIZE);
> -			iobase = NULL;
>  		}
> -		i++;
>  	}
>  
> 
>  	if (!inta_addr[i]) {

Re: [PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 03:44:31AM -0700, Joe Perches wrote:
> On Tue, 2021-07-13 at 13:40 +0300, Andy Shevchenko wrote:
> > The loop can be refactored by using ARRAY_SIZE() instead of NULL terminator.
> > This reduces code base and makes it easier to read and understand.

Thanks for review! My answers below.

> > +/* inta_addr are the configuration addresses of the ITE */
> > +static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0, 0x200, 0x280, };
> 
> Why move this outside the only function it's used in?

Because it's a static one. I prefer to see global variables easily when reading
the code.

> The trailing comma isn't necessary/useful and possibly confusing too.

True, since it's one line.

> >  static int pci_ite887x_init(struct pci_dev *dev)
> >  {
> > -	/* inta_addr are the configuration addresses of the ITE */
> > -	static const short inta_addr[] = { 0x2a0, 0x2c0, 0x220, 0x240, 0x1e0,
> > -							0x200, 0x280, 0 };
> >  	int ret, i, type;
> >  	struct resource *iobase = NULL;
> >  	u32 miscr, uartbar, ioport;
> > 
> >  	/* search for the base-ioport */
> > -	i = 0;
> > -	while (inta_addr[i] && iobase == NULL) {
> > -		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE,
> > -								"ite887x");
> > +	for (i = 0; i < ARRAY_SIZE(inta_addr); i++) {
> > +		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");
> >  		if (iobase != NULL) {
> 
> continue and unindent the block below?

As a separate patch perhaps?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init()

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 08:57:06AM +0200, Jiri Slaby wrote:
> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> > The loop can be refactored by using ARRAY_SIZE() instead of NULL terminator.
> > This reduces code base and makes it easier to read and understand.

> > +		iobase = request_region(inta_addr[i], ITE_887x_IOSIZE, "ite887x");
> 
> Irrelevant whitespace change.
> 
> >   		if (iobase != NULL) {
> >   			/* write POSIO0R - speed | size | ioport */
> >   			pci_write_config_dword(dev, ITE_887x_POSIO0,
> >   				ITE_887x_POSIO_ENABLE | ITE_887x_POSIO_SPEED |
> >   				ITE_887x_POSIO_IOSIZE_32 | inta_addr[i]);
> >   			/* write INTCBAR - ioport */
> > -			pci_write_config_dword(dev, ITE_887x_INTCBAR,
> > -								inta_addr[i]);
> > +			pci_write_config_dword(dev, ITE_887x_INTCBAR, inta_addr[i]);
> 
> detto
> 
> >   			ret = inb(inta_addr[i]);
> >   			if (ret != 0xff) {
> >   				/* ioport connected */
> >   				break;
> >   			}
> >   			release_region(iobase->start, ITE_887x_IOSIZE);
> > -			iobase = NULL;
> >   		}
> > -		i++;

I believe with Joe's suggestion I can improve entire body of this branch
perhaps in a separate patch. Do you prefer one or two patches?

> >   	if (!inta_addr[i]) {
> 
> OOB access?

Yep, Dan reported the same. Missed during conversion. Will fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Ralf Ramsauer]

On 14/07/2021 08:54, Jiri Slaby wrote:
> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
>> There is no need to try MSI/MSI-X only on selected devices.
>> If MSI is not supported while neing advertised it means device
> 
> being
> 
>> is broken and we rather introduce a list of such devices which
>> hopefully will be small or never appear.
> 
> Hmm, have you checked the commit which introduced the whitelist?
> 
>     Nevertheless, this needs to handled with care: while many 8250 devices
>     actually claim to support MSI(-X) interrupts it should not be
> enabled be
>     default. I had at least one device in my hands with broken MSI
>     implementation.
> 
>     So better introduce a whitelist with devices that are known to support
>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
> 
> 
> You should have at least CCed the author for an input.

Yep, back then I was testing three different 8250 pci cards. All of them
claimed to support MSI, while one really worked with MSI, the one that I
whitelisted. So I thought it would be better to use legacy IRQs as long
as no one tested a specific card to work with MSI.

> 
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/tty/serial/8250/8250_pci.c | 28 ++++++++--------------------
>>   1 file changed, 8 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_pci.c
>> b/drivers/tty/serial/8250/8250_pci.c
>> index 937861327aca..02825c8c5f84 100644
>> --- a/drivers/tty/serial/8250/8250_pci.c
>> +++ b/drivers/tty/serial/8250/8250_pci.c
>> @@ -58,18 +58,6 @@ struct serial_private {
>>     #define PCI_DEVICE_ID_HPE_PCI_SERIAL    0x37e
>>   -static const struct pci_device_id pci_use_msi[] = {
>> -    { PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9900,
>> -             0xA000, 0x1000) },
>> -    { PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9912,
>> -             0xA000, 0x1000) },
>> -    { PCI_DEVICE_SUB(PCI_VENDOR_ID_NETMOS, PCI_DEVICE_ID_NETMOS_9922,
>> -             0xA000, 0x1000) },
>> -    { PCI_DEVICE_SUB(PCI_VENDOR_ID_HP_3PAR,
>> PCI_DEVICE_ID_HPE_PCI_SERIAL,
>> -             PCI_ANY_ID, PCI_ANY_ID) },
>> -    { }
>> -};
>> -

Don't do that… And don't convert it to a blacklist. A blacklist will
break users until they report that something doesn't work.

  Ralf

>>   static int pci_default_setup(struct serial_private*,
>>         const struct pciserial_board*, struct uart_8250_port *, int);
>>   @@ -3994,14 +3982,9 @@ pciserial_init_ports(struct pci_dev *dev,
>> const struct pciserial_board *board)
>>       if (board->flags & FL_NOIRQ) {
>>           uart.port.irq = 0;
>>       } else {
>> -        if (pci_match_id(pci_use_msi, dev)) {
>> -            dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
>> -            pci_set_master(dev);
>> -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
>> -        } else {
>> -            dev_dbg(&dev->dev, "Using legacy interrupts\n");
>> -            rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY);
>> -        }
>> +        pci_set_master(dev);
> 
> But bus mastering is not about MSIs. I *think* it's still OK, but you
> need to document that in the commit log too.
> 
> Actually, why the commit which added this code turns on bus mastering?
> 
>> +
>> +        rc = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
>>           if (rc < 0) {
>>               kfree(priv);
>>               priv = ERR_PTR(rc);
>> @@ -4009,6 +3992,11 @@ pciserial_init_ports(struct pci_dev *dev, const
>> struct pciserial_board *board)
>>           }
>>             uart.port.irq = pci_irq_vector(dev, 0);
>> +
>> +        if (pci_dev_msi_enabled(dev))
>> +            dev_dbg(&dev->dev, "Using MSI(-X) interrupts\n");
>> +        else
>> +            dev_dbg(&dev->dev, "Using legacy interrupts\n");
>>       }
>>         uart.port.dev = &dev->dev;
>>
> 
> thanks,

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
<ralf.ramsauer@oth-regensburg.de> wrote:
> On 14/07/2021 08:54, Jiri Slaby wrote:
> > On 13. 07. 21, 12:40, Andy Shevchenko wrote:

> > Hmm, have you checked the commit which introduced the whitelist?
> >
> >     Nevertheless, this needs to handled with care: while many 8250 devices
> >     actually claim to support MSI(-X) interrupts it should not be
> > enabled be
> >     default. I had at least one device in my hands with broken MSI
> >     implementation.
> >
> >     So better introduce a whitelist with devices that are known to support
> >     MSI(-X) interrupts. I tested all devices mentioned in the patch.
> >
> >
> > You should have at least CCed the author for an input.
>
> Yep, back then I was testing three different 8250 pci cards. All of them
> claimed to support MSI, while one really worked with MSI, the one that I
> whitelisted. So I thought it would be better to use legacy IRQs as long
> as no one tested a specific card to work with MSI.

Can you shed a light eventually what those cards are?

> Don't do that… And don't convert it to a blacklist. A blacklist will
> break users until they report that something doesn't work.

White list is not okay either. MSI in general is a right thing to do.
preventing users from MSI is asking for the performance degradation
and IRQ resource conflicts (in case the IRQ line is shared).

Besides that, shouldn't it be rather the specific field in private (to
8250_pci) structure than constantly growing list?

-- 
With Best Regards,
Andy Shevchenko

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Ralf Ramsauer]

On 14/07/2021 15:35, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> <ralf.ramsauer@oth-regensburg.de> wrote:
>> On 14/07/2021 08:54, Jiri Slaby wrote:
>>> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> 
>>> Hmm, have you checked the commit which introduced the whitelist?
>>>
>>>     Nevertheless, this needs to handled with care: while many 8250 devices
>>>     actually claim to support MSI(-X) interrupts it should not be
>>> enabled be
>>>     default. I had at least one device in my hands with broken MSI
>>>     implementation.
>>>
>>>     So better introduce a whitelist with devices that are known to support
>>>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
>>>
>>>
>>> You should have at least CCed the author for an input.
>>
>> Yep, back then I was testing three different 8250 pci cards. All of them
>> claimed to support MSI, while one really worked with MSI, the one that I
>> whitelisted. So I thought it would be better to use legacy IRQs as long
>> as no one tested a specific card to work with MSI.
> 
> Can you shed a light eventually what those cards are?

That's been a while. Let me check that if I can still find them, and
I'll test them once again against MSI being enabled. But this can take
some days.

  Ralf

> 
>> Don't do that… And don't convert it to a blacklist. A blacklist will
>> break users until they report that something doesn't work.
> 
> White list is not okay either. MSI in general is a right thing to do.
> preventing users from MSI is asking for the performance degradation
> and IRQ resource conflicts (in case the IRQ line is shared).
> 
> Besides that, shouldn't it be rather the specific field in private (to
> 8250_pci) structure than constantly growing list?

Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[rwright]

On Wed, Jul 14, 2021 at 12:15:25PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 9:55 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> ...
> Thanks, but I still think that blacklisting is better. All drivers I
> have split (or participated in splitting) from 8250_pci have enabled
> MSI for the entire subset they serve for.
> ...
> Thanks. I also added Randy, who extended the list.

My own opinion is that a whitelist to enroll devices as they are tested
is the safer approach, for the reason that getting test coverage on many
of the older devices would be difficult.  For example, I see id's of HP
devices in the code that are probably 20 years old, and I doubt whether
there are operational examples inside HPE today.

That said, I can offer to test that a new patch to 8250_pci.c works on
the device I recently added.  Please cc me directly if that is helpful,
as I don't always read the mailing lists such as linux-serial promptly.

-- 
Randy Wright - Hewlett Packard Enterprise - rwright@hpe.com

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Ralf Ramsauer]

On 14/07/2021 15:35, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> <ralf.ramsauer@oth-regensburg.de> wrote:
>> On 14/07/2021 08:54, Jiri Slaby wrote:
>>> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> 
>>> Hmm, have you checked the commit which introduced the whitelist?
>>>
>>>     Nevertheless, this needs to handled with care: while many 8250 devices
>>>     actually claim to support MSI(-X) interrupts it should not be
>>> enabled be
>>>     default. I had at least one device in my hands with broken MSI
>>>     implementation.
>>>
>>>     So better introduce a whitelist with devices that are known to support
>>>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
>>>
>>>
>>> You should have at least CCed the author for an input.
>>
>> Yep, back then I was testing three different 8250 pci cards. All of them
>> claimed to support MSI, while one really worked with MSI, the one that I
>> whitelisted. So I thought it would be better to use legacy IRQs as long
>> as no one tested a specific card to work with MSI.
> 
> Can you shed a light eventually what those cards are?

So I found a no-name el-cheapo card that has some issues with MSI:

18:00.0 Serial controller: Device 1c00:3253 (rev 10) (prog-if 05 [16850])

The card comes with two serial lines. It comes perfectly up, if I enable
it to use MSI in the whitelist:

serial 0000:18:00.0: Using MSI(-X) interrupts
serial 0000:18:00.0: Setup PCI port: port 40c0, irq 104, type 0
0000:18:00.0: ttyS6 at I/O 0x40c0 (irq = 104, base_baud = 115200) is a
XR16850
serial 0000:18:00.0: Setup PCI port: port 40c8, irq 104, type 0
0000:18:00.0: ttyS7 at I/O 0x40c8 (irq = 104, base_baud = 115200) is a
XR16850

After loading 8250_pci, lspci -vvs 18:0.0 tells:

	Capabilities: [68] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000fee000b8  Data: 0000
		Masking: ffffffff  Pending: 00000000

Looks good so far. Now let's echo to the device.

$ echo asdf > /dev/ttyS6

-- stuck. The echoing process stucks at close():

write(1, "asdf\n", 5)                   = 5
close(1

Stuck in the sense of: the echo is still killable, no crashes. The same
happens if I try to access the device with stty. So something is odd
here. However, the Netmos cards that I whitelisted do a great job.

So I can't tell if I was just unlucky to grab a card that has issues
with MSI, and this is an exception rather than the rule…

HTH,
  Ralf


> 
>> Don't do that… And don't convert it to a blacklist. A blacklist will
>> break users until they report that something doesn't work.
> 
> White list is not okay either. MSI in general is a right thing to do.
> preventing users from MSI is asking for the performance degradation
> and IRQ resource conflicts (in case the IRQ line is shared).
> 
> Besides that, shouldn't it be rather the specific field in private (to
> 8250_pci) structure than constantly growing list?
> 

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

On Fri, Jul 16, 2021 at 4:07 PM Ralf Ramsauer
<ralf.ramsauer@oth-regensburg.de> wrote:
> On 14/07/2021 15:35, Andy Shevchenko wrote:
> > On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> > <ralf.ramsauer@oth-regensburg.de> wrote:
> >> On 14/07/2021 08:54, Jiri Slaby wrote:
> >>> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> >
> >>> Hmm, have you checked the commit which introduced the whitelist?
> >>>
> >>>     Nevertheless, this needs to handled with care: while many 8250 devices
> >>>     actually claim to support MSI(-X) interrupts it should not be
> >>> enabled be
> >>>     default. I had at least one device in my hands with broken MSI
> >>>     implementation.
> >>>
> >>>     So better introduce a whitelist with devices that are known to support
> >>>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
> >>>
> >>>
> >>> You should have at least CCed the author for an input.
> >>
> >> Yep, back then I was testing three different 8250 pci cards. All of them
> >> claimed to support MSI, while one really worked with MSI, the one that I
> >> whitelisted. So I thought it would be better to use legacy IRQs as long
> >> as no one tested a specific card to work with MSI.
> >
> > Can you shed a light eventually what those cards are?

> So I found a no-name el-cheapo card that has some issues with MSI:

Win Chip Head (WCH)

> 18:00.0 Serial controller: Device 1c00:3253 (rev 10) (prog-if 05 [16850])
>
> The card comes with two serial lines. It comes perfectly up, if I enable
> it to use MSI in the whitelist:
>
> serial 0000:18:00.0: Using MSI(-X) interrupts
> serial 0000:18:00.0: Setup PCI port: port 40c0, irq 104, type 0
> 0000:18:00.0: ttyS6 at I/O 0x40c0 (irq = 104, base_baud = 115200) is a
> XR16850
> serial 0000:18:00.0: Setup PCI port: port 40c8, irq 104, type 0
> 0000:18:00.0: ttyS7 at I/O 0x40c8 (irq = 104, base_baud = 115200) is a
> XR16850
>
> After loading 8250_pci, lspci -vvs 18:0.0 tells:
>
>         Capabilities: [68] MSI: Enable+ Count=1/32 Maskable+ 64bit+
>                 Address: 00000000fee000b8  Data: 0000
>                 Masking: ffffffff  Pending: 00000000
>
> Looks good so far. Now let's echo to the device.
>
> $ echo asdf > /dev/ttyS6
>
> -- stuck. The echoing process stucks at close():
>
> write(1, "asdf\n", 5)                   = 5
> close(1
>
> Stuck in the sense of: the echo is still killable, no crashes. The same
> happens if I try to access the device with stty. So something is odd
> here. However, the Netmos cards that I whitelisted do a great job.

Can you share somehow the `lspci -vvv -xx -nk; lspci -t` with and
without MSI enabled? (I want to understand what is going on with it)

> So I can't tell if I was just unlucky to grab a card that has issues
> with MSI, and this is an exception rather than the rule…

-- 
With Best Regards,
Andy Shevchenko

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Andy Shevchenko]

On Fri, Jul 16, 2021 at 05:27:36PM +0200, Ralf Ramsauer wrote:
> On 16/07/2021 17:01, Andy Shevchenko wrote:
> > On Fri, Jul 16, 2021 at 4:07 PM Ralf Ramsauer
> > <ralf.ramsauer@oth-regensburg.de> wrote:
> >> On 14/07/2021 15:35, Andy Shevchenko wrote:
> >>> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> >>> <ralf.ramsauer@oth-regensburg.de> wrote:
> >>>> On 14/07/2021 08:54, Jiri Slaby wrote:
> >>>>> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
> >>>
> >>>>> Hmm, have you checked the commit which introduced the whitelist?
> >>>>>
> >>>>>     Nevertheless, this needs to handled with care: while many 8250 devices
> >>>>>     actually claim to support MSI(-X) interrupts it should not be
> >>>>> enabled be
> >>>>>     default. I had at least one device in my hands with broken MSI
> >>>>>     implementation.
> >>>>>
> >>>>>     So better introduce a whitelist with devices that are known to support
> >>>>>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
> >>>>>
> >>>>>
> >>>>> You should have at least CCed the author for an input.
> >>>>
> >>>> Yep, back then I was testing three different 8250 pci cards. All of them
> >>>> claimed to support MSI, while one really worked with MSI, the one that I
> >>>> whitelisted. So I thought it would be better to use legacy IRQs as long
> >>>> as no one tested a specific card to work with MSI.
> >>>
> >>> Can you shed a light eventually what those cards are?
> > 
> >> So I found a no-name el-cheapo card that has some issues with MSI:
> > 
> > Win Chip Head (WCH)
> > 
> >> 18:00.0 Serial controller: Device 1c00:3253 (rev 10) (prog-if 05 [16850])

Thank you!

One more thing, ist it possible to see entire PCI configuration space (w/ or
w/o MSI, I don't think it matters)? Something like

	`lspci -nk -vvv -xxx -s 18:0`

to run.

(I believe there are a lot of 0xff bytes)

-- 
With Best Regards,
Andy Shevchenko

Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X

[Ralf Ramsauer]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]



On 16/07/2021 19:27, Andy Shevchenko wrote:
> On Fri, Jul 16, 2021 at 05:27:36PM +0200, Ralf Ramsauer wrote:
>> On 16/07/2021 17:01, Andy Shevchenko wrote:
>>> On Fri, Jul 16, 2021 at 4:07 PM Ralf Ramsauer
>>> <ralf.ramsauer@oth-regensburg.de> wrote:
>>>> On 14/07/2021 15:35, Andy Shevchenko wrote:
>>>>> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
>>>>> <ralf.ramsauer@oth-regensburg.de> wrote:
>>>>>> On 14/07/2021 08:54, Jiri Slaby wrote:
>>>>>>> On 13. 07. 21, 12:40, Andy Shevchenko wrote:
>>>>>
>>>>>>> Hmm, have you checked the commit which introduced the whitelist?
>>>>>>>
>>>>>>>     Nevertheless, this needs to handled with care: while many 8250 devices
>>>>>>>     actually claim to support MSI(-X) interrupts it should not be
>>>>>>> enabled be
>>>>>>>     default. I had at least one device in my hands with broken MSI
>>>>>>>     implementation.
>>>>>>>
>>>>>>>     So better introduce a whitelist with devices that are known to support
>>>>>>>     MSI(-X) interrupts. I tested all devices mentioned in the patch.
>>>>>>>
>>>>>>>
>>>>>>> You should have at least CCed the author for an input.
>>>>>>
>>>>>> Yep, back then I was testing three different 8250 pci cards. All of them
>>>>>> claimed to support MSI, while one really worked with MSI, the one that I
>>>>>> whitelisted. So I thought it would be better to use legacy IRQs as long
>>>>>> as no one tested a specific card to work with MSI.
>>>>>
>>>>> Can you shed a light eventually what those cards are?
>>>
>>>> So I found a no-name el-cheapo card that has some issues with MSI:
>>>
>>> Win Chip Head (WCH)
>>>
>>>> 18:00.0 Serial controller: Device 1c00:3253 (rev 10) (prog-if 05 [16850])
> 
> Thank you!
> 
> One more thing, ist it possible to see entire PCI configuration space (w/ or
> w/o MSI, I don't think it matters)? Something like
> 
> 	`lspci -nk -vvv -xxx -s 18:0`
> 
> to run.
> 
> (I believe there are a lot of 0xff bytes)

Find it attached, w/ MSI+. Not that many, only the 0xffs for the MSI
mask, afaict.

  Ralf

[-- Attachment #2: 18.0.txt --]
[-- Type: text/plain, Size: 3929 bytes --]

18:00.0 0700: 1c00:3253 (rev 10) (prog-if 05 [16850])
	Subsystem: 1c00:3253
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 104
	NUMA node: 0
	Region 0: I/O ports at 4000 [size=256]
	Region 1: Memory at ab000000 (32-bit, prefetchable) [size=32K]
	Region 2: I/O ports at 4100 [size=4]
	Expansion ROM at ab200000 [disabled] [size=32K]
	Capabilities: [60] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [68] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000fee000b8  Data: 0000
		Masking: ffffffff  Pending: 00000000
	Capabilities: [80] Express (v2) Legacy Endpoint, MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <2us, L1 <32us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
		DevCtl:	CorrErr- NonFatalErr+ FatalErr+ UnsupReq+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 256 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s unlimited, L1 unlimited
			ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp-
		LnkCtl:	ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s (ok), Width x1 (ok)
			TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		DevCap2: Completion Timeout: Not Supported, TimeoutDis+ NROPrPrP- LTR-
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS-
			 AtomicOpsCap: 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR- OBFF Disabled,
			 AtomicOpsCtl: ReqEn-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete- EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt+ RxOF- MalfTLP- ECRC- UnsupReq- ACSViol+
		UESvrt:	DLP+ SDES- TLP+ FCP+ CmpltTO+ CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC+ UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr+ BadTLP+ BadDLLP+ Rollover+ Timeout+ AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn+ ECRCChkCap+ ECRCChkEn+
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Kernel driver in use: serial
	Kernel modules: 8250_pci
00: 00 1c 53 32 07 04 10 00 10 05 00 07 00 00 00 00
10: 01 40 00 00 08 00 00 ab 01 41 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 00 1c 53 32
30: 00 80 ff ff 60 00 00 00 00 00 00 00 ff 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
60: 01 68 c3 c9 00 00 00 00 05 80 8b 01 b8 00 e0 fe
70: 00 00 00 00 00 00 00 00 ff ff ff ff 00 00 00 00
80: 10 00 12 00 41 8b 64 00 3e 28 10 00 11 fc 07 00
90: 40 00 11 10 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00