| Message ID | 20170510122959.4uupohiz3ukzm4c4@debian1.home |
|---|---|
| State | Not Applicable |
| Headers | show |
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@rsyncme.org> > --- > 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. > } > > 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. > */ > bool pci_check_and_mask_intx(struct pci_dev *dev) > -- > 2.1.4 >
[+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@rsyncme.org> > > --- > > 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 > >
On Tue, 23 May 2017 11:19:29 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > [+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@rsyncme.org> > > > --- > > > 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. Agreed, the only reason I can think that we'd ever want to point this out as a hardware bug would be if the device is PCI-express or if we can find other evidence in config space that the device should be compliant to the PCI 2.3 spec. Also, dev_err is sort of justified in the original error case because something unexpected happened. The register is being poked from somewhere else or changed spontaneously. Not supporting INTx masking is a completely valid state for hardware, so I'd at best consider it a dev_dbg level test. > > 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. I can imagine it being useful to allow a user to have some visibility to this property for a device, but a printk doesn't feel like a useful way to do that. I don't know if it's worth a sysfs attribute though. > 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. I think that would be fine, the only users are uio and vfio, the former testing it in the probe function, the latter testing it before giving the user access to the device (and therefore before interrupts are configured). Thanks, Alex > > > > } > > > > > > 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 > > >
Would the pci_setup_device() be a good place to move this check into? On Tue, May 23, 2017 at 5:39 PM, Alex Williamson <alex.williamson@redhat.com> wrote: > On Tue, 23 May 2017 11:19:29 -0500 > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> [+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@rsyncme.org> >> > > --- >> > > 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. > > Agreed, the only reason I can think that we'd ever want to point this > out as a hardware bug would be if the device is PCI-express or if we > can find other evidence in config space that the device should be > compliant to the PCI 2.3 spec. Also, dev_err is sort of justified in > the original error case because something unexpected happened. The > register is being poked from somewhere else or changed spontaneously. > Not supporting INTx masking is a completely valid state for hardware, > so I'd at best consider it a dev_dbg level test. > >> >> 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. > > I can imagine it being useful to allow a user to have some visibility > to this property for a device, but a printk doesn't feel like a useful > way to do that. I don't know if it's worth a sysfs attribute though. > >> 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. > > I think that would be fine, the only users are uio and vfio, the former > testing it in the probe function, the latter testing it before giving > the user access to the device (and therefore before interrupts are > configured). Thanks, > > Alex > >> >> > > } >> > > >> > > 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 >> > > >
On Tue, May 23, 2017 at 08:14:26PM +0100, Piotr Gregor wrote:
> Would the pci_setup_device() be a good place to move this check to?
Seems like a reasonable place.
On Tue, May 23, 2017 at 04:45:31PM -0500, Bjorn Helgaas wrote: > On Tue, May 23, 2017 at 08:14:26PM +0100, Piotr Gregor wrote: > > Would the pci_setup_device() be a good place to move this check to? > > Seems like a reasonable place. Will upload new patch moving test of INTx masking support to pci_setup_device(). There is just one thing: a quirk for devices that have this support broken (e.g. Ralink RT2800 802.11n). It is quirk_broken_intx_masking defined in quirks.c: 3190 /* 3191 * Some devices may pass our check in pci_intx_mask_supported() if 3192 * PCI_COMMAND_INTX_DISABLE works though they actually do not properly 3193 * support this feature. 3194 */ 3195 static void quirk_broken_intx_masking(struct pci_dev *dev) 3196 { 3197 dev->broken_intx_masking = 1; 3198 } 3199 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0030, 3200 quirk_broken_intx_masking); 3201 DECLARE_PCI_FIXUP_FINAL(0x1814, 0x0601, /* Ralink RT2800 802.11n PCI */ 3202 quirk_broken_intx_masking); The pci_intx_mask_supported() currently does check for dev->broken_intx_masking before performing INTx masking test by reading/writing registers. If we move pci_intx_mask_supported() check to pci_setup_device() just before header switch is entered: 1402 if (pci_intx_mask_supported(dev)) 1403 dev->intx_mask_support = 1; 1404 1405 switch (dev->hdr_type) { /* header type */ will the quirk be already applied so that pci_intx_mask_supported() can use broken_intx_masking() flag? Or should it just perform the test without check for this flag and only when checking later for INTx masking test we will use that flag (which would be assigned later), so it could be for example static inline bool pci_is_intx_mask_supported(struct pci_dev *pdev) { return (pdev->intx_mask_support && !pdev->broken_intx_masking); }
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); } 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. */ bool pci_check_and_mask_intx(struct pci_dev *dev)
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@rsyncme.org> --- drivers/pci/pci.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)