[PATCH 0/4] Cover letter

[PATCH 0/4] Cover letter

[Noa Osherovich]

The following 3 patches unmark some Mellanox devices as having broken
INTx masking.

The first patch moves all broken INTx masking to FINAL instead of
HEADER so that the fixup can execute on all archs.
The second fixup changes the Mellanox marking to be according to a
list of devices rather than a general fixup by vendor.
The third patch excludes some Mellanox devices from the INTx broken
marking according to device ID / firmware version.

Noa Osherovich (3):
  PCI: Use FINAL fixup to mark broken INTx masking
  PCI: Convert Mellanox quirks to be for listed devices only
  PCI: Add checks to mellanox_check_broken_intx_masking

 drivers/pci/quirks.c | 172 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 136 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] PCI: Use FINAL fixup to mark broken INTx masking

[Noa Osherovich]

Convert all quirk_broken_intx_masking quirks from HEADER to FINAL.
None of those calls need to be in HEADER but some might need to be
called as FINAL. Moving them all to FINAL will save time during
pci_do_fixups.

Signed-off-by: Noa Osherovich <noaos@mellanox.com>
---
 drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c232729f5b1b..85048fdf2474 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3146,53 +3146,53 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
 {
 	dev->broken_intx_masking = 1;
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, 0x0030,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(0x1814, 0x0601, /* Ralink RT2800 802.11n PCI */
-			 quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0030,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(0x1814, 0x0601, /* Ralink RT2800 802.11n PCI */
+			quirk_broken_intx_masking);
 /*
  * Realtek RTL8169 PCI Gigabit Ethernet Controller (rev 10)
  * Subsystem: Realtek RTL8169/8110 Family PCI Gigabit Ethernet NIC
  *
  * RTL8110SC - Fails under PCI device assignment using DisINTx masking.
  */
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_REALTEK, 0x8169,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
-			 quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
+			quirk_broken_intx_masking);
 
 /*
  * Intel i40e (XL710/X710) 10/20/40GbE NICs all have broken INTx masking,
  * DisINTx can be set but the interrupt status bit is non-functional.
  */
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1572,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1574,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1580,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1581,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1583,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1584,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1585,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1586,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1587,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1588,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1589,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d0,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d1,
-			 quirk_broken_intx_masking);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d2,
-			 quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1572,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1574,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1580,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1581,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1583,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1584,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1585,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1586,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1587,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1588,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1589,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d0,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d1,
+			quirk_broken_intx_masking);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d2,
+			quirk_broken_intx_masking);
 
 static void quirk_no_bus_reset(struct pci_dev *dev)
 {
-- 
1.8.3.1

Re: [PATCH 1/4] PCI: Use FINAL fixup to mark broken INTx masking

[Gavin Shan]

On Sun, Nov 13, 2016 at 04:21:39PM +0200, Noa Osherovich wrote:
>Convert all quirk_broken_intx_masking quirks from HEADER to FINAL.
>None of those calls need to be in HEADER but some might need to be
>called as FINAL. Moving them all to FINAL will save time during
>pci_do_fixups.
>
>Signed-off-by: Noa Osherovich <noaos@mellanox.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++--------------------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>index c232729f5b1b..85048fdf2474 100644
>--- a/drivers/pci/quirks.c
>+++ b/drivers/pci/quirks.c
>@@ -3146,53 +3146,53 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
> {
> 	dev->broken_intx_masking = 1;
> }
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, 0x0030,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(0x1814, 0x0601, /* Ralink RT2800 802.11n PCI */
>-			 quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0030,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(0x1814, 0x0601, /* Ralink RT2800 802.11n PCI */
>+			quirk_broken_intx_masking);
> /*
>  * Realtek RTL8169 PCI Gigabit Ethernet Controller (rev 10)
>  * Subsystem: Realtek RTL8169/8110 Family PCI Gigabit Ethernet NIC
>  *
>  * RTL8110SC - Fails under PCI device assignment using DisINTx masking.
>  */
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_REALTEK, 0x8169,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>-			 quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>+			quirk_broken_intx_masking);
>
> /*
>  * Intel i40e (XL710/X710) 10/20/40GbE NICs all have broken INTx masking,
>  * DisINTx can be set but the interrupt status bit is non-functional.
>  */
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1572,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1574,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1580,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1581,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1583,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1584,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1585,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1586,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1587,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1588,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1589,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d0,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d1,
>-			 quirk_broken_intx_masking);
>-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x37d2,
>-			 quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1572,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1574,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1580,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1581,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1583,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1584,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1585,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1586,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1587,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1588,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1589,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d0,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d1,
>+			quirk_broken_intx_masking);
>+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x37d2,
>+			quirk_broken_intx_masking);
>

There could be one follow-up patch to combine those intel's
quirks to one, so that more time can be saved. We don't have
to cover it in this patchset though.

> static void quirk_no_bus_reset(struct pci_dev *dev)
> {

Thanks,
Gavin

[PATCH 2/4] PCI: Convert Mellanox quirks to be for listed devices only

[Noa Osherovich]

Change Mellanox's broken_intx_masking quirk from an "all Mellanox
devices" to a quirk for listed devices only.

Signed-off-by: Noa Osherovich <noaos@mellanox.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/pci/quirks.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 85048fdf2474..d3977c847e1f 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3158,8 +3158,59 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
  */
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
 			quirk_broken_intx_masking);
+
+#define PCI_DEVICE_ID_MELLANOX_HERMON_SDR 0x6340
+#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR 0x634a
+#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR 0x6354
+#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR_GEN2 0x6732
+#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR_GEN2 0x673c
+#define PCI_DEVICE_ID_MELLANOX_HERMON_EN 0x6368
+#define PCI_DEVICE_ID_MELLANOX_HERMON_EN_GEN2 0x6750
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN 0x6372
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_T_GEN2 0x675a
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_GEN2 0x6764
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_5_GEN2 0x6746
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX2 0x676e
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX3 0x1003
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO 0x1007
+#define PCI_DEVICE_ID_MELLANOX_CONNECTIB 0x1011
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX4 0x1013
+#define PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX 0x1015
+
+static u16 mellanox_broken_intx_devs[] = {
+	PCI_DEVICE_ID_MELLANOX_HERMON_SDR,
+	PCI_DEVICE_ID_MELLANOX_HERMON_DDR,
+	PCI_DEVICE_ID_MELLANOX_HERMON_QDR,
+	PCI_DEVICE_ID_MELLANOX_HERMON_DDR_GEN2,
+	PCI_DEVICE_ID_MELLANOX_HERMON_QDR_GEN2,
+	PCI_DEVICE_ID_MELLANOX_HERMON_EN,
+	PCI_DEVICE_ID_MELLANOX_HERMON_EN_GEN2,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_T_GEN2,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_GEN2,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_5_GEN2,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
+	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
+	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
+};
+
+static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
+		if (dev->device == mellanox_broken_intx_devs[i]) {
+			dev->broken_intx_masking = 1;
+			return;
+		}
+	}
+}
+
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
-			quirk_broken_intx_masking);
+			mellanox_check_broken_intx_masking);
 
 /*
  * Intel i40e (XL710/X710) 10/20/40GbE NICs all have broken INTx masking,
-- 
1.8.3.1

Re: [PATCH 2/4] PCI: Convert Mellanox quirks to be for listed devices only

[Gavin Shan]

On Sun, Nov 13, 2016 at 04:21:40PM +0200, Noa Osherovich wrote:
>Change Mellanox's broken_intx_masking quirk from an "all Mellanox
>devices" to a quirk for listed devices only.
>
>Signed-off-by: Noa Osherovich <noaos@mellanox.com>
>Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
> drivers/pci/quirks.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>index 85048fdf2474..d3977c847e1f 100644
>--- a/drivers/pci/quirks.c
>+++ b/drivers/pci/quirks.c
>@@ -3158,8 +3158,59 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
>  */
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169,
> 			quirk_broken_intx_masking);
>+
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_SDR 0x6340
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR 0x634a
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR 0x6354
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_DDR_GEN2 0x6732
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_QDR_GEN2 0x673c
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_EN 0x6368
>+#define PCI_DEVICE_ID_MELLANOX_HERMON_EN_GEN2 0x6750
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN 0x6372
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_T_GEN2 0x675a
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_GEN2 0x6764
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_5_GEN2 0x6746
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX2 0x676e
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX3 0x1003
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO 0x1007
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTIB 0x1011
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX4 0x1013
>+#define PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX 0x1015
>+
>+static u16 mellanox_broken_intx_devs[] = {
>+	PCI_DEVICE_ID_MELLANOX_HERMON_SDR,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_DDR,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_QDR,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_DDR_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_QDR_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_EN,
>+	PCI_DEVICE_ID_MELLANOX_HERMON_EN_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_T_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX_EN_5_GEN2,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
>+	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
>+};
>+
>+static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
>+{
>+	int i;
>+
>+	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
>+		if (dev->device == mellanox_broken_intx_devs[i]) {
>+			dev->broken_intx_masking = 1;
>+			return;
>+		}
>+	}
>+}
>+

@dev might be replaced with @pdev. Usually, @dev means "struct device"
instance while @pdev represents "struct pci_dev" instance. The older
PCI code seems using them interchangeably.

> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>-			quirk_broken_intx_masking);
>+			mellanox_check_broken_intx_masking);
>
> /*
>  * Intel i40e (XL710/X710) 10/20/40GbE NICs all have broken INTx masking,

Thanks,
Gavin

[PATCH 3/4] PCI: Add checks to mellanox_check_broken_intx_masking

[Noa Osherovich]

Mellanox devices were marked as having INTx masking ability broken.
As a result, the VFIO driver fails to start when more than one device
function is passed-through to a VM if both have the same INTx pin.

Prior to Connect-IB, Mellanox devices exposed to the operating system
one PCI function per all ports.
Starting from Connect-IB, the devices are function-per-port. When
passing the second function to a VM, VFIO will fail to start.

Exclude ConnectX-4, ConnectX4-Lx and Connect-IB from the list of
Mellanox devices marked as having broken INTx masking:

- ConnectX-4 and ConnectX4-LX firmware version is checked. If INTx
  masking is supported, we unmark the broken INTx masking.
- Connect-IB does not support INTx currently so will not cause any
  problem.

Fixes: 11e42532ada31 ('PCI: Assume all Mellanox devices have ...')
Signed-off-by: Noa Osherovich <noaos@mellanox.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d3977c847e1f..cbd6776e70e6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3192,21 +3192,70 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
 	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
 	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
 	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
-	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
-	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
-	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
 };
 
+#define CONNECTX_4_CURR_MAX_MINOR 99
+#define CONNECTX_4_INTX_SUPPORT_MINOR 14
+
+/*
+ * Checking ConnectX-4/LX FW version to see if it supports legacy interrupts.
+ * If so, don't mark it as broken.
+ * FW minor > 99 means older FW version format and no INTx masking support.
+ * FW minor < 14 means new FW version format and no INTx masking support.
+ */
 static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
 {
+	__be32 __iomem *fw_ver;
+	u16 fw_major;
+	u16 fw_minor;
+	u16 fw_subminor;
+	u32 fw_maj_min;
+	u32 fw_sub_min;
 	int i;
 
+	dev->broken_intx_masking = 1;
+
 	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
-		if (dev->device == mellanox_broken_intx_devs[i]) {
-			dev->broken_intx_masking = 1;
+		if (dev->device == mellanox_broken_intx_devs[i])
 			return;
-		}
 	}
+
+	/* Getting here means Connect-IB cards and up. Connect-IB has no INTx
+	 * support so shouldn't be checked further
+	 */
+	if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB) {
+		dev->broken_intx_masking = 0;
+		return;
+	}
+
+	/* For ConnectX-4 and ConnectX-4LX, need to check FW support */
+	if (pci_enable_device_mem(dev)) {
+		dev_warn(&dev->dev, "Can't enable device memory\n");
+		return;
+	}
+
+	/* Convert from PCI bus to resource space. */
+	fw_ver = ioremap(pci_resource_start(dev, 0), 4);
+	if (!fw_ver) {
+		dev_warn(&dev->dev, "Can't map ConnectX-4 initialization segment\n");
+		return;
+	}
+
+	/* Reading from resource space should be 32b aligned */
+	fw_maj_min = ioread32be(fw_ver);
+	fw_sub_min = ioread32be(fw_ver + 1);
+	fw_major = fw_maj_min & 0xffff;
+	fw_minor = fw_maj_min >> 16;
+	fw_subminor = fw_sub_min & 0xffff;
+	if (fw_minor > CONNECTX_4_CURR_MAX_MINOR ||
+	    fw_minor < CONNECTX_4_INTX_SUPPORT_MINOR)
+		dev_warn(&dev->dev, "ConnectX-4: FW %u.%u.%u doesn't support INTx masking, disabling. Please upgrade FW to %d.14.1100 and up for INTx support\n",
+			 fw_major, fw_minor, fw_subminor, dev->device ==
+			 PCI_DEVICE_ID_MELLANOX_CONNECTX4 ? 12 : 14);
+	else
+		dev->broken_intx_masking = 0;
+
+	iounmap(fw_ver);
 }
 
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
-- 
1.8.3.1

Re: [PATCH 3/4] PCI: Add checks to mellanox_check_broken_intx_masking

[Gavin Shan]

On Sun, Nov 13, 2016 at 04:21:41PM +0200, Noa Osherovich wrote:
>Mellanox devices were marked as having INTx masking ability broken.
>As a result, the VFIO driver fails to start when more than one device
>function is passed-through to a VM if both have the same INTx pin.
>
>Prior to Connect-IB, Mellanox devices exposed to the operating system
>one PCI function per all ports.
>Starting from Connect-IB, the devices are function-per-port. When
>passing the second function to a VM, VFIO will fail to start.
>
>Exclude ConnectX-4, ConnectX4-Lx and Connect-IB from the list of
>Mellanox devices marked as having broken INTx masking:
>
>- ConnectX-4 and ConnectX4-LX firmware version is checked. If INTx
>  masking is supported, we unmark the broken INTx masking.
>- Connect-IB does not support INTx currently so will not cause any
>  problem.
>
>Fixes: 11e42532ada31 ('PCI: Assume all Mellanox devices have ...')
>Signed-off-by: Noa Osherovich <noaos@mellanox.com>
>Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

With below comments addressed:

>---
> drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>index d3977c847e1f..cbd6776e70e6 100644
>--- a/drivers/pci/quirks.c
>+++ b/drivers/pci/quirks.c
>@@ -3192,21 +3192,70 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
> 	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
> 	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
> 	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
>-	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
>-	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
>-	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
> };
>
>+#define CONNECTX_4_CURR_MAX_MINOR 99
>+#define CONNECTX_4_INTX_SUPPORT_MINOR 14
>+
>+/*
>+ * Checking ConnectX-4/LX FW version to see if it supports legacy interrupts.
>+ * If so, don't mark it as broken.
>+ * FW minor > 99 means older FW version format and no INTx masking support.
>+ * FW minor < 14 means new FW version format and no INTx masking support.
>+ */
> static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
> {
>+	__be32 __iomem *fw_ver;
>+	u16 fw_major;
>+	u16 fw_minor;
>+	u16 fw_subminor;
>+	u32 fw_maj_min;
>+	u32 fw_sub_min;
> 	int i;
>
>+	dev->broken_intx_masking = 1;
>+
> 	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
>-		if (dev->device == mellanox_broken_intx_devs[i]) {
>-			dev->broken_intx_masking = 1;
>+		if (dev->device == mellanox_broken_intx_devs[i])
> 			return;
>-		}
> 	}
>+
>+	/* Getting here means Connect-IB cards and up. Connect-IB has no INTx
>+	 * support so shouldn't be checked further
>+	 */
>+	if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB) {
>+		dev->broken_intx_masking = 0;
>+		return;
>+	}
>+
>+	/* For ConnectX-4 and ConnectX-4LX, need to check FW support */
>+	if (pci_enable_device_mem(dev)) {
>+		dev_warn(&dev->dev, "Can't enable device memory\n");
>+		return;
>+	}

It might be safer to set @broken_intx_masking in the failing path. On the
following exit or failing path, the device needs to be disabled with function
pci_disable_device(). Otherwise, &dev->enable_cnt, tracking if the device is
enabled or not, will be unbalanced.

>+
>+	/* Convert from PCI bus to resource space. */
>+	fw_ver = ioremap(pci_resource_start(dev, 0), 4);
>+	if (!fw_ver) {
>+		dev_warn(&dev->dev, "Can't map ConnectX-4 initialization segment\n");
>+		return;
>+	}
>+
>+	/* Reading from resource space should be 32b aligned */
>+	fw_maj_min = ioread32be(fw_ver);
>+	fw_sub_min = ioread32be(fw_ver + 1);
>+	fw_major = fw_maj_min & 0xffff;
>+	fw_minor = fw_maj_min >> 16;
>+	fw_subminor = fw_sub_min & 0xffff;
>+	if (fw_minor > CONNECTX_4_CURR_MAX_MINOR ||
>+	    fw_minor < CONNECTX_4_INTX_SUPPORT_MINOR)
>+		dev_warn(&dev->dev, "ConnectX-4: FW %u.%u.%u doesn't support INTx masking, disabling. Please upgrade FW to %d.14.1100 and up for INTx support\n",
>+			 fw_major, fw_minor, fw_subminor, dev->device ==
>+			 PCI_DEVICE_ID_MELLANOX_CONNECTX4 ? 12 : 14);
>+	else
>+		dev->broken_intx_masking = 0;
>+
>+	iounmap(fw_ver);
> }

Noa, it doesn't look quite correct: when a device ID doesn't match with
anyone in the list, CONNECTIB, CONNECT4 or CONNECTX4_LX. The code goes
though as it's CONNECT4 or CONNECT4_LX. The firmware version retrieved
from MMIO register (BAR 0, offset 0/4) are checked. I don't think it's
assured that the registers are for firmware version on the device. It
seems you need more checks here: @broken_intx_masking is untouched and
kept as 0 by default when the device ID isn't in the intrest list, which
is the policy you introduced in PATCH[2/3]. Something like below would
work:

static void mellanox_check_broken_intx_masking(struct pci_dev *pdev)
{
     /*
      * Set @broken_intx_masking to 1 when device ID matches with
      * anyone in the list. No code change to PATCH[2/3] is needed.
      */
     if (pdev->device in mellanox_broken_intx_devs) {
         pdev->broken_intx_masking = 1;
         return;
     }

     if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB)
         return;

     if (dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4 &&
         dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4_LE)
         return;

     /* Check firmware version on CONNECT4 or CONNECT4_LE */
}

>
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,

Thanks,
Gavin

Re: [PATCH 3/4] PCI: Add checks to mellanox_check_broken_intx_masking

[Bjorn Helgaas]

On Mon, Nov 14, 2016 at 11:15:35AM +1100, Gavin Shan wrote:
> On Sun, Nov 13, 2016 at 04:21:41PM +0200, Noa Osherovich wrote:
> >Mellanox devices were marked as having INTx masking ability broken.
> >As a result, the VFIO driver fails to start when more than one device
> >function is passed-through to a VM if both have the same INTx pin.
> >
> >Prior to Connect-IB, Mellanox devices exposed to the operating system
> >one PCI function per all ports.
> >Starting from Connect-IB, the devices are function-per-port. When
> >passing the second function to a VM, VFIO will fail to start.
> >
> >Exclude ConnectX-4, ConnectX4-Lx and Connect-IB from the list of
> >Mellanox devices marked as having broken INTx masking:
> >
> >- ConnectX-4 and ConnectX4-LX firmware version is checked. If INTx
> >  masking is supported, we unmark the broken INTx masking.
> >- Connect-IB does not support INTx currently so will not cause any
> >  problem.
> >
> >Fixes: 11e42532ada31 ('PCI: Assume all Mellanox devices have ...')
> >Signed-off-by: Noa Osherovich <noaos@mellanox.com>
> >Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
> 
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> 
> With below comments addressed:

Noa, would you mind refreshing this to address Gavin's comments?
I don't want to risk doing it myself and breaking something.

> >---
> > drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 55 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >index d3977c847e1f..cbd6776e70e6 100644
> >--- a/drivers/pci/quirks.c
> >+++ b/drivers/pci/quirks.c
> >@@ -3192,21 +3192,70 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
> > 	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
> > 	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
> > 	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
> >-	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
> >-	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
> >-	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
> > };
> >
> >+#define CONNECTX_4_CURR_MAX_MINOR 99
> >+#define CONNECTX_4_INTX_SUPPORT_MINOR 14
> >+
> >+/*
> >+ * Checking ConnectX-4/LX FW version to see if it supports legacy interrupts.
> >+ * If so, don't mark it as broken.
> >+ * FW minor > 99 means older FW version format and no INTx masking support.
> >+ * FW minor < 14 means new FW version format and no INTx masking support.
> >+ */
> > static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
> > {
> >+	__be32 __iomem *fw_ver;
> >+	u16 fw_major;
> >+	u16 fw_minor;
> >+	u16 fw_subminor;
> >+	u32 fw_maj_min;
> >+	u32 fw_sub_min;
> > 	int i;
> >
> >+	dev->broken_intx_masking = 1;
> >+
> > 	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
> >-		if (dev->device == mellanox_broken_intx_devs[i]) {
> >-			dev->broken_intx_masking = 1;
> >+		if (dev->device == mellanox_broken_intx_devs[i])
> > 			return;
> >-		}
> > 	}
> >+
> >+	/* Getting here means Connect-IB cards and up. Connect-IB has no INTx
> >+	 * support so shouldn't be checked further
> >+	 */
> >+	if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB) {
> >+		dev->broken_intx_masking = 0;
> >+		return;
> >+	}
> >+
> >+	/* For ConnectX-4 and ConnectX-4LX, need to check FW support */
> >+	if (pci_enable_device_mem(dev)) {
> >+		dev_warn(&dev->dev, "Can't enable device memory\n");
> >+		return;
> >+	}
> 
> It might be safer to set @broken_intx_masking in the failing path. On the
> following exit or failing path, the device needs to be disabled with function
> pci_disable_device(). Otherwise, &dev->enable_cnt, tracking if the device is
> enabled or not, will be unbalanced.
> 
> >+
> >+	/* Convert from PCI bus to resource space. */
> >+	fw_ver = ioremap(pci_resource_start(dev, 0), 4);
> >+	if (!fw_ver) {
> >+		dev_warn(&dev->dev, "Can't map ConnectX-4 initialization segment\n");
> >+		return;
> >+	}
> >+
> >+	/* Reading from resource space should be 32b aligned */
> >+	fw_maj_min = ioread32be(fw_ver);
> >+	fw_sub_min = ioread32be(fw_ver + 1);
> >+	fw_major = fw_maj_min & 0xffff;
> >+	fw_minor = fw_maj_min >> 16;
> >+	fw_subminor = fw_sub_min & 0xffff;
> >+	if (fw_minor > CONNECTX_4_CURR_MAX_MINOR ||
> >+	    fw_minor < CONNECTX_4_INTX_SUPPORT_MINOR)
> >+		dev_warn(&dev->dev, "ConnectX-4: FW %u.%u.%u doesn't support INTx masking, disabling. Please upgrade FW to %d.14.1100 and up for INTx support\n",
> >+			 fw_major, fw_minor, fw_subminor, dev->device ==
> >+			 PCI_DEVICE_ID_MELLANOX_CONNECTX4 ? 12 : 14);
> >+	else
> >+		dev->broken_intx_masking = 0;
> >+
> >+	iounmap(fw_ver);
> > }
> 
> Noa, it doesn't look quite correct: when a device ID doesn't match with
> anyone in the list, CONNECTIB, CONNECT4 or CONNECTX4_LX. The code goes
> though as it's CONNECT4 or CONNECT4_LX. The firmware version retrieved
> from MMIO register (BAR 0, offset 0/4) are checked. I don't think it's
> assured that the registers are for firmware version on the device. It
> seems you need more checks here: @broken_intx_masking is untouched and
> kept as 0 by default when the device ID isn't in the intrest list, which
> is the policy you introduced in PATCH[2/3]. Something like below would
> work:
> 
> static void mellanox_check_broken_intx_masking(struct pci_dev *pdev)
> {
>      /*
>       * Set @broken_intx_masking to 1 when device ID matches with
>       * anyone in the list. No code change to PATCH[2/3] is needed.
>       */
>      if (pdev->device in mellanox_broken_intx_devs) {
>          pdev->broken_intx_masking = 1;
>          return;
>      }
> 
>      if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB)
>          return;
> 
>      if (dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4 &&
>          dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4_LE)
>          return;
> 
>      /* Check firmware version on CONNECT4 or CONNECT4_LE */
> }
> 
> >
> > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
> 
> Thanks,
> Gavin
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] PCI: Add checks to mellanox_check_broken_intx_masking

[Noa Osherovich]

Hi Bjorn, Gavin,

On 11/15/2016 12:35 AM, Bjorn Helgaas wrote:

> On Mon, Nov 14, 2016 at 11:15:35AM +1100, Gavin Shan wrote:
>> On Sun, Nov 13, 2016 at 04:21:41PM +0200, Noa Osherovich wrote:
>>> Mellanox devices were marked as having INTx masking ability broken.
>>> As a result, the VFIO driver fails to start when more than one device
>>> function is passed-through to a VM if both have the same INTx pin.
>>>
>>> Prior to Connect-IB, Mellanox devices exposed to the operating system
>>> one PCI function per all ports.
>>> Starting from Connect-IB, the devices are function-per-port. When
>>> passing the second function to a VM, VFIO will fail to start.
>>>
>>> Exclude ConnectX-4, ConnectX4-Lx and Connect-IB from the list of
>>> Mellanox devices marked as having broken INTx masking:
>>>
>>> - ConnectX-4 and ConnectX4-LX firmware version is checked. If INTx
>>>  masking is supported, we unmark the broken INTx masking.
>>> - Connect-IB does not support INTx currently so will not cause any
>>>  problem.
>>>
>>> Fixes: 11e42532ada31 ('PCI: Assume all Mellanox devices have ...')
>>> Signed-off-by: Noa Osherovich <noaos@mellanox.com>
>>> Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
>> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>
>> With below comments addressed:
> Noa, would you mind refreshing this to address Gavin's comments?
> I don't want to risk doing it myself and breaking something.

The series is ready after Gavin's fixes, I'll send it after an internal

review.

>>> ---
>>> drivers/pci/quirks.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 55 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index d3977c847e1f..cbd6776e70e6 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3192,21 +3192,70 @@ static void quirk_broken_intx_masking(struct pci_dev *dev)
>>> 	PCI_DEVICE_ID_MELLANOX_CONNECTX2,
>>> 	PCI_DEVICE_ID_MELLANOX_CONNECTX3,
>>> 	PCI_DEVICE_ID_MELLANOX_CONNECTX3_PRO,
>>> -	PCI_DEVICE_ID_MELLANOX_CONNECTIB,
>>> -	PCI_DEVICE_ID_MELLANOX_CONNECTX4,
>>> -	PCI_DEVICE_ID_MELLANOX_CONNECTX4_LX
>>> };
>>>
>>> +#define CONNECTX_4_CURR_MAX_MINOR 99
>>> +#define CONNECTX_4_INTX_SUPPORT_MINOR 14
>>> +
>>> +/*
>>> + * Checking ConnectX-4/LX FW version to see if it supports legacy interrupts.
>>> + * If so, don't mark it as broken.
>>> + * FW minor > 99 means older FW version format and no INTx masking support.
>>> + * FW minor < 14 means new FW version format and no INTx masking support.
>>> + */
>>> static void mellanox_check_broken_intx_masking(struct pci_dev *dev)
>>> {
>>> +	__be32 __iomem *fw_ver;
>>> +	u16 fw_major;
>>> +	u16 fw_minor;
>>> +	u16 fw_subminor;
>>> +	u32 fw_maj_min;
>>> +	u32 fw_sub_min;
>>> 	int i;
>>>
>>> +	dev->broken_intx_masking = 1;
>>> +
>>> 	for (i = 0; i < ARRAY_SIZE(mellanox_broken_intx_devs); i++) {
>>> -		if (dev->device == mellanox_broken_intx_devs[i]) {
>>> -			dev->broken_intx_masking = 1;
>>> +		if (dev->device == mellanox_broken_intx_devs[i])
>>> 			return;
>>> -		}
>>> 	}
>>> +
>>> +	/* Getting here means Connect-IB cards and up. Connect-IB has no INTx
>>> +	 * support so shouldn't be checked further
>>> +	 */
>>> +	if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB) {
>>> +		dev->broken_intx_masking = 0;
>>> +		return;
>>> +	}
>>> +
>>> +	/* For ConnectX-4 and ConnectX-4LX, need to check FW support */
>>> +	if (pci_enable_device_mem(dev)) {
>>> +		dev_warn(&dev->dev, "Can't enable device memory\n");
>>> +		return;
>>> +	}
>> It might be safer to set @broken_intx_masking in the failing path. On the
>> following exit or failing path, the device needs to be disabled with function
>> pci_disable_device(). Otherwise, &dev->enable_cnt, tracking if the device is
>> enabled or not, will be unbalanced.
>>
>>> +
>>> +	/* Convert from PCI bus to resource space. */
>>> +	fw_ver = ioremap(pci_resource_start(dev, 0), 4);
>>> +	if (!fw_ver) {
>>> +		dev_warn(&dev->dev, "Can't map ConnectX-4 initialization segment\n");
>>> +		return;
>>> +	}
>>> +
>>> +	/* Reading from resource space should be 32b aligned */
>>> +	fw_maj_min = ioread32be(fw_ver);
>>> +	fw_sub_min = ioread32be(fw_ver + 1);
>>> +	fw_major = fw_maj_min & 0xffff;
>>> +	fw_minor = fw_maj_min >> 16;
>>> +	fw_subminor = fw_sub_min & 0xffff;
>>> +	if (fw_minor > CONNECTX_4_CURR_MAX_MINOR ||
>>> +	    fw_minor < CONNECTX_4_INTX_SUPPORT_MINOR)
>>> +		dev_warn(&dev->dev, "ConnectX-4: FW %u.%u.%u doesn't support INTx masking, disabling. Please upgrade FW to %d.14.1100 and up for INTx support\n",
>>> +			 fw_major, fw_minor, fw_subminor, dev->device ==
>>> +			 PCI_DEVICE_ID_MELLANOX_CONNECTX4 ? 12 : 14);
>>> +	else
>>> +		dev->broken_intx_masking = 0;
>>> +
>>> +	iounmap(fw_ver);
>>> }
>> Noa, it doesn't look quite correct: when a device ID doesn't match with
>> anyone in the list, CONNECTIB, CONNECT4 or CONNECTX4_LX. The code goes
>> though as it's CONNECT4 or CONNECT4_LX. The firmware version retrieved
>> from MMIO register (BAR 0, offset 0/4) are checked. I don't think it's
>> assured that the registers are for firmware version on the device. It
>> seems you need more checks here: @broken_intx_masking is untouched and
>> kept as 0 by default when the device ID isn't in the intrest list, which
>> is the policy you introduced in PATCH[2/3]. Something like below would
>> work:
>>
>> static void mellanox_check_broken_intx_masking(struct pci_dev *pdev)
>> {
>>      /*
>>       * Set @broken_intx_masking to 1 when device ID matches with
>>       * anyone in the list. No code change to PATCH[2/3] is needed.
>>       */
>>      if (pdev->device in mellanox_broken_intx_devs) {
>>          pdev->broken_intx_masking = 1;
>>          return;
>>      }
>>
>>      if (dev->device == PCI_DEVICE_ID_MELLANOX_CONNECTIB)
>>          return;
>>
>>      if (dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4 &&
>>          dev->device != PCI_DEVICE_ID_MELLANOX_CONNECTX4_LE)
>>          return;
>>
>>      /* Check firmware version on CONNECT4 or CONNECT4_LE */
>> }
>>
>>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>> Thanks,
>> Gavin
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Cover letter

[Gavin Shan]

On Sun, Nov 13, 2016 at 04:21:38PM +0200, Noa Osherovich wrote:
>The following 3 patches unmark some Mellanox devices as having broken
>INTx masking.
>
>The first patch moves all broken INTx masking to FINAL instead of
>HEADER so that the fixup can execute on all archs.
>The second fixup changes the Mellanox marking to be according to a
>list of devices rather than a general fixup by vendor.
>The third patch excludes some Mellanox devices from the INTx broken
>marking according to device ID / firmware version.
>

Noa, some of your script might be broken. As the subject, which is
"[PATCH 0/4] Cover letter", there are 4 patches in total. However,
I just saw 3 patches.

Thanks,
Gavin

>Noa Osherovich (3):
>  PCI: Use FINAL fixup to mark broken INTx masking
>  PCI: Convert Mellanox quirks to be for listed devices only
>  PCI: Add checks to mellanox_check_broken_intx_masking
>
> drivers/pci/quirks.c | 172 ++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 136 insertions(+), 36 deletions(-)
>
>-- 
>1.8.3.1
>