[PATCH] PCI: Move test of INTx masking to pci_setup_device

[PATCH] PCI: Move test of INTx masking to pci_setup_device

[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 simplifies test performed in pci_intx_mask_supported
and introduces intx_mask_support field in struct pci_dev to store
the result. The test itself is moved to pci_setup_device.
The result can be queried at any time later from the pci_dev with

static inline bool pci_is_intx_mask_supported(struct pci_dev *pdev)
{
        /*
         * INTx masking is supported if device passed INTx test
         * and it's INTx masking feature works properly.
         */
        return (pdev->intx_mask_support && !pdev->broken_intx_masking);
}

Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
---
 drivers/pci/pci.c             | 42 +++++++++++++++++++++++-------------------
 drivers/pci/probe.c           |  3 +++
 drivers/uio/uio_pci_generic.c |  2 +-
 drivers/vfio/pci/vfio_pci.c   |  2 +-
 include/linux/pci.h           | 10 ++++++++++
 5 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..bcaab9b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3709,42 +3709,46 @@ void pci_intx(struct pci_dev *pdev, int enable)
 EXPORT_SYMBOL_GPL(pci_intx);
 
 /**
- * pci_intx_mask_supported - probe for INTx masking support
+ * pci_intx_mask_supported - probe for INTx masking support in pci_setup_device
  * @dev: the PCI device to operate on
  *
- * Check if the device dev support INTx masking via the config space
- * command word.
+ * Check if the device dev supports INTx masking via the config space
+ * command word. Executed when PCI device is setup. Result is saved
+ * in intx_mask_support field of pci_dev and should be checked
+ * with pci_is_intx_mask_supported() after PCI device has been setup
+ * to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register.
  */
 bool pci_intx_mask_supported(struct pci_dev *dev)
 {
-	bool mask_supported = false;
-	u16 orig, new;
+	u16 orig, toggle, new;
 
+	/*
+	 * If device doesn't support this feature though it could pass the test.
+	 */
 	if (dev->broken_intx_masking)
 		return false;
 
 	pci_cfg_access_lock(dev);
 
+	/*
+	 * Perform the test.
+	 */
 	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
 	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.
+	 * Restore initial state.
 	 */
-	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_write_config_word(dev, PCI_COMMAND, orig);
 
 	pci_cfg_access_unlock(dev);
-	return mask_supported;
+
+	if (new == toggle)
+		return true;
+
+	return false;
 }
 EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
 
@@ -3798,7 +3802,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)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..16c60ce 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1399,6 +1399,9 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	if (pci_intx_mask_supported(dev))
+		dev->intx_mask_support = 1;
+
 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index d0b508b..8cd443c 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -73,7 +73,7 @@ static int probe(struct pci_dev *pdev,
 		return -ENODEV;
 	}
 
-	if (!pci_intx_mask_supported(pdev)) {
+	if (!pci_is_intx_mask_supported(pdev)) {
 		err = -ENODEV;
 		goto err_verify;
 	}
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 324c52e..933456e 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -239,7 +239,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
 			vdev->nointx = true;
 			pci_intx(pdev, 0);
 		} else
-			vdev->pci_2_3 = pci_intx_mask_supported(pdev);
+			vdev->pci_2_3 = pci_is_intx_mask_supported(pdev);
 	}
 
 	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b..0fbd278 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -367,6 +367,7 @@ struct pci_dev {
 	unsigned int    __aer_firmware_first_valid:1;
 	unsigned int	__aer_firmware_first:1;
 	unsigned int	broken_intx_masking:1;
+	unsigned int	intx_mask_support:1;	/* INTx masking is supported */
 	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 +1004,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_is_intx_mask_supported(struct pci_dev *pdev)
+{
+	/*
+	 * INTx masking is supported if device passed INTx test and it's INTx
+	 * masking feature works properly.
+	 */
+	return (pdev->intx_mask_support && !pdev->broken_intx_masking);
+}
+
 static inline int pci_is_enabled(struct pci_dev *pdev)
 {
 	return (atomic_read(&pdev->enable_cnt) > 0);
-- 
2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Michael S. Tsirkin]

On Thu, May 25, 2017 at 07:13:23PM +0100, Piotr Gregor wrote:
> 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 simplifies test performed in pci_intx_mask_supported
> and introduces intx_mask_support field in struct pci_dev to store
> the result. The test itself is moved to pci_setup_device.
> The result can be queried at any time later from the pci_dev with
> 
> static inline bool pci_is_intx_mask_supported(struct pci_dev *pdev)
> {
>         /*
>          * INTx masking is supported if device passed INTx test
>          * and it's INTx masking feature works properly.
>          */
>         return (pdev->intx_mask_support && !pdev->broken_intx_masking);
> }
> 
> Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
> ---
>  drivers/pci/pci.c             | 42 +++++++++++++++++++++++-------------------
>  drivers/pci/probe.c           |  3 +++
>  drivers/uio/uio_pci_generic.c |  2 +-
>  drivers/vfio/pci/vfio_pci.c   |  2 +-
>  include/linux/pci.h           | 10 ++++++++++
>  5 files changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..bcaab9b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3709,42 +3709,46 @@ void pci_intx(struct pci_dev *pdev, int enable)
>  EXPORT_SYMBOL_GPL(pci_intx);
>  
>  /**
> - * pci_intx_mask_supported - probe for INTx masking support
> + * pci_intx_mask_supported - probe for INTx masking support in pci_setup_device
>   * @dev: the PCI device to operate on
>   *
> - * Check if the device dev support INTx masking via the config space
> - * command word.
> + * Check if the device dev supports INTx masking via the config space
> + * command word. Executed when PCI device is setup. Result is saved
> + * in intx_mask_support field of pci_dev and should be checked
> + * with pci_is_intx_mask_supported() after PCI device has been setup
> + * to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register.
>   */
>  bool pci_intx_mask_supported(struct pci_dev *dev)
>  {
> -	bool mask_supported = false;
> -	u16 orig, new;
> +	u16 orig, toggle, new;
>  
> +	/*
> +	 * If device doesn't support this feature though it could pass the test.
> +	 */
>  	if (dev->broken_intx_masking)
>  		return false;
>  
>  	pci_cfg_access_lock(dev);
>  
> +	/*
> +	 * Perform the test.
> +	 */
>  	pci_read_config_word(dev, PCI_COMMAND, &orig);
> -	pci_write_config_word(dev, PCI_COMMAND,
> -			      orig ^ PCI_COMMAND_INTX_DISABLE);
> +	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
> +	pci_write_config_word(dev, PCI_COMMAND, toggle);
>  	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.
> +	 * Restore initial state.
>  	 */
> -	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_write_config_word(dev, PCI_COMMAND, orig);
>  
>  	pci_cfg_access_unlock(dev);
> -	return mask_supported;
> +
> +	if (new == toggle)
> +		return true;
> +
> +	return false;
>  }
>  EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
>  
> @@ -3798,7 +3802,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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..16c60ce 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1399,6 +1399,9 @@ int pci_setup_device(struct pci_dev *dev)
>  		}
>  	}
>  
> +	if (pci_intx_mask_supported(dev))
> +		dev->intx_mask_support = 1;
> +
>  	switch (dev->hdr_type) {		    /* header type */
>  	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
>  		if (class == PCI_CLASS_BRIDGE_PCI)
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index d0b508b..8cd443c 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -73,7 +73,7 @@ static int probe(struct pci_dev *pdev,
>  		return -ENODEV;
>  	}
>  
> -	if (!pci_intx_mask_supported(pdev)) {
> +	if (!pci_is_intx_mask_supported(pdev)) {
>  		err = -ENODEV;
>  		goto err_verify;
>  	}
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 324c52e..933456e 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -239,7 +239,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
>  			vdev->nointx = true;
>  			pci_intx(pdev, 0);
>  		} else
> -			vdev->pci_2_3 = pci_intx_mask_supported(pdev);
> +			vdev->pci_2_3 = pci_is_intx_mask_supported(pdev);
>  	}
>  
>  	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..0fbd278 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -367,6 +367,7 @@ struct pci_dev {
>  	unsigned int    __aer_firmware_first_valid:1;
>  	unsigned int	__aer_firmware_first:1;
>  	unsigned int	broken_intx_masking:1;
> +	unsigned int	intx_mask_support:1;	/* INTx masking is supported */
>  	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 +1004,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_is_intx_mask_supported(struct pci_dev *pdev)
> +{
> +	/*
> +	 * INTx masking is supported if device passed INTx test and it's INTx
> +	 * masking feature works properly.
> +	 */
> +	return (pdev->intx_mask_support && !pdev->broken_intx_masking);
> +}
> +

And now there's pci_is_intx_mask_supported and pci_intx_mask_supported
which are the same except pci_intx_mask_supported shouldn't be called by
anyone except in one place in pci code.

Pls don't do this. Rename pci_intx_mask_supported -> __pci_intx_mask_supported
and then everyone can just keep calling pci_intx_mask_supported as previously,
except it'll be faster and safer. And unexport
__pci_intx_mask_supported.


>  static inline int pci_is_enabled(struct pci_dev *pdev)
>  {
>  	return (atomic_read(&pdev->enable_cnt) > 0);
> -- 
> 2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Alex Williamson]

On Thu, 25 May 2017 21:22:01 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, May 25, 2017 at 07:13:23PM +0100, Piotr Gregor wrote:
> > 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 simplifies test performed in pci_intx_mask_supported
> > and introduces intx_mask_support field in struct pci_dev to store
> > the result. The test itself is moved to pci_setup_device.
> > The result can be queried at any time later from the pci_dev with
> > 
> > static inline bool pci_is_intx_mask_supported(struct pci_dev *pdev)
> > {
> >         /*
> >          * INTx masking is supported if device passed INTx test
> >          * and it's INTx masking feature works properly.
> >          */
> >         return (pdev->intx_mask_support && !pdev->broken_intx_masking);
> > }
> > 
> > Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
> > ---
> >  drivers/pci/pci.c             | 42 +++++++++++++++++++++++-------------------
> >  drivers/pci/probe.c           |  3 +++
> >  drivers/uio/uio_pci_generic.c |  2 +-
> >  drivers/vfio/pci/vfio_pci.c   |  2 +-
> >  include/linux/pci.h           | 10 ++++++++++
> >  5 files changed, 38 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b01bd5b..bcaab9b 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3709,42 +3709,46 @@ void pci_intx(struct pci_dev *pdev, int enable)
> >  EXPORT_SYMBOL_GPL(pci_intx);
> >  
> >  /**
> > - * pci_intx_mask_supported - probe for INTx masking support
> > + * pci_intx_mask_supported - probe for INTx masking support in pci_setup_device
> >   * @dev: the PCI device to operate on
> >   *
> > - * Check if the device dev support INTx masking via the config space
> > - * command word.
> > + * Check if the device dev supports INTx masking via the config space
> > + * command word. Executed when PCI device is setup. Result is saved
> > + * in intx_mask_support field of pci_dev and should be checked
> > + * with pci_is_intx_mask_supported() after PCI device has been setup
> > + * to avoid reading/writing of PCI_COMMAND_INTX_DISABLE register.
> >   */
> >  bool pci_intx_mask_supported(struct pci_dev *dev)
> >  {
> > -	bool mask_supported = false;
> > -	u16 orig, new;
> > +	u16 orig, toggle, new;
> >  
> > +	/*
> > +	 * If device doesn't support this feature though it could pass the test.
> > +	 */
> >  	if (dev->broken_intx_masking)
> >  		return false;
> >  
> >  	pci_cfg_access_lock(dev);
> >  
> > +	/*
> > +	 * Perform the test.
> > +	 */
> >  	pci_read_config_word(dev, PCI_COMMAND, &orig);
> > -	pci_write_config_word(dev, PCI_COMMAND,
> > -			      orig ^ PCI_COMMAND_INTX_DISABLE);
> > +	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
> > +	pci_write_config_word(dev, PCI_COMMAND, toggle);
> >  	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.
> > +	 * Restore initial state.
> >  	 */
> > -	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_write_config_word(dev, PCI_COMMAND, orig);
> >  
> >  	pci_cfg_access_unlock(dev);
> > -	return mask_supported;
> > +
> > +	if (new == toggle)
> > +		return true;
> > +
> > +	return false;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
> >  
> > @@ -3798,7 +3802,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)
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 19c8950..16c60ce 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1399,6 +1399,9 @@ int pci_setup_device(struct pci_dev *dev)
> >  		}
> >  	}
> >  
> > +	if (pci_intx_mask_supported(dev))
> > +		dev->intx_mask_support = 1;
> > +
> >  	switch (dev->hdr_type) {		    /* header type */
> >  	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
> >  		if (class == PCI_CLASS_BRIDGE_PCI)
> > diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> > index d0b508b..8cd443c 100644
> > --- a/drivers/uio/uio_pci_generic.c
> > +++ b/drivers/uio/uio_pci_generic.c
> > @@ -73,7 +73,7 @@ static int probe(struct pci_dev *pdev,
> >  		return -ENODEV;
> >  	}
> >  
> > -	if (!pci_intx_mask_supported(pdev)) {
> > +	if (!pci_is_intx_mask_supported(pdev)) {
> >  		err = -ENODEV;
> >  		goto err_verify;
> >  	}
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index 324c52e..933456e 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -239,7 +239,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> >  			vdev->nointx = true;
> >  			pci_intx(pdev, 0);
> >  		} else
> > -			vdev->pci_2_3 = pci_intx_mask_supported(pdev);
> > +			vdev->pci_2_3 = pci_is_intx_mask_supported(pdev);
> >  	}
> >  
> >  	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 33c2b0b..0fbd278 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -367,6 +367,7 @@ struct pci_dev {
> >  	unsigned int    __aer_firmware_first_valid:1;
> >  	unsigned int	__aer_firmware_first:1;
> >  	unsigned int	broken_intx_masking:1;
> > +	unsigned int	intx_mask_support:1;	/* INTx masking is supported */
> >  	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 +1004,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_is_intx_mask_supported(struct pci_dev *pdev)
> > +{
> > +	/*
> > +	 * INTx masking is supported if device passed INTx test and it's INTx
> > +	 * masking feature works properly.
> > +	 */
> > +	return (pdev->intx_mask_support && !pdev->broken_intx_masking);
> > +}
> > +  
> 
> And now there's pci_is_intx_mask_supported and pci_intx_mask_supported
> which are the same except pci_intx_mask_supported shouldn't be called by
> anyone except in one place in pci code.
> 
> Pls don't do this. Rename pci_intx_mask_supported -> __pci_intx_mask_supported
> and then everyone can just keep calling pci_intx_mask_supported as previously,
> except it'll be faster and safer. And unexport
> __pci_intx_mask_supported.


I would additionally suggest not to have pci_dev.broken_intx_masking
and pci_dev.intx_masking_support.  Let's just use broken_intx_masking to
track either broken or unsupported.  Then pci_intx_mask_supported()
becomes simply "return !pdev->broken_intx_masking;".  Of course we skip
testing of devices that are already marked by quirks as broken.  Thanks,

Alex

> >  static inline int pci_is_enabled(struct pci_dev *pdev)
> >  {
> >  	return (atomic_read(&pdev->enable_cnt) > 0);
> > -- 
> > 2.1.4  

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Piotr Gregor]

Very good suggestions. Will be reflected in changed patch.

[PATCH] PCI: Move test of INTx masking to pci_setup_device

[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 __pci_intx_mask_supported and unexports the former.
The result of INTx masking test is saved in broken_intx_masking
field of struct pci_dev (which now has extended meaning: if true
then the test has failed or the feature is not supported).
The __pci_intx_mask_supported test is moved to pci_setup_device.
The 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 passed INTx test
         * and it's INTx masking feature works properly.
         */
        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 <piotrgregor@rsyncme.org>
---
 drivers/pci/pci.c   | 45 ++++++++++++++++++++++++---------------------
 drivers/pci/probe.c |  3 +++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..8d5628e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3709,44 +3709,47 @@ void pci_intx(struct pci_dev *pdev, int enable)
 EXPORT_SYMBOL_GPL(pci_intx);
 
 /**
- * pci_intx_mask_supported - probe for INTx masking support
+ * pci_intx_mask_supported - probe for INTx masking support in pci_setup_device
  * @dev: the PCI device to operate on
  *
- * Check if the device dev support INTx masking via the config space
- * command word.
+ * Check if the device 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 pci_dev and can be checked
+ * with pci_intx_mask_supported after PCI device has been setup
+ * (avoids testing of PCI_COMMAND_INTX_DISABLE register at runtime).
  */
-bool pci_intx_mask_supported(struct pci_dev *dev)
+bool __pci_intx_mask_supported(struct pci_dev *dev)
 {
-	bool mask_supported = false;
-	u16 orig, new;
+	u16 orig, toggle, new;
 
+	/*
+	 * If device doesn't support this feature though it could pass the test.
+	 */
 	if (dev->broken_intx_masking)
 		return false;
 
 	pci_cfg_access_lock(dev);
 
+	/*
+	 * Perform the test.
+	 */
 	pci_read_config_word(dev, PCI_COMMAND, &orig);
-	pci_write_config_word(dev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(dev, PCI_COMMAND, toggle);
 	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.
+	 * Restore initial state.
 	 */
-	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_write_config_word(dev, PCI_COMMAND, orig);
 
 	pci_cfg_access_unlock(dev);
-	return mask_supported;
+
+	if (new == toggle)
+		return true;
+
+	return false;
 }
-EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
 
 static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
 {
@@ -3798,7 +3801,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)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..b343b14 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1399,6 +1399,9 @@ int pci_setup_device(struct pci_dev *dev)
 		}
 	}
 
+	if (!dev->broken_intx_masking && !__pci_intx_mask_supported(dev))
+		dev->broken_intx_masking = 1;
+
 	switch (dev->hdr_type) {		    /* header type */
 	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
 		if (class == PCI_CLASS_BRIDGE_PCI)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b..58c6fe3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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 device passed INTx test and it's INTx
+	 * masking feature works properly.
+	 */
+	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,7 @@ 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_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);
-- 
2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Alex Williamson]

On Thu, 25 May 2017 22:32:25 +0100
Piotr Gregor <piotrgregor@rsyncme.org> wrote:

> 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 __pci_intx_mask_supported and unexports the former.
> The result of INTx masking test is saved in broken_intx_masking
> field of struct pci_dev (which now has extended meaning: if true
> then the test has failed or the feature is not supported).
> The __pci_intx_mask_supported test is moved to pci_setup_device.
> The 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 passed INTx test
>          * and it's INTx masking feature works properly.
>          */
>         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 <piotrgregor@rsyncme.org>
> ---
>  drivers/pci/pci.c   | 45 ++++++++++++++++++++++++---------------------
>  drivers/pci/probe.c |  3 +++
>  include/linux/pci.h | 13 +++++++++++--
>  3 files changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..8d5628e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3709,44 +3709,47 @@ void pci_intx(struct pci_dev *pdev, int enable)
>  EXPORT_SYMBOL_GPL(pci_intx);
>  
>  /**
> - * pci_intx_mask_supported - probe for INTx masking support
> + * pci_intx_mask_supported - probe for INTx masking support in pci_setup_device
>   * @dev: the PCI device to operate on
>   *
> - * Check if the device dev support INTx masking via the config space
> - * command word.
> + * Check if the device 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 pci_dev and can be checked
> + * with pci_intx_mask_supported after PCI device has been setup
> + * (avoids testing of PCI_COMMAND_INTX_DISABLE register at runtime).
>   */
> -bool pci_intx_mask_supported(struct pci_dev *dev)
> +bool __pci_intx_mask_supported(struct pci_dev *dev)
>  {
> -	bool mask_supported = false;
> -	u16 orig, new;
> +	u16 orig, toggle, new;
>  
> +	/*
> +	 * If device doesn't support this feature though it could pass the test.
> +	 */
>  	if (dev->broken_intx_masking)
>  		return false;
>  
>  	pci_cfg_access_lock(dev);
>  
> +	/*
> +	 * Perform the test.
> +	 */
>  	pci_read_config_word(dev, PCI_COMMAND, &orig);
> -	pci_write_config_word(dev, PCI_COMMAND,
> -			      orig ^ PCI_COMMAND_INTX_DISABLE);
> +	toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
> +	pci_write_config_word(dev, PCI_COMMAND, toggle);
>  	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.
> +	 * Restore initial state.
>  	 */
> -	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_write_config_word(dev, PCI_COMMAND, orig);
>  
>  	pci_cfg_access_unlock(dev);
> -	return mask_supported;
> +
> +	if (new == toggle)
> +		return true;
> +
> +	return false;
>  }
> -EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
>  
>  static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
>  {
> @@ -3798,7 +3801,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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..b343b14 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1399,6 +1399,9 @@ int pci_setup_device(struct pci_dev *dev)
>  		}
>  	}
>  
> +	if (!dev->broken_intx_masking && !__pci_intx_mask_supported(dev))
> +		dev->broken_intx_masking = 1;

This is still harder than it needs to be, we could just name the
function pci_test_intx_masking() and let it set broken_intx_masking
directly, returning immediately if already set via a quirk.  Also,
shouldn't we move the test function to this file and make it static?
Thanks,

Alex

> +
>  	switch (dev->hdr_type) {		    /* header type */
>  	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
>  		if (class == PCI_CLASS_BRIDGE_PCI)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..58c6fe3 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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 device passed INTx test and it's INTx
> +	 * masking feature works properly.
> +	 */
> +	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,7 @@ 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_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);

[PATCH] PCI: Move test of INTx masking to pci_setup_device

[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 <piotrgregor@rsyncme.org>
---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h | 13 +++++++++++--
 3 files changed, 56 insertions(+), 43 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..7c4e1aa 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..ee6b55c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b..8a307fc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
-- 
2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Alex Williamson]

On Fri, 26 May 2017 22:02:25 +0100
Piotr Gregor <piotrgregor@rsyncme.org> wrote:

> 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 <piotrgregor@rsyncme.org>
> ---
>  drivers/pci/pci.c   | 42 +-----------------------------------------
>  drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h | 13 +++++++++++--
>  3 files changed, 56 insertions(+), 43 deletions(-)
 
Looks reasonable to me.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>


> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..7c4e1aa 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..ee6b55c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..8a307fc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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);

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Michael S. Tsirkin]

On Fri, May 26, 2017 at 10:02:25PM +0100, Piotr Gregor wrote:
> 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 <piotrgregor@rsyncme.org>

Looks sane

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  drivers/pci/pci.c   | 42 +-----------------------------------------
>  drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h | 13 +++++++++++--
>  3 files changed, 56 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..7c4e1aa 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..ee6b55c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..8a307fc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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);
> -- 
> 2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Bjorn Helgaas]

On Fri, May 26, 2017 at 10:02:25PM +0100, Piotr Gregor wrote:
> 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 <piotrgregor@rsyncme.org>
> ---
>  drivers/pci/pci.c   | 42 +-----------------------------------------
>  drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h | 13 +++++++++++--
>  3 files changed, 56 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..7c4e1aa 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950..ee6b55c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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;

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.

> +
> +	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.

> +	/*
> +	 * 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);

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.

>  	switch (dev->hdr_type) {		    /* header type */
>  	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
>  		if (class == PCI_CLASS_BRIDGE_PCI)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..8a307fc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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);


commit 73ab6bc006ba966e233785577a1742c6cace9c8a
Author: Piotr Gregor <piotrgregor@rsyncme.org>
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 <piotrgregor@rsyncme.org>
    [bhelgaas: changelog, remove quirk check, remove locking, move
    dev->broken_intx_masking assignment to caller]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
    Acked-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5bba8e6..7c4e1aa67c67 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950c6c38..28c57b31261c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b77429..4f0613d5d2d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Piotr Gregor]

[-- Attachment #1: Type: text/plain, Size: 15696 bytes --]

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;

I have attached patch (created on pci/enumeration branch,
should I commit this to your repo too?).

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;

cheers,
Piotr


On Fri, Jun 16, 2017 at 05:54:46PM -0500, Bjorn Helgaas wrote:
> On Fri, May 26, 2017 at 10:02:25PM +0100, Piotr Gregor wrote:
> > 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 <piotrgregor@rsyncme.org>
> > ---
> >  drivers/pci/pci.c   | 42 +-----------------------------------------
> >  drivers/pci/probe.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h | 13 +++++++++++--
> >  3 files changed, 56 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b01bd5b..7c4e1aa 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -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)
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 19c8950..ee6b55c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -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;
> 
> 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.
> 
> > +
> > +	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.
> 
> > +	/*
> > +	 * 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);
> 
> 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.
> 
> >  	switch (dev->hdr_type) {		    /* header type */
> >  	case PCI_HEADER_TYPE_NORMAL:		    /* standard header */
> >  		if (class == PCI_CLASS_BRIDGE_PCI)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 33c2b0b..8a307fc 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -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);
> 
> 
> commit 73ab6bc006ba966e233785577a1742c6cace9c8a
> Author: Piotr Gregor <piotrgregor@rsyncme.org>
> 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 <piotrgregor@rsyncme.org>
>     [bhelgaas: changelog, remove quirk check, remove locking, move
>     dev->broken_intx_masking assignment to caller]
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>     Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5bba8e6..7c4e1aa67c67 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 19c8950c6c38..28c57b31261c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b77429..4f0613d5d2d9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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);

[-- Attachment #2: 0001-PCI-Test-INTx-masking-during-enumeration-not-at-run-.patch --]
[-- Type: text/x-diff, Size: 6206 bytes --]

>From 6d814685b907980b4cae5c27c57fb4bdea13d5bb Mon Sep 17 00:00:00 2001
From: Piotr Gregor <piotrgregor@rsyncme.org>
Date: Fri, 26 May 2017 22:02:25 +0100
Subject: [PATCH] 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 <piotrgregor@rsyncme.org>
[bhelgaas: changelog, remove quirk check, remove locking, move
dev->broken_intx_masking assignment to caller]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/pci/pci.c   | 42 +-----------------------------------------
 drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++
 include/linux/pci.h | 12 ++++++++++--
 3 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..7c4e1aa 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..e2c2650 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1330,6 +1330,34 @@ static void pci_msi_setup_pci_dev(struct pci_dev *dev)
 }
 
 /**
+ * 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)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b..4f0613d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
-- 
2.1.4

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Bjorn Helgaas]

On Sat, Jun 17, 2017 at 06:45:44PM +0100, Piotr Gregor wrote:
> 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;

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

> 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

Re: [PATCH] PCI: Move test of INTx masking to pci_setup_device

[Piotr Gregor]

On Sat, Jun 17, 2017 at 08:14:33PM -0500, Bjorn Helgaas wrote:
> On Sat, Jun 17, 2017 at 06:45:44PM +0100, Piotr Gregor wrote:
> > 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;
> 
> Oh, you're absolutely right, thanks for catching that!  I updated my
> pci/enumeration branch.
> 
> > 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

Hi Bjorn,

You are right, having

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

would be incorrect, as if new != toggle then PCI_COMMAND_INTX_DISABLE is not writable
so INTx masking support should be considered broken (regardless of PCI version).

Piotr