PCI/MSI: bugfix/utilize for msi_capability_init()

PATCH In review Hidetoshi Seto 3 participants 4 messages Dec 24, 2008 – Jan 16, 2009 PCI

lore

Share & tools

lore

Hidetoshi Seto

author

2008-12-24 08:27 UTC #0 lore raw

PATCH 1/1 PCI/MSI: bugfix/utilize for msi_capability_init()

This patch fix a following bug and does a cleanup.

bug:

	commit 5993760f7fc75b77e4701f1e56dc84c0d6cf18d5
	had a wrong change (since is_64 is boolean[0|1]):

- pci_write_config_dword(dev,
- msi_mask_bits_reg(pos, is_64bit_address(control)),
- maskbits);
+ pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);

utilize:

	Unify separated if (entry->msi_attrib.maskbit) statements.

Signed-off-by Hidetoshi Seto <[email protected]>

---
 drivers/pci/msi.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

drivers/pci/msi.c +7 -9

index 74801f7..5aee7ee 100644
@@ -378,21 +378,19 @@ static int msi_capability_init(struct pci_dev *dev)
 	entry->msi_attrib.masked = 1;
 	entry->msi_attrib.default_irq = dev->irq;	/* Save IOAPIC IRQ */
 	entry->msi_attrib.pos = pos;
-	if (entry->msi_attrib.maskbit) {
-		entry->mask_base = (void __iomem *)(long)msi_mask_bits_reg(pos,
-				entry->msi_attrib.is_64);
-	}
 	entry->dev = dev;
 	if (entry->msi_attrib.maskbit) {
-		unsigned int maskbits, temp;
+		unsigned int base, maskbits, temp;
+
+		base = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
+		entry->mask_base = (void __iomem *)(long)base;
+
 		/* All MSIs are unmasked by default, Mask them all */
-		pci_read_config_dword(dev,
-			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
-			&maskbits);
+		pci_read_config_dword(dev, base, &maskbits);
 		temp = (1 << multi_msi_capable(control));
 		temp = ((temp - 1) & ~temp);
 		maskbits |= temp;
-		pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);
+		pci_write_config_dword(dev, base, maskbits);
 		entry->msi_attrib.maskbits_mask = temp;
 	}
 	list_add_tail(&entry->list, &dev->msi_list);

Signature · 1 line

1.6.1.rc4

Jike Song

2008-12-24 09:01 UTC #1 lore raw

Review

Quoted 11 lines · Hidetoshi Seto

This patch fix a following bug and does a cleanup.
bug:
commit 5993760f7fc75b77e4701f1e56dc84c0d6cf18d5 had a wrong change (since is_64 is boolean[0|1]):
(quoted diff) +1 -3
-               pci_write_config_dword(dev,
-                       msi_mask_bits_reg(pos, is_64bit_address(control)),
-                       maskbits);
+               pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);

Yes, really a nasty bug. I'm feeling guilty...
Should this fix hit 2.6.28 release? I CCed Jesse for his point.

Signature · 2 lines

Thanks,
Jike

Hidetoshi Seto

author

2008-12-25 06:18 UTC #2 lore raw

Quoted 8 lines · Jike Song

Quoted 6 lines · Hidetoshi Seto
This patch fix a following bug and does a cleanup.
bug:
commit 5993760f7fc75b77e4701f1e56dc84c0d6cf18d5 had a wrong change (since is_64 is boolean[0|1]):

Quoted 7 lines

Review
Quoted 5 lines
(quoted diff) +1 -3
-               pci_write_config_dword(dev,
-                       msi_mask_bits_reg(pos, is_64bit_address(control)),
-                       maskbits);
+               pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);
Yes, really a nasty bug. I'm feeling guilty...
Should this fix hit 2.6.28 release? I CCed Jesse for his point.

Unfortunately we failed to take this fix into Santa's bag...

Note that this bug affects mask condition after calling pci_enable_msi() for devices with MSI capability with (optional) per-vector masking support. Devices with MSI-X or MSI without masking support are safe. Once unmask/mask_msi_irq() is called, the condition should be fixed.

And the wrong pci_write_config_dword() will have no effects because target registers (fields in header, such as Vendor ID) are read-only.

Thanks,
H.Seto

Jesse Barnes

2009-01-16 18:37 UTC #3 lore raw

Quoted 21 lines · Hidetoshi Seto

Quoted 9 lines · Jike Song
On Wed, Dec 24, 2008 at 4:27 PM, Hidetoshi Seto
Quoted 6 lines · <[email protected]> wrote:
This patch fix a following bug and does a cleanup.
bug:
commit 5993760f7fc75b77e4701f1e56dc84c0d6cf18d5 had a wrong change (since is_64 is boolean[0|1]):
Quoted 8 lines
Quoted 5 lines
(quoted diff) +0 -2
-               pci_write_config_dword(dev,
-                       msi_mask_bits_reg(pos,
is_64bit_address(control)), - maskbits);
+ pci_write_config_dword(dev, entry->msi_attrib.is_64,
maskbits);
Yes, really a nasty bug. I'm feeling guilty...
Should this fix hit 2.6.28 release? I CCed Jesse for his point.
Unfortunately we failed to take this fix into Santa's bag...

Quoted 7 lines

Note that this bug affects mask condition after calling pci_enable_msi() for devices with MSI capability with (optional) per-vector masking support. Devices with MSI-X or MSI without masking support are safe. Once unmask/mask_msi_irq() is called, the condition should be fixed.
And the wrong pci_write_config_dword() will have no effects because target registers (fields in header, such as Vendor ID) are read-only.

Sorry, finally picked this one up. I've added a "cc: stable" on it as well, so when I push it to Linus it can go into 2.6.28.x.

In the future, please cc me on patches that are ready for upstream too, makes it easier for me to track things.

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center