| Message ID | 20130430074454.GF7237@elgon.mountain |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Apr 30, 2013 at 1:44 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > 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 <dan.carpenter@oracle.com> 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 > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d40bed7..2c10752 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -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); -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 30, 2013 at 10:45:35AM -0600, Bjorn Helgaas wrote: > On Tue, Apr 30, 2013 at 1:44 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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 <dan.carpenter@oracle.com> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index d40bed7..2c10752 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -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);
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 <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html