Skip to contentSkip to conversation

PCI: set ->mask_pos correctly

PATCH In review Dan Carpenter 2 participants 3 messages Apr 30, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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
index d40bed7..2c10752 100644
@@ -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);
maintainer
#1 lore raw
Quoted 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.

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
index d40bed7..2c10752 100644
@@ -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);
#2 lore raw
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

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