Messages in this thread | | | Subject | Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X | From | Jiri Slaby <> | Date | Wed, 14 Jul 2021 08:54:40 +0200 |
| |
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
| |