linux-kernel - Re: [EXT] Re: [PATCH v1 3/4] serial: 8250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <784629f9-677e-ee53-aceb-89397ce0951a@oth-regensburg.de>
Date:   Wed, 14 Jul 2021 14:49:51 +0200
From:   Ralf Ramsauer <ralf.ramsauer@...-regensburg.de>
To:     Jiri Slaby <jirislaby@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        <linux-serial@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [EXT] Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X



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@...ux.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,