PCI: Move test of INTx masking to pci_setup_device

The test for INTx masking via config space command performed in pci_intx_mask_supported() should be performed before PCI device can be used. This is to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register which may collide with MSI/MSI-X interrupts.

This patch moves test performed in pci_intx_mask_supported() to

static void pci_test_intx_masking(struct pci_dev *dev)

defined in drivers/pci/probe.c.

This function is called from pci_setup_device(). It skips the test if the device has been already marked to have broken INTx masking feature. Otherwise the test is executed and broken_intx_masking field of struct pci_dev is set accordingly. broken_intx_masking meaning is: if it is true then the test has been either skipped because the device has been already known to have broken INTx masking support, or the test's been done and it has detected INTx masking support to be broken.
The test result can be queried at any time later from the pci_dev using same interface as before (though whith changed implementation)

static inline bool pci_intx_mask_supported(struct pci_dev *pdev) {
/*
* INTx masking is supported if device has not been marked
* to have this feature broken and it has passed
* pci_test_intx_masking() test.
*/
return !pdev->broken_intx_masking;
}

so current users of pci_intx_mask_supported: uio and vfio, keep their code unchanged.

Signed-off-by Piotr Gregor <[email protected]>

---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 56 insertions(+), 43 deletions(-)

drivers/pci/pci.c +1 -41

index b01bd5b..7c4e1aa 100644
@@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-/**
- * pci_intx_mask_supported - probe for INTx masking support
- * @dev: the PCI device to operate on
- *
- * Check if the device dev support INTx masking via the config space
- * command word.
- */
-bool pci_intx_mask_supported(struct pci_dev *dev)
-{
-	bool mask_supported = false;
-	u16 orig, new;
-
-	if (dev->broken_intx_masking)
-		return false;
-
-	pci_cfg_access_lock(dev);
-
-	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(dev, PCI_COMMAND, &new);
-
-	/*
-	 * There's no way to protect against hardware bugs or detect them
-	 * reliably, but as long as we know what the value should be, let's
-	 * go ahead and check it.
-	 */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
-			orig, new);
-	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
-		mask_supported = true;
-		pci_write_config_word(dev, PCI_COMMAND, orig);
-	}
-
-	pci_cfg_access_unlock(dev);
-	return mask_supported;
-}
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
-
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
 	struct pci_bus *bus = dev->bus;

@@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
  * @dev: the PCI device to operate on
  *
  * Check if the device dev has its INTx line asserted, mask it and
- * return true in that case. False is returned if not interrupt was
+ * return true in that case. False is returned if no interrupt was
  * pending.
  */
 bool pci_check_and_mask_intx(struct pci_dev *dev)

drivers/pci/probe.c +44 -0

index 19c8950..ee6b55c 100644
@@ -1330,6 +1330,48 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 }
 
 /**
+ * pci_test_intx_masking - probe for INTx masking support
+ * @dev: the PCI device to operate on
+ *
+ * Check if the @dev supports INTx masking via the config space
+ * command word. Executed when PCI device is setup. Result is saved
+ * in broken_intx_masking field of struct pci_dev and can be checked
+ * with pci_intx_mask_supported at any time later, after the PCI device
+ * has been setup (this avoids testing of PCI_COMMAND_INTX_DISABLE
+ * register at runtime).
+ */
+static void pci_test_intx_masking(struct pci_dev *dev)
+{
+	u16 orig, toggle, new;
+
+	/*
+	 * If device doesn't support this feature though it could pass the test.
+	 */
+	if (dev->broken_intx_masking)
+		return;
+
+	pci_cfg_access_lock(dev);
+
+	/*
+	 * Perform the test.
+	 */
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	/*
+	 * Restore initial state.
+	 */
+	pci_write_config_word(dev, PCI_COMMAND, orig);
+
+	pci_cfg_access_unlock(dev);
+
+	if (new != toggle)
+		dev->broken_intx_masking = 1;
+}
+
+/**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *

@@ -1399,6 +1441,8 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	pci_test_intx_masking(dev);
+
 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)

include/linux/pci.h +11 -2

index 33c2b0b..8a307fc 100644
@@ -366,7 +366,7 @@ struct pci_dev {
 	unsigned int	is_thunderbolt:1; /* Thunderbolt controller */
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
-	unsigned int	broken_intx_masking:1;
+	unsigned int	broken_intx_masking:1; /* INTx masking can't be used */
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;

@@ -1003,6 +1003,16 @@ int __must_check pci_reenable_device(struct pci_dev *);
 int __must_check pcim_enable_device(struct pci_dev *pdev);
 void pcim_pin_device(struct pci_dev *pdev);
 
+static inline bool pci_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if device has not been marked
+	 * to have this feature broken and it has passed
+	 * pci_test_intx_masking() test.
+	 */
+	return !pdev->broken_intx_masking;
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);

@@ -1026,7 +1036,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
-bool pci_intx_mask_supported(struct pci_dev *dev);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
 int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask);

The test for INTx masking via config space command performed in pci_intx_mask_supported() should be performed before PCI device can be used. This is to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register which may collide with MSI/MSI-X interrupts.

This patch moves test performed in pci_intx_mask_supported() to

static void pci_test_intx_masking(struct pci_dev *dev)

defined in drivers/pci/probe.c.

This function is called from pci_setup_device(). It skips the test if the device has been already marked to have broken INTx masking feature. Otherwise the test is executed and broken_intx_masking field of struct pci_dev is set accordingly. broken_intx_masking meaning is: if it is true then the test has been either skipped because the device has been already known to have broken INTx masking support, or the test's been done and it has detected INTx masking support to be broken.
The test result can be queried at any time later from the pci_dev using same interface as before (though whith changed implementation)

static inline bool pci_intx_mask_supported(struct pci_dev *pdev) {
/*
* INTx masking is supported if device has not been marked
* to have this feature broken and it has passed
* pci_test_intx_masking() test.
*/
return !pdev->broken_intx_masking;
}

so current users of pci_intx_mask_supported: uio and vfio, keep their code unchanged.

Signed-off-by Piotr Gregor <[email protected]>

---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 56 insertions(+), 43 deletions(-)

drivers/pci/pci.c +1 -41

index b01bd5b..7c4e1aa 100644
@@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-/**
- * pci_intx_mask_supported - probe for INTx masking support
- * @dev: the PCI device to operate on
- *
- * Check if the device dev support INTx masking via the config space
- * command word.
- */
-bool pci_intx_mask_supported(struct pci_dev *dev)
-{
-	bool mask_supported = false;
-	u16 orig, new;
-
-	if (dev->broken_intx_masking)
-		return false;
-
-	pci_cfg_access_lock(dev);
-
-	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(dev, PCI_COMMAND, &new);
-
-	/*
-	 * There's no way to protect against hardware bugs or detect them
-	 * reliably, but as long as we know what the value should be, let's
-	 * go ahead and check it.
-	 */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
-			orig, new);
-	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
-		mask_supported = true;
-		pci_write_config_word(dev, PCI_COMMAND, orig);
-	}
-
-	pci_cfg_access_unlock(dev);
-	return mask_supported;
-}
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
-
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
 	struct pci_bus *bus = dev->bus;

@@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
  * @dev: the PCI device to operate on
  *
  * Check if the device dev has its INTx line asserted, mask it and
- * return true in that case. False is returned if not interrupt was
+ * return true in that case. False is returned if no interrupt was
  * pending.
  */
 bool pci_check_and_mask_intx(struct pci_dev *dev)

drivers/pci/probe.c +44 -0

index 19c8950..ee6b55c 100644
@@ -1330,6 +1330,48 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 }
 
 /**
+ * pci_test_intx_masking - probe for INTx masking support
+ * @dev: the PCI device to operate on
+ *
+ * Check if the @dev supports INTx masking via the config space
+ * command word. Executed when PCI device is setup. Result is saved
+ * in broken_intx_masking field of struct pci_dev and can be checked
+ * with pci_intx_mask_supported at any time later, after the PCI device
+ * has been setup (this avoids testing of PCI_COMMAND_INTX_DISABLE
+ * register at runtime).
+ */
+static void pci_test_intx_masking(struct pci_dev *dev)
+{
+	u16 orig, toggle, new;
+
+	/*
+	 * If device doesn't support this feature though it could pass the test.
+	 */
+	if (dev->broken_intx_masking)
+		return;
+
+	pci_cfg_access_lock(dev);
+
+	/*
+	 * Perform the test.
+	 */
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	/*
+	 * Restore initial state.
+	 */
+	pci_write_config_word(dev, PCI_COMMAND, orig);
+
+	pci_cfg_access_unlock(dev);
+
+	if (new != toggle)
+		dev->broken_intx_masking = 1;
+}
+
+/**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *

@@ -1399,6 +1441,8 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	pci_test_intx_masking(dev);
+
 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)

include/linux/pci.h +11 -2

index 33c2b0b..8a307fc 100644
@@ -366,7 +366,7 @@ struct pci_dev {
 	unsigned int	is_thunderbolt:1; /* Thunderbolt controller */
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
-	unsigned int	broken_intx_masking:1;
+	unsigned int	broken_intx_masking:1; /* INTx masking can't be used */
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;

@@ -1003,6 +1003,16 @@ int __must_check pci_reenable_device(struct pci_dev *);
 int __must_check pcim_enable_device(struct pci_dev *pdev);
 void pcim_pin_device(struct pci_dev *pdev);
 
+static inline bool pci_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if device has not been marked
+	 * to have this feature broken and it has passed
+	 * pci_test_intx_masking() test.
+	 */
+	return !pdev->broken_intx_masking;
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);

@@ -1026,7 +1036,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
-bool pci_intx_mask_supported(struct pci_dev *dev);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
 int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask);

Signature · 1 line

2.1.4

The test for INTx masking via config space command performed in pci_intx_mask_supported() should be performed before PCI device can be used. This is to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register which may collide with MSI/MSI-X interrupts.

This patch moves test performed in pci_intx_mask_supported() to

static void pci_test_intx_masking(struct pci_dev *dev)

defined in drivers/pci/probe.c.

This function is called from pci_setup_device(). It skips the test if the device has been already marked to have broken INTx masking feature. Otherwise the test is executed and broken_intx_masking field of struct pci_dev is set accordingly. broken_intx_masking meaning is: if it is true then the test has been either skipped because the device has been already known to have broken INTx masking support, or the test's been done and it has detected INTx masking support to be broken.
The test result can be queried at any time later from the pci_dev using same interface as before (though whith changed implementation)

static inline bool pci_intx_mask_supported(struct pci_dev *pdev) {
/*
* INTx masking is supported if device has not been marked
* to have this feature broken and it has passed
* pci_test_intx_masking() test.
*/
return !pdev->broken_intx_masking;
}

so current users of pci_intx_mask_supported: uio and vfio, keep their code unchanged.

Signed-off-by Piotr Gregor <[email protected]>

---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 56 insertions(+), 43 deletions(-)

drivers/pci/pci.c +1 -41

index b01bd5b..7c4e1aa 100644
@@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-/**
- * pci_intx_mask_supported - probe for INTx masking support
- * @dev: the PCI device to operate on
- *
- * Check if the device dev support INTx masking via the config space
- * command word.
- */
-bool pci_intx_mask_supported(struct pci_dev *dev)
-{
-	bool mask_supported = false;
-	u16 orig, new;
-
-	if (dev->broken_intx_masking)
-		return false;
-
-	pci_cfg_access_lock(dev);
-
-	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(dev, PCI_COMMAND, &new);
-
-	/*
-	 * There's no way to protect against hardware bugs or detect them
-	 * reliably, but as long as we know what the value should be, let's
-	 * go ahead and check it.
-	 */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
-			orig, new);
-	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
-		mask_supported = true;
-		pci_write_config_word(dev, PCI_COMMAND, orig);
-	}
-
-	pci_cfg_access_unlock(dev);
-	return mask_supported;
-}
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
-
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
 	struct pci_bus *bus = dev->bus;

@@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
  * @dev: the PCI device to operate on
  *
  * Check if the device dev has its INTx line asserted, mask it and
- * return true in that case. False is returned if not interrupt was
+ * return true in that case. False is returned if no interrupt was
  * pending.
  */
 bool pci_check_and_mask_intx(struct pci_dev *dev)

drivers/pci/probe.c +9 -0

index 19c8950..ee6b55c 100644
@@ -1330,6 +1330,48 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 }
 
 /**
+ * pci_test_intx_masking - probe for INTx masking support
+ * @dev: the PCI device to operate on
+ *
+ * Check if the @dev supports INTx masking via the config space
+ * command word. Executed when PCI device is setup. Result is saved
+ * in broken_intx_masking field of struct pci_dev and can be checked
+ * with pci_intx_mask_supported at any time later, after the PCI device
+ * has been setup (this avoids testing of PCI_COMMAND_INTX_DISABLE
+ * register at runtime).

drivers/pci/probe.c +20 -0

+	/*
+	 * Perform the test.
+	 */
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	/*
+	 * Restore initial state.
+	 */
+	pci_write_config_word(dev, PCI_COMMAND, orig);
+
+	pci_cfg_access_unlock(dev);
+
+	if (new != toggle)
+		dev->broken_intx_masking = 1;
+}
+
+/**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *

drivers/pci/probe.c +0 -0

 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)

include/linux/pci.h +11 -1

index 33c2b0b..8a307fc 100644
@@ -366,7 +366,7 @@ struct pci_dev {
 	unsigned int	is_thunderbolt:1; /* Thunderbolt controller */
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
-	unsigned int	broken_intx_masking:1;
+	unsigned int	broken_intx_masking:1; /* INTx masking can't be used */
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;

@@ -1003,6 +1003,16 @@ int __must_check pci_reenable_device(struct pci_dev *);
 int __must_check pcim_enable_device(struct pci_dev *pdev);
 void pcim_pin_device(struct pci_dev *pdev);
 
+static inline bool pci_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if device has not been marked
+	 * to have this feature broken and it has passed
+	 * pci_test_intx_masking() test.
+	 */
+	return !pdev->broken_intx_masking;
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);

Quoted 119 lines · Piotr Gregor

The test for INTx masking via config space command performed in pci_intx_mask_supported() should be performed before PCI device can be used. This is to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register which may collide with MSI/MSI-X interrupts.

This patch moves test performed in pci_intx_mask_supported() to

static void pci_test_intx_masking(struct pci_dev *dev)

defined in drivers/pci/probe.c.

This function is called from pci_setup_device(). It skips the test if the device has been already marked to have broken INTx masking feature. Otherwise the test is executed and broken_intx_masking field of struct pci_dev is set accordingly. broken_intx_masking meaning is: if it is true then the test has been either skipped because the device has been already known to have broken INTx masking support, or the test's been done and it has detected INTx masking support to be broken.
The test result can be queried at any time later from the pci_dev using same interface as before (though whith changed implementation)

static inline bool pci_intx_mask_supported(struct pci_dev *pdev) {
/*
* INTx masking is supported if device has not been marked
* to have this feature broken and it has passed
* pci_test_intx_masking() test.
*/
return !pdev->broken_intx_masking;
}

so current users of pci_intx_mask_supported: uio and vfio, keep their code unchanged.

Signed-off-by Piotr Gregor <[email protected]>

---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 56 insertions(+), 43 deletions(-)

drivers/pci/pci.c +1 -41

index b01bd5b..7c4e1aa 100644
@@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-/**
- * pci_intx_mask_supported - probe for INTx masking support
- * @dev: the PCI device to operate on
- *
- * Check if the device dev support INTx masking via the config space
- * command word.
- */
-bool pci_intx_mask_supported(struct pci_dev *dev)
-{
-	bool mask_supported = false;
-	u16 orig, new;
-
-	if (dev->broken_intx_masking)
-		return false;
-
-	pci_cfg_access_lock(dev);
-
-	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(dev, PCI_COMMAND, &new);
-
-	/*
-	 * There's no way to protect against hardware bugs or detect them
-	 * reliably, but as long as we know what the value should be, let's
-	 * go ahead and check it.
-	 */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
-			orig, new);
-	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
-		mask_supported = true;
-		pci_write_config_word(dev, PCI_COMMAND, orig);
-	}
-
-	pci_cfg_access_unlock(dev);
-	return mask_supported;
-}
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
-
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
 	struct pci_bus *bus = dev->bus;

@@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
  * @dev: the PCI device to operate on
  *
  * Check if the device dev has its INTx line asserted, mask it and
- * return true in that case. False is returned if not interrupt was
+ * return true in that case. False is returned if no interrupt was
  * pending.
  */
 bool pci_check_and_mask_intx(struct pci_dev *dev)

drivers/pci/probe.c +9 -0

index 19c8950..ee6b55c 100644
@@ -1330,6 +1330,48 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 }
 
 /**
+ * pci_test_intx_masking - probe for INTx masking support
+ * @dev: the PCI device to operate on
+ *
+ * Check if the @dev supports INTx masking via the config space
+ * command word. Executed when PCI device is setup. Result is saved
+ * in broken_intx_masking field of struct pci_dev and can be checked
+ * with pci_intx_mask_supported at any time later, after the PCI device
+ * has been setup (this avoids testing of PCI_COMMAND_INTX_DISABLE
+ * register at runtime).

drivers/pci/probe.c static void pci_msi_setup_pci_dev(struct pci_dev *dev) · line 1351

Quoted 10 lines

drivers/pci/probe.c +10 -0

+ */
+static void pci_test_intx_masking(struct pci_dev *dev)
+{
+	u16 orig, toggle, new;
+
+	/*
+	 * If device doesn't support this feature though it could pass the test.
+	 */
+	if (dev->broken_intx_masking)
+		return;

I don't think we need this check anymore, because broken_intx_masking is only set by "FINAL" quirks, which run long after this code.

drivers/pci/probe.c static void pci_msi_setup_pci_dev(struct pci_dev *dev) · line 1353

Quoted 2 lines

drivers/pci/probe.c +2 -0

+
+	pci_cfg_access_lock(dev);

I don't think we need this locking either. This lock protects against user-space config access, and the device isn't accessible to user space yet.

Quoted 24 lines

drivers/pci/probe.c +20 -0

+	/*
+	 * Perform the test.
+	 */
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	/*
+	 * Restore initial state.
+	 */
+	pci_write_config_word(dev, PCI_COMMAND, orig);
+
+	pci_cfg_access_unlock(dev);
+
+	if (new != toggle)
+		dev->broken_intx_masking = 1;
+}
+
+/**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *

drivers/pci/probe.c int pci_setup_device(struct pci_dev *dev) · line 1444

Quoted 4 lines

drivers/pci/probe.c +1 -0

@@ -1399,6 +1441,8 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	pci_test_intx_masking(dev);

I think it's nicer to read the code if the assignment isn't hidden away, e.g., something like this:

  dev->broken_intx_masking = pci_test_intx_masking(dev);

That's more like what we do for dev->cfg_size and similar things.

I provisionally made these changes and attached the updated patch below. Let me know if it looks OK.

This is on my pci/enumeration branch, intended for v4.13.

Quoted 34 lines

drivers/pci/probe.c +0 -0

 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)

include/linux/pci.h +11 -1

index 33c2b0b..8a307fc 100644
@@ -366,7 +366,7 @@ struct pci_dev {
 	unsigned int	is_thunderbolt:1; /* Thunderbolt controller */
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
-	unsigned int	broken_intx_masking:1;
+	unsigned int	broken_intx_masking:1; /* INTx masking can't be used */
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;

@@ -1003,6 +1003,16 @@ int __must_check pci_reenable_device(struct pci_dev *);
 int __must_check pcim_enable_device(struct pci_dev *pdev);
 void pcim_pin_device(struct pci_dev *pdev);
 
+static inline bool pci_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if device has not been marked
+	 * to have this feature broken and it has passed
+	 * pci_test_intx_masking() test.
+	 */
+	return !pdev->broken_intx_masking;
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);

include/linux/pci.h int __must_check pci_set_mwi(struct pci_dev *dev); · line 1041

Quoted 7 lines

include/linux/pci.h +0 -1

@@ -1026,7 +1036,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
-bool pci_intx_mask_supported(struct pci_dev *dev);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
 int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask);

commit 73ab6bc006ba966e233785577a1742c6cace9c8a

Author: Piotr Gregor <[email protected]>
Date:   Fri May 26 22:02:25 2017 +0100

PCI: Test INTx masking during enumeration, not at run-time

The test for INTx masking via PCI_COMMAND_INTX_DISABLE performed in pci_intx_mask_supported() should be done before the device can be used. This is to avoid writing PCI_COMMAND while the driver owns the device, in case that has any effect on MSI/MSI-X interrupts.

Move the content of pci_intx_mask_supported() to pci_intx_mask_broken() and call it from pci_setup_device().

The test result can be queried at any time later using the same pci_intx_mask_supported() interface as before (though with changed implementation), so callers (uio, vfio) should be unaffected.

Signed-off-by Piotr Gregor <[email protected]>

[bhelgaas: changelog, remove quirk check, remove locking, move dev->broken_intx_masking assignment to caller]

Signed-off-by Bjorn Helgaas <[email protected]>

Reviewed-by Alex Williamson <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

drivers/pci/pci.c +1 -41

index b01bd5bba8e6..7c4e1aa67c67 100644
@@ -3708,46 +3708,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-/**
- * pci_intx_mask_supported - probe for INTx masking support
- * @dev: the PCI device to operate on
- *
- * Check if the device dev support INTx masking via the config space
- * command word.
- */
-bool pci_intx_mask_supported(struct pci_dev *dev)
-{
-	bool mask_supported = false;
-	u16 orig, new;
-
-	if (dev->broken_intx_masking)
-		return false;
-
-	pci_cfg_access_lock(dev);
-
-	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(dev, PCI_COMMAND, &new);
-
-	/*
-	 * There's no way to protect against hardware bugs or detect them
-	 * reliably, but as long as we know what the value should be, let's
-	 * go ahead and check it.
-	 */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
-			orig, new);
-	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
-		mask_supported = true;
-		pci_write_config_word(dev, PCI_COMMAND, orig);
-	}
-
-	pci_cfg_access_unlock(dev);
-	return mask_supported;
-}
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
-
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
 	struct pci_bus *bus = dev->bus;

@@ -3798,7 +3758,7 @@ static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
  * @dev: the PCI device to operate on
  *
  * Check if the device dev has its INTx line asserted, mask it and
- * return true in that case. False is returned if not interrupt was
+ * return true in that case. False is returned if no interrupt was
  * pending.
  */
 bool pci_check_and_mask_intx(struct pci_dev *dev)

drivers/pci/probe.c +30 -0

index 19c8950c6c38..28c57b31261c 100644
@@ -1329,6 +1329,34 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 		pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
 }
 
+/**
+ * pci_intx_mask_broken - test PCI_COMMAND_INTX_DISABLE writability
+ * @dev: PCI device
+ *
+ * Test whether PCI_COMMAND_INTX_DISABLE is writable for @dev.  Check this
+ * at enumeration-time to avoid modifying PCI_COMMAND at run-time.
+ */
+static int pci_intx_mask_broken(struct pci_dev *dev)
+{
+	u16 orig, toggle, new;
+
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	pci_write_config_word(dev, PCI_COMMAND, orig);
+
+	/*
+	 * PCI_COMMAND_INTX_DISABLE was reserved and read-only prior to PCI
+	 * r2.3, so strictly speaking, a device is not *broken* if it's not
+	 * writable.
+	 */
+	if (new == toggle)
+		return 1;
+	return 0;
+}
+
 /**
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill

@@ -1399,6 +1427,8 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	dev->broken_intx_masking = pci_intx_mask_broken(dev);
+
 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)

include/linux/pci.h +10 -2

index 33c2b0b77429..4f0613d5d2d9 100644
@@ -366,7 +366,7 @@ struct pci_dev {
 	unsigned int	is_thunderbolt:1; /* Thunderbolt controller */
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
-	unsigned int	broken_intx_masking:1;
+	unsigned int	broken_intx_masking:1; /* INTx masking can't be used */
 	unsigned int	io_window_1k:1;	/* Intel P2P bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;

@@ -1003,6 +1003,15 @@ int __must_check pci_reenable_device(struct pci_dev *);
 int __must_check pcim_enable_device(struct pci_dev *pdev);
 void pcim_pin_device(struct pci_dev *pdev);
 
+static inline bool pci_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if PCI_COMMAND_INTX_DISABLE is
+	 * writable and no quirk has marked the feature broken.
+	 */
+	return !pdev->broken_intx_masking;
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);

@@ -1026,7 +1035,6 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
-bool pci_intx_mask_supported(struct pci_dev *dev);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
 int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask);

From 6d814685b907980b4cae5c27c57fb4bdea13d5bb Mon Sep 17 00:00:00 2001

Hi Bjorn,

The pci_cfg_access_lock is most likely not needed there. The assignment by return type is indeed preferred in this case.

However, you have changed the meaning of returned boolean information by pci_intx_mask_broken leaving pci_intx_mask_supported unchanged. The test should be:

    if (new != toggle) /* the test failed */
	        return 1;
	return 0;

Regarding v2.3 - do you think it is worth to apply the check so we would have something like

    if ((new == toggle) || PCI_VERSION_PRIOR_TO_23) /* test OK or PCI prior to r2.3 */
	        return 0;
	return 1;

Quoted 5 lines · Piotr Gregor

Hi Bjorn,

The pci_cfg_access_lock is most likely not needed there. The assignment by return type is indeed preferred in this case.

Quoted 7 lines

However, you have changed the meaning of returned boolean information by pci_intx_mask_broken leaving pci_intx_mask_supported unchanged. The test should be:

    if (new != toggle) /* the test failed */
	        return 1;
	return 0;

Oh, you're absolutely right, thanks for catching that! I updated my pci/enumeration branch.

Quoted 6 lines

Regarding v2.3 - do you think it is worth to apply the check so we would have something like

    if ((new == toggle) || PCI_VERSION_PRIOR_TO_23) /* test OK or PCI prior to r2.3 */
	        return 0;
	return 1;

I'm not sure how to test for r2.3 compliance. But even if we could, I guess I think the current code is probably better because it actually checks the property we care about, not a spec revision that is one step removed from the property.

Bjorn