[PATCH] PCI/MSI: bugfix/utilize for msi_capability_init()

[PATCH] PCI/MSI: bugfix/utilize for msi_capability_init()

[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]):

-               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 <seto.hidetoshi@jp.fujitsu.com>
---
 drivers/pci/msi.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 74801f7..5aee7ee 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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);
-- 
1.6.1.rc4

Re: [PATCH] PCI/MSI: bugfix/utilize for msi_capability_init()

[Jike Song]

On Wed, Dec 24, 2008 at 4:27 PM, Hidetoshi Seto
<seto.hidetoshi@jp.fujitsu.com> 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]):
>
> -               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.

-- 
Thanks,
Jike

Re: [PATCH] PCI/MSI: bugfix/utilize for msi_capability_init()

[Hidetoshi Seto]

Jike Song wrote:
> On Wed, Dec 24, 2008 at 4:27 PM, Hidetoshi Seto
> <seto.hidetoshi@jp.fujitsu.com> 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]):
>>
>> -               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

Re: [PATCH] PCI/MSI: bugfix/utilize for msi_capability_init()

[Jesse Barnes]

On Wednesday, December 24, 2008 10:17 pm Hidetoshi Seto wrote:
> Jike Song wrote:
> > On Wed, Dec 24, 2008 at 4:27 PM, Hidetoshi Seto
> >
> > <seto.hidetoshi@jp.fujitsu.com> 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]):
> >>
> >> -               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.

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,
-- 
Jesse Barnes, Intel Open Source Technology Center