Patchwork PCI: set ->mask_pos correctly

login
register
mail settings
Submitter Dan Carpenter
Date April 30, 2013, 7:44 a.m.
Message ID <20130430074454.GF7237@elgon.mountain>
Download mbox | patch
Permalink /patch/240578/
State Accepted
Headers show

Comments

Dan Carpenter - April 30, 2013, 7:44 a.m.
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
Bjorn Helgaas - April 30, 2013, 4:45 p.m.
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
Dan Carpenter - April 30, 2013, 8:57 p.m.
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

Patch

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);