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

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

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

[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]

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 
> > ---
> >  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, );
> > -   pci_write_config_word(dev, PCI_COMMAND,
> > - orig ^ PCI_COMMAND_INTX_DISABLE);
> > -   pci_read_config_word(dev, PCI_COMMAND, );
> > -
> > -   /*
> > -* 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, "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 

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

[Piotr Gregor]

Hi Bjorn,

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

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 
> > ---
> >  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, );
> > -   pci_write_config_word(dev, PCI_COMMAND,
> > - orig ^ PCI_COMMAND_INTX_DISABLE);
> > -   pci_read_config_word(dev, PCI_COMMAND, );
> > -
> > -   /*
> > -* 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, "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 

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 
> ---
>  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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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 

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 
> ---
>  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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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)
> + 

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 

Looks sane

Acked-by: Michael S. Tsirkin 


> ---
>  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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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 

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 

Looks sane

Acked-by: Michael S. Tsirkin 


> ---
>  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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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.
> +  */
> + 

[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 
---
 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, );
-   pci_write_config_word(dev, PCI_COMMAND,
- orig ^ PCI_COMMAND_INTX_DISABLE);
-   pci_read_config_word(dev, PCI_COMMAND, );
-
-   /*
-* 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, "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, );
+   toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+   pci_write_config_word(dev, 

[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 
---
 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, );
-   pci_write_config_word(dev, PCI_COMMAND,
- orig ^ PCI_COMMAND_INTX_DISABLE);
-   pci_read_config_word(dev, PCI_COMMAND, );
-
-   /*
-* 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, "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, );
+   toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
+   pci_write_config_word(dev, PCI_COMMAND, toggle);
+   

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


> 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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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 

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


> 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, );
> - pci_write_config_word(dev, PCI_COMMAND,
> -   orig ^ PCI_COMMAND_INTX_DISABLE);
> - pci_read_config_word(dev, PCI_COMMAND, );
> -
> - /*
> -  * 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, "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.
> +  */
> + 

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

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  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 
> ---
>  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, );
> - 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, );
>  
>   /*
> -  * 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, "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() 

[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 
---
 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, );
-   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, );
 
/*
-* 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, "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
--- 

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

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

[Piotr Gregor]

Very good suggestions. Will be reflected in changed patch.

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

[Piotr Gregor]

Very good suggestions. Will be reflected in changed patch.

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"  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 
> > ---
> >  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, );
> > -   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, );
> >  
> > /*
> > -* 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, "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 == 

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

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

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

[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 
---
 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, );
-   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, );
 
/*
-* 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, "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 

[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 
---
 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, );
-   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, );
 
/*
-* 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, "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