Re: [PATCH 2/4] PCI: Support multiple MSI

[Michael Ellerman]

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