Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v8 08/11] pci-epf-test/pci_endpoint_test: Add MSI-X
 support
To:     Kishon Vijay Abraham I <kishon@ti.com>,
        Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
        "Joao.Pinto@synopsys.com" <Joao.Pinto@synopsys.com>,
        "jingoohan1@gmail.com" <jingoohan1@gmail.com>,
        "adouglas@cadence.com" <adouglas@cadence.com>,
        "jesper.nilsson@axis.com" <jesper.nilsson@axis.com>,
        "shawn.lin@rock-chips.com" <shawn.lin@rock-chips.com>
CC:     "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <cover.1530891871.git.gustavo.pimentel@synopsys.com>
 <bbee5395f02f728be88797bb450b999725ffbbc3.1530891871.git.gustavo.pimentel@synopsys.com>
 <d670712a-d436-69eb-1daf-c66b717a36e6@ti.com>
 <ec072f79-47e5-0666-7a6e-331364ec5000@synopsys.com>
 <7bc01ee7-0645-0dcb-c74d-da3937e7d4a5@ti.com>
From:   Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
Message-ID: <b4c9447f-af71-6318-cd8d-1b4f47941476@synopsys.com>
Date:   Mon, 9 Jul 2018 18:40:04 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.0
MIME-Version: 1.0
In-Reply-To: <7bc01ee7-0645-0dcb-c74d-da3937e7d4a5@ti.com>
Content-Type: text/plain; charset="windows-1252"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 09/07/2018 11:26, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Monday 09 July 2018 03:38 PM, Gustavo Pimentel wrote:
>> Hi,
>>
>> On 09/07/2018 05:48, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Friday 06 July 2018 09:21 PM, Gustavo Pimentel wrote:
>>>> Add MSI-X support and update driver documentation accordingly.
>>>>
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> ---
>>>> Change v2->v3:
>>>>  - New patch file created base on the previous patch
>>>> "misc: pci_endpoint_test: Add MSI-X support" patch file following
>>>> Kishon's suggestion.
>>>> Change v3->v4:
>>>>  - Rebased to Lorenzo's master branch v4.18-rc1.
>>>> Change v4->v5:
>>>>  - Nothing changed, just to follow the patch set version.
>>>> Change v5->v6:
>>>>  - Moved PCITEST_MSIX ioctl entry from patch #10 to here.
>>>>  - Documented ioctl parameter type associated to
>>>> drivers/misc/pci_endpoint_test.c driver.
>>>> Change v6->v7:
>>>>  - Updated documentation.
>>>>  - Added flag that enables or not the MSI-X on the EP features. 
>>>> Change v7->v8:
>>>>  - Re-sending the patch series.
>>>>
>>>>  Documentation/PCI/endpoint/pci-test-function.txt |  2 +-
>>>>  Documentation/ioctl/ioctl-number.txt             |  1 +
>>>>  Documentation/misc-devices/pci-endpoint-test.txt |  3 +++
>>>>  drivers/misc/pci_endpoint_test.c                 | 29 +++++++++++++++++-------
>>>>  drivers/pci/controller/dwc/pcie-designware-ep.c  |  1 +
>>>>  drivers/pci/endpoint/functions/pci-epf-test.c    | 29 ++++++++++++++++++++++--
>>>>  include/linux/pci-epc.h                          |  1 +
>>>>  include/uapi/linux/pcitest.h                     |  1 +
>>>>  8 files changed, 56 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/Documentation/PCI/endpoint/pci-test-function.txt b/Documentation/PCI/endpoint/pci-test-function.txt
>>>> index 7ee2361..b1cbff3 100644
>>>> --- a/Documentation/PCI/endpoint/pci-test-function.txt
>>>> +++ b/Documentation/PCI/endpoint/pci-test-function.txt
>>>> @@ -34,7 +34,7 @@ that the endpoint device must perform.
>>>>  Bitfield Description:
>>>>    Bit 0		: raise legacy IRQ
>>>>    Bit 1		: raise MSI IRQ
>>>> -  Bit 2		: raise MSI-X IRQ (reserved for future implementation)
>>>> +  Bit 2		: raise MSI-X IRQ
>>>>    Bit 3		: read command (read data from RC buffer)
>>>>    Bit 4		: write command (write data to RC buffer)
>>>>    Bit 5		: copy command (copy data from one RC buffer to another
>>>> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
>>>> index 480c860..65259d4 100644
>>>> --- a/Documentation/ioctl/ioctl-number.txt
>>>> +++ b/Documentation/ioctl/ioctl-number.txt
>>>> @@ -166,6 +166,7 @@ Code  Seq#(hex)	Include File		Comments
>>>>  'P'	all	linux/soundcard.h	conflict!
>>>>  'P'	60-6F	sound/sscape_ioctl.h	conflict!
>>>>  'P'	00-0F	drivers/usb/class/usblp.c	conflict!
>>>> +'P'	01-07	drivers/misc/pci_endpoint_test.c	conflict!
>>>>  'Q'	all	linux/soundcard.h
>>>>  'R'	00-1F	linux/random.h		conflict!
>>>>  'R'	01	linux/rfkill.h		conflict!
>>>> diff --git a/Documentation/misc-devices/pci-endpoint-test.txt b/Documentation/misc-devices/pci-endpoint-test.txt
>>>> index 4ebc359..fdfa0f6 100644
>>>> --- a/Documentation/misc-devices/pci-endpoint-test.txt
>>>> +++ b/Documentation/misc-devices/pci-endpoint-test.txt
>>>> @@ -10,6 +10,7 @@ The PCI driver for the test device performs the following tests
>>>>  	*) verifying addresses programmed in BAR
>>>>  	*) raise legacy IRQ
>>>>  	*) raise MSI IRQ
>>>> +	*) raise MSI-X IRQ
>>>>  	*) read data
>>>>  	*) write data
>>>>  	*) copy data
>>>> @@ -25,6 +26,8 @@ ioctl
>>>>   PCITEST_LEGACY_IRQ: Tests legacy IRQ
>>>>   PCITEST_MSI: Tests message signalled interrupts. The MSI number
>>>>  	      to be tested should be passed as argument.
>>>> + PCITEST_MSIX: Tests message signalled interrupts. The MSI-X number
>>>> +	      to be tested should be passed as argument.
>>>>   PCITEST_WRITE: Perform write tests. The size of the buffer should be passed
>>>>  		as argument.
>>>>   PCITEST_READ: Perform read tests. The size of the buffer should be passed
>>>> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
>>>> index 349794c..f4fef10 100644
>>>> --- a/drivers/misc/pci_endpoint_test.c
>>>> +++ b/drivers/misc/pci_endpoint_test.c
>>>> @@ -39,13 +39,14 @@
>>>>  
>>>>  #define IRQ_TYPE_LEGACY				0
>>>>  #define IRQ_TYPE_MSI				1
>>>> +#define IRQ_TYPE_MSIX				2
>>>>  
>>>>  #define PCI_ENDPOINT_TEST_MAGIC			0x0
>>>>  
>>>>  #define PCI_ENDPOINT_TEST_COMMAND		0x4
>>>>  #define COMMAND_RAISE_LEGACY_IRQ		BIT(0)
>>>>  #define COMMAND_RAISE_MSI_IRQ			BIT(1)
>>>> -/* BIT(2) is reserved for raising MSI-X IRQ command */
>>>> +#define COMMAND_RAISE_MSIX_IRQ			BIT(2)
>>>>  #define COMMAND_READ				BIT(3)
>>>>  #define COMMAND_WRITE				BIT(4)
>>>>  #define COMMAND_COPY				BIT(5)
>>>> @@ -84,7 +85,7 @@ MODULE_PARM_DESC(no_msi, "Disable MSI interrupt in pci_endpoint_test");
>>>>  
>>>>  static int irq_type = IRQ_TYPE_MSI;
>>>>  module_param(irq_type, int, 0444);
>>>> -MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI)");
>>>> +MODULE_PARM_DESC(irq_type, "IRQ mode selection in pci_endpoint_test (0 - Legacy, 1 - MSI, 2 - MSI-X)");
>>>>  
>>>>  enum pci_barno {
>>>>  	BAR_0,
>>>> @@ -202,16 +203,18 @@ static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>>>>  }
>>>>  
>>>>  static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
>>>> -				      u8 msi_num)
>>>> +				       u16 msi_num, bool msix)
>>>>  {
>>>>  	u32 val;
>>>>  	struct pci_dev *pdev = test->pdev;
>>>>  
>>>>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
>>>> -				 IRQ_TYPE_MSI);
>>>> +				 msix == false ? IRQ_TYPE_MSI :
>>>> +				 IRQ_TYPE_MSIX);
>>>>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, msi_num);
>>>>  	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
>>>> -				 COMMAND_RAISE_MSI_IRQ);
>>>> +				 msix == false ? COMMAND_RAISE_MSI_IRQ :
>>>> +				 COMMAND_RAISE_MSIX_IRQ);
>>>>  	val = wait_for_completion_timeout(&test->irq_raised,
>>>>  					  msecs_to_jiffies(1000));
>>>>  	if (!val)
>>>> @@ -456,7 +459,8 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>>>>  		ret = pci_endpoint_test_legacy_irq(test);
>>>>  		break;
>>>>  	case PCITEST_MSI:
>>>> -		ret = pci_endpoint_test_msi_irq(test, arg);
>>>> +	case PCITEST_MSIX:
>>>> +		ret = pci_endpoint_test_msi_irq(test, arg, cmd == PCITEST_MSIX);
>>>>  		break;
>>>>  	case PCITEST_WRITE:
>>>>  		ret = pci_endpoint_test_write(test, arg);
>>>> @@ -542,6 +546,12 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>  			dev_err(dev, "Failed to get MSI interrupts\n");
>>>>  		test->num_irqs = irq;
>>>>  		break;
>>>> +	case IRQ_TYPE_MSIX:
>>>> +		irq = pci_alloc_irq_vectors(pdev, 1, 2048, PCI_IRQ_MSIX);
>>>> +		if (irq < 0)
>>>> +			dev_err(dev, "Failed to get MSI-X interrupts\n");
>>>> +		test->num_irqs = irq;
>>>> +		break;
>>>>  	default:
>>>>  		dev_err(dev, "Invalid IRQ type selected\n");
>>>>  	}
>>>> @@ -558,8 +568,9 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>  				       pci_endpoint_test_irqhandler,
>>>>  				       IRQF_SHARED, DRV_MODULE_NAME, test);
>>>>  		if (err)
>>>> -			dev_err(dev, "failed to request IRQ %d for MSI %d\n",
>>>> -				pci_irq_vector(pdev, i), i + 1);
>>>> +			dev_err(dev, "Failed to request IRQ %d for MSI%s %d\n",
>>>> +				pci_irq_vector(pdev, i),
>>>> +				irq_type == IRQ_TYPE_MSIX ? "-X" : "", i + 1);
>>>>  	}
>>>>  
>>>>  	for (bar = BAR_0; bar <= BAR_5; bar++) {
>>>> @@ -625,6 +636,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>>>>  
>>>>  err_disable_msi:
>>>>  	pci_disable_msi(pdev);
>>>> +	pci_disable_msix(pdev);
>>>>  	pci_release_regions(pdev);
>>>>  
>>>>  err_disable_pdev:
>>>> @@ -656,6 +668,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
>>>>  	for (i = 0; i < test->num_irqs; i++)
>>>>  		devm_free_irq(&pdev->dev, pci_irq_vector(pdev, i), test);
>>>>  	pci_disable_msi(pdev);
>>>> +	pci_disable_msix(pdev);
>>>>  	pci_release_regions(pdev);
>>>>  	pci_disable_device(pdev);
>>>>  }
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> index 70d0688..36d83d1 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
>>>> @@ -585,6 +585,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>>>>  	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
>>>>  
>>>>  	epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;
>>>> +	epc->features |= EPC_FEATURE_MSIX_AVAILABLE;
>>>
>>> This indicates all vendors of designware has MSIX enabled which is not true.
>>> We'll need more logic than that.
>>
>> You mentioned and excellent point. Any particularity related to this features
>> should be implemented each specific driver (in this case on
>> pcie-designware-plat.c file).
>>
>> And by looking at this code now that you mentioned, I think the line code
>> "epc->features = EPC_FEATURE_NO_LINKUP_NOTIFIER;" was added by mistake, if I
>> remember well by default the linkup notification is expected, right?
> 
> right, since dra7x was the first platform to have EP support added and it has
> linkup notification mechanism.
>>
>> If I'm right, I may create an extra patch removing this 2 lines, do you agree?

I will send a patch as soon as possible to fix this nasty bug. Unfortunately it
will be more than just deleting 2 lines....

> 
> Agree. I think we should use ->ep_init() present in dw_pcie_ep_ops for
> populating features. ->ep_init() right now is called in dw_pcie_ep_init() which
> is not needed. Stuffs that are right now done in ->ep_init callbacks can be
> done even before invoking dw_pcie_ep_init().

I don't think so, that we can invoke before, dw_pcie_ep_init() is responsible
for creating the epc object and set the object address to the epc pointer
present on the ep struct. This pointer is used later to access and set the
feature field.

> 
> We might have to add a new API pci_epc_init() to be invoked from function
> driver, which should invoke ->ep_init() callback. The features of a particular
> platform should be populated here.

My solution for now is to repair this damage as soon as possible, for that I'll
move the the feature setting from the pcie-designware-ep.c to the
pcie-designware-plat.c and change some code order to work.

I propose we implement this change after this patch has entered, otherwise we'll
be adding yet another degree of complexity to this series, already quite complex.

Can you test the the patch series v9 on your side Kishon?

Regards,
Gustavo

> 
> Thanks
> Kishon
>