Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v1 3/4] serial: 8250_pci: Always try MSI/MSI-X
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de>
References: <20210713104026.58560-1-andriy.shevchenko@linux.intel.com>
 <20210713104026.58560-3-andriy.shevchenko@linux.intel.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Message-ID: <9af24b96-8119-7ccf-f0d0-d725af80aa0b@kernel.org>
Date:   Wed, 14 Jul 2021 08:54:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210713104026.58560-3-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset=iso-8859-2; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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