| Message ID | 20210713104026.58560-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] serial: 8250_pci: Refactor the loop in pci_ite887x_init() | expand |
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
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
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 <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
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
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 <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,
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
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?
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
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? >
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
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
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 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
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]) {
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(-)