Re: [PATCH 5/6] PCI MSI: Refactor interrupt masking code

[Matthew Wilcox <matthew@wil.cx>]

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."