Re: [PATCH] pci: Handle the case when PCI_COMMAND register hasn't changed in INTx masking test

[Bjorn Helgaas]

[+cc Alex]

On Mon, May 22, 2017 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> Hi Piotr,
> 
> On Wed, May 10, 2017 at 01:30:01PM +0100, Piotr Gregor wrote:
> > The check for interrupt masking support is done by reading
> > the PCI_COMMAND config word
> > 
> > 	pci_read_config_word(dev, PCI_COMMAND, &orig);
> > 
> > then flipping the PCI_COMMAND_INTX_DISABLE bit and writing result back
> > 
> > 	pci_write_config_word(dev, PCI_COMMAND,
> > 		orig ^ PCI_COMMAND_INTX_DISABLE);
> > 
> > The expected result is that following read of the PCI_COMMAND
> > 
> > 	pci_read_config_word(dev, PCI_COMMAND, &new);
> > 
> > returns PCI_COMMAND with only PCI_COMMAND_INTX_DISABLE bit changed.
> > 
> > There are two possible outcomes:
> > 
> > 	1.	Command is the same (hasn't changed)
> > 	2.	Commmand has changed
> > 
> > And the second of them may be decoupled to:
> > 
> > 	2.1	Command changed only for PCI_COMMAND_INTX_DISABLE bit
> > 		(hasn't changed for bits different than PCI_COMMAND_INTX_DISABLE)
> > 	2.2	Command changed for bit(s) different than PCI_COMMAND_INTX_DISABLE bit
> > 		(and maybe for PCI_COMMAND_INTX_DISABLE too)
> > 
> > The 2.1 is expected result and anything else is error.
> > The 2.2 outcome is handled by pci_intx_mask_supported by printing
> > appropriate message to the log, but outcome 1 is not handled.
> > 
> > Log the message about command not being changed at all.
> > 
> > Signed-off-by: Piotr Gregor <piotrgregor@xxxxxxxxxxx>
> > ---
> >  drivers/pci/pci.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b01bd5b..67a611e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3712,7 +3712,7 @@ 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
> > + * Check if the device dev supports INTx masking via the config space
> >   * command word.
> >   */
> >  bool pci_intx_mask_supported(struct pci_dev *dev)
> > @@ -3736,11 +3736,21 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
> >  	 * go ahead and check it.
> >  	 */
> >  	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
> > +		/*
> > +		 * If anything else than PCI_COMMAND_INTX_DISABLE bit has
> > +		 * changed (and maybe PCI_COMMAND_INTX_DISABLE too)
> > +		 */
> >  		dev_err(&dev->dev, "Command register changed from 0x%x to 0x%x: driver or hardware bug?\n",
> >  			orig, new);
> >  	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
> > +		/*
> > +		 * OK. Only PCI_COMMAND_INTX_DISABLE bit has changed
> > +		 */
> >  		mask_supported = true;
> >  		pci_write_config_word(dev, PCI_COMMAND, orig);
> > +	} else {
> > +		dev_err(&dev->dev, "Command register hasn't changed when written from 0x%x to 0x%x: driver or hardware bug?\n",
> > +			orig, new);
> 
> Bit 10 was a reserved bit in PCI r2.2 and was defined as
> PCI_COMMAND_INTX_DISABLE in r2.3 of the spec, so I don't think we
> should treat this as an error.

If we make a change here, I think it should be simplified to look like
this:

  pci_cfg_access_lock(dev);

  pci_read_config_word(dev, PCI_COMMAND, &orig);
  toggle = orig ^ PCI_COMMAND_INTX_DISABLE;
  pci_write_config_word(dev, PCI_COMMAND, toggle);
  pci_read_config_word(dev, PCI_COMMAND, &new);
  pci_write_config_word(dev, PCI_COMMAND, orig);

  pci_cfg_access_unlock(dev);

  if (new == toggle)
    return true;

  return false;

I don't really see the value in cluttering the code to check for
things that "can't happen."  There's an infinite amount of that sort
of stuff.  If we know about possible issues, that's one thing, but
this seems like just looking for trouble.

It makes me a little nervous to have this interface that toggles
PCI_COMMAND_INTX_DISABLE at run-time.  This could be called after a
driver has set up interrupts, and I think there are some interactions
between INTx and MSI/MSI-X that might cause unexpected things to
happen if we toggle PCI_COMMAND_INTX_DISABLE.

I'd rather have the core check it once during enumeration (before we
give it to a driver and before any interrupts are configured) and save
the result in struct pci_dev.

> >  	}
> >  
> >  	pci_cfg_access_unlock(dev);
> > @@ -3798,7 +3808,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 interrupt wasn't
> >   * pending.

We should definitely fix this typo.  Maybe "if no interrupt was
pending"?

> >   */
> >  bool pci_check_and_mask_intx(struct pci_dev *dev)
> > -- 
> > 2.1.4
> >