PCI: set ->mask_pos correctly
Subsystems (2)
PATCH 1/1 PCI: set ->mask_pos correctly
The "+" operation has higher precedence than "?:" and ->msi_cap is always non-zero here so the original statement is equivalent to:
entry->mask_pos = PCI_MSI_MASK_64;
Which wasn't the intent.
drivers/pci/msi.c +4 -2
@@ -563,8 +563,10 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) entry->msi_attrib.default_irq = dev->irq;/* Save IOAPIC IRQ */ entry->msi_attrib.pos = dev->msi_cap; - entry->mask_pos = dev->msi_cap + (control &PCI_MSI_FLAGS_64BIT ) ? -PCI_MSI_MASK_64 :PCI_MSI_MASK_32 ; +if (control &PCI_MSI_FLAGS_64BIT ) + entry->mask_pos = dev->msi_cap +PCI_MSI_MASK_64 ; +else + entry->mask_pos = dev->msi_cap +PCI_MSI_MASK_32 ;/* All MSIs are unmasked by default, Mask them all */ if (entry->msi_attrib.maskbit)pci_read_config_dword (dev, entry->mask_pos, &entry->masked);
Bjorn Helgaas
maintainerQuoted 8 lines · Dan Carpenter
The "+" operation has higher precedence than "?:" and ->msi_cap is always non-zero here so the original statement is equivalent to:
entry->mask_pos = PCI_MSI_MASK_64;Which wasn't the intent.
Signed-off-by Dan Carpenter <[email protected]>
Ouch, my fault, sorry about that. Thanks for finding this. I put this in my for-linus branch and will try to get this in before v3.10-rc1.
How did you find this? I guess MSI didn't work right on a device with 32-bit message address and per-vector masking?
Bjorn
Quoted 17 lines
drivers/pci/msi.c +4
-2@@ -563,8 +563,10 @@ static int msi_capability_init(struct pci_dev *dev, int nvec) entry->msi_attrib.default_irq = dev->irq;/* Save IOAPIC IRQ */ entry->msi_attrib.pos = dev->msi_cap; - entry->mask_pos = dev->msi_cap + (control &PCI_MSI_FLAGS_64BIT ) ? -PCI_MSI_MASK_64 :PCI_MSI_MASK_32 ; +if (control &PCI_MSI_FLAGS_64BIT ) + entry->mask_pos = dev->msi_cap +PCI_MSI_MASK_64 ; +else + entry->mask_pos = dev->msi_cap +PCI_MSI_MASK_32 ;/* All MSIs are unmasked by default, Mask them all */ if (entry->msi_attrib.maskbit)pci_read_config_dword (dev, entry->mask_pos, &entry->masked);
Quoted 8 lines · Bjorn Helgaas
Quoted 7 lines · Dan Carpenter
The "+" operation has higher precedence than "?:" and ->msi_cap is always non-zero here so the original statement is equivalent to:
entry->mask_pos = PCI_MSI_MASK_64;Which wasn't the intent.
Quoted 9 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>Ouch, my fault, sorry about that. Thanks for finding this. I put this in my for-linus branch and will try to get this in before v3.10-rc1.
How did you find this? I guess MSI didn't work right on a device with 32-bit message address and per-vector masking?
Static analysis. I have a check that complains about every condition like:
if (foo + bar) { ...It has too many false positives to foist on the public though.
regards,
dan carpenter