[1/1] Fix ARI enabling for a NVME devices

Commit Message

Andrei Leonvikov Sept. 3, 2019, 12:53 p.m. UTC

From: Andrei Leonchikov <[e-mail]>

Signed-off-by: Andrei Leonchikov <[e-mail]>
---
 drivers/pci/pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Sept. 3, 2019, 1:14 p.m. UTC | #1

On Tue, Sep 03, 2019 at 12:53:15PM +0000, Andrei Leonvikov wrote:
> +	if ((dev->driver != NULL) && (strncmp(dev->driver->name, "nvme", 4) == 0)) {
> +		// for NVME device this field always zero, but ARI can be enabled
> +		pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
> +		if (!(cap & PCI_EXP_DEVCAP2_ARI))
> +			return;
> +	}

Besides the missing patch description, all the obvious style issues, and
the fact that you can't just check a driver name a here:

There are plenty NVMe drives that support the ARI capability, and I
don't know of any standard saying nvme device should be treated special.

Christoph Hellwig Sept. 3, 2019, 4:33 p.m. UTC | #2

[adding back the Cc list]

On Tue, Sep 03, 2019 at 07:24:15PM +0300, Андрей Леончиков wrote:
> All drives has ARI capability, but everywhere the PCI_EXP_DEVCAP2_ARI
> in the DEVCAP2 register is reset (see NVMe specification, bit 5).
> At the same time, when the device is initialized, the DEVSAP register is
> requested and this bit is checked. And if it is reset, ARI will never turn
> on.
> Because of this, it will be impossible to correctly initialize more than 8
> functions per interface (1 physical and 7 virtual).
> At the moment we are developing a disk, one of the requirements for
> which is the correct operation of up to 128 virtual functions on one
> interface.
> During testing of this device, this behavior was noticed.

Looking at the PCIe spec this bit actually means "ARI forwarding
supported" and isn't the actual ARI support.  And the PCIe spec says
about that:

"Applicable only to Switch
Downstream Ports and Root Ports; must be 0b for other
Function types. This bit must be set to 1b if a Switch
Downstream Port or Root Port supports this optional capability.
See Section 6.13 for additional details."

So I don't see how we'd ever see this bit set on an actual NVMe device.

And yes, the name for our define is a little misnamed.

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b27b5af3..ed5f0888c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3149,9 +3149,12 @@  void pci_configure_ari(struct pci_dev *dev)
 	if (!bridge)
 		return;
 
-	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
-	if (!(cap & PCI_EXP_DEVCAP2_ARI))
-		return;
+	if ((dev->driver != NULL) && (strncmp(dev->driver->name, "nvme", 4) == 0)) {
+		// for NVME device this field always zero, but ARI can be enabled
+		pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
+		if (!(cap & PCI_EXP_DEVCAP2_ARI))
+			return;
+	}
 
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ARI)) {
 		pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,