On Wed, 2008-07-09 at 19:43 -0600, Matthew Wilcox wrote:
> On Thu, Jul 10, 2008 at 11:32:44AM +1000, Michael Ellerman wrote:
> > > int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> > > {
> > > + if (type == PCI_CAP_ID_MSI && nvec > 1)
> > > + return 1;
> >
> > This should go in arch_msi_check_device(). We might move it into a
> > ppc_md routine eventually.
>
> I'm OK with that, but ...
>
> > > int __attribute__ ((weak))
> > > arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> > > {
> > > - struct msi_desc *entry;
> > > + struct msi_desc *desc;
> > > int ret;
> > >
> > > - list_for_each_entry(entry, &dev->msi_list, list) {
> > > - ret = arch_setup_msi_irq(dev, entry);
> > > + if ((type == PCI_CAP_ID_MSI) && (nvec > 1))
> > > + return 1;
> >
> > I think the check should be in the generic arch_msi_check_device(), so
> > archs can override just the check.
>
> ... then x86 has to implement arch_msi_check_device in order to _not_
> perform the check, which feels a bit bass-ackwards to me.
Agreed, but I think that's still better. You might have alignment
constraints or whatever you need to check as well.
> > >
> > > void __attribute__ ((weak))
> > > -arch_teardown_msi_irqs(struct pci_dev *dev)
> > > +arch_teardown_msi_irqs(struct pci_dev *dev, int nvec)
> > > {
> > > struct msi_desc *entry;
> > >
> > > list_for_each_entry(entry, &dev->msi_list, list) {
> > > - if (entry->irq != 0)
> > > - arch_teardown_msi_irq(entry->irq);
> > > + int i;
> > > + if (entry->irq == 0)
> > > + continue;
> > > + for (i = 0; i < nvec; i++)
> > > + arch_teardown_msi_irq(entry->irq + i);
> >
> > This looks wrong. You're looping through all MSIs for the device, and
> > then for each one you're looping through all MSIs for the device. And
> > you're assuming they're contiguous, which they won't be for MSI-X.
> >
> > AFAICS this code should work for you as it was.
>
> For MSI-X, nvec will be = 1. Maybe I should call it something else to
> avoid confusion. The code won't work for me as-was because it won't
> call arch_teardown_msi_irq() for all entries.
It will call arch_teardown_msi_irq() for all entries, unless they were
never allocated (entry->irq == 0). Or are we talking about different
things?
If you mean that you're allocating more irqs than there are entries then
you need to deal with that in arch_teardown_msi_irqs().
> > > @@ -737,6 +737,8 @@ extern void msi_remove_pci_irq_vectors(struct pci_dev *dev);
> > > extern void pci_restore_msi_state(struct pci_dev *dev);
> > > #endif
> > >
> > > +#define pci_enable_msi(pdev) pci_enable_msi_block(pdev, 1)
> >
> > Someone will probably say this should be a static inline.
>
> Not quite sure why. You don't get any better typechecking by making it
> a static inline.
Yeah I agree, just pointing it out.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
Attachment:
signature.asc
Description: This is a digitally signed message part
