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 <[email protected]>
---
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
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 <[email protected]>
---
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
PCI subsystem provides convenient shortcut macros for message printing.
Use those macros instead of dev_*().
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 <[email protected]>.\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
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 <[email protected]>
---
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
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.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
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/[email protected]
On Wed, Jul 14, 2021 at 9:55 AM Jiri Slaby <[email protected]> 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
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
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]) {
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
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
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 <[email protected]>
>> ---
>> 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,
On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
<[email protected]> 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
On 14/07/2021 15:35, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> <[email protected]> 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?
On Wed, Jul 14, 2021 at 12:15:25PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 9:55 AM Jiri Slaby <[email protected]> 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 - [email protected]
On 14/07/2021 15:35, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> <[email protected]> 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?
>
On Fri, Jul 16, 2021 at 4:07 PM Ralf Ramsauer
<[email protected]> wrote:
> On 14/07/2021 15:35, Andy Shevchenko wrote:
> > On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> > <[email protected]> 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
On 16/07/2021 17:01, Andy Shevchenko wrote:
> On Fri, Jul 16, 2021 at 4:07 PM Ralf Ramsauer
> <[email protected]> wrote:
>> On 14/07/2021 15:35, Andy Shevchenko wrote:
>>> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
>>> <[email protected]> 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)
Sure, please find it attached.
Ralf
>
>> 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…
>
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
> > <[email protected]> wrote:
> >> On 14/07/2021 15:35, Andy Shevchenko wrote:
> >>> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
> >>> <[email protected]> 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
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
>>> <[email protected]> wrote:
>>>> On 14/07/2021 15:35, Andy Shevchenko wrote:
>>>>> On Wed, Jul 14, 2021 at 3:56 PM Ralf Ramsauer
>>>>> <[email protected]> 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