On Tue, Mar 03, 2009 at 11:16:12AM +1100, Michael Ellerman wrote:
> I don't see why msi_set_mask_bit() or msi_mask_irq() need to return
> anything, their return values are never used AFAICT.
You're right. Changed to void.
> > @@ -356,21 +361,15 @@ static int msi_capability_init(struct pci_dev *dev)
> > entry->msi_attrib.is_64 = is_64bit_address(control);
> > entry->msi_attrib.entry_nr = 0;
> > entry->msi_attrib.maskbit = is_mask_bit_support(control);
> > - 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) {
> > - unsigned int base, maskbits, temp;
> > -
> > - base = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
> > - entry->mask_pos = base;
> > - /* All MSIs are unmasked by default, Mask them all */
> > - pci_read_config_dword(dev, base, &maskbits);
> > - temp = msi_mask((control & PCI_MSI_FLAGS_QMASK) >> 1);
> > - maskbits |= temp;
> > - pci_write_config_dword(dev, base, maskbits);
> > - entry->msi_attrib.maskbits_mask = temp;
> > - }
> > +
> > + entry->mask_pos = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
> > + /* All MSIs are unmasked by default, Mask them all */
> > + pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
> > + mask = msi_capable_mask(control);
> > + msi_mask_irq(entry, mask, mask);
>
> This looked a little weird at first, in that we're unconditionally doing
> the mask - but we're not, msi_mask_irq() checks for us. I guess it's no
> drama reading from mask_pos even if it's not implemented.
Hm, wasn't quite my intent. Here's the replacement:
entry->mask_pos = msi_mask_bits_reg(pos, entry->msi_attrib.is_64);
/* All MSIs are unmasked by default, Mask them all */
if (entry->msi_attrib.maskbit)
pci_read_config_dword(dev, entry->mask_pos, &entry->masked);
mask = msi_capable_mask(control);
msi_mask_irq(entry, mask, mask);
So mask_pos still points somewhere bogus, but all uses of it are now guarded by msi_attrib.maskbit, which is OK.
> > @@ -435,11 +434,12 @@ static int msix_capability_init(struct pci_dev *dev,
> > entry->msi_attrib.is_msix = 1;
> > entry->msi_attrib.is_64 = 1;
> > entry->msi_attrib.entry_nr = j;
> > - entry->msi_attrib.maskbit = 1;
> > - entry->msi_attrib.masked = 1;
> > entry->msi_attrib.default_irq = dev->irq;
> > entry->msi_attrib.pos = pos;
> > entry->mask_base = base;
> > + entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE +
> > + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> > + msix_mask_irq(entry, 1);
>
> I was going to say "why bother with the readl". But checking the spec,
> the rest of the bits are reserved and we mustn't muck with them.
Yeah, we've got away with that until now. I just checked PCIe 2.1
(out today), and, er, it seems we can't rely on that any longer.
Something about a "TPH Requester Capability" and a "Steering Tag".
I'm looking forward to learning more about those in the next few months.
> > @@ -568,18 +570,13 @@ void pci_msi_shutdown(struct pci_dev* dev)
> > dev->msi_enabled = 0;
> >
> > BUG_ON(list_empty(&dev->msi_list));
> > - entry = list_entry(dev->msi_list.next, struct msi_desc, list);
> > - /* Return the the pci reset with msi irqs unmasked */
> > - if (entry->msi_attrib.maskbit) {
> > - u32 mask = entry->msi_attrib.maskbits_mask;
> > - struct irq_desc *desc = irq_to_desc(dev->irq);
> > - msi_set_mask_bits(desc, mask, ~mask);
> > - }
> > - if (entry->msi_attrib.is_msix)
> > - return;
>
> You loose this return case, but we should never have hit it AFAICS
> because of the check of !dev->msi_enabled earlier - so I think it's ok.
Yeah, I deleted it on purpose.
Thanks for the review!
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
