On Fri, 2009-05-15 at 09:49 +0900, Hidetoshi Seto wrote:
> It is definitely cleanup, not for NIU MSI-X problem, and not for .30
>
> One point...
>
> Matthew Wilcox wrote:
> > + nr_entries = msix_setup_entries(dev, pos, entries, nvec);
> >
> > ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
> > - if (ret < 0) {
> > - /* If we had some success report the number of irqs
> > - * we succeeded in setting up. */
> > - int avail = 0;
> > - list_for_each_entry(entry, &dev->msi_list, list) {
> > - if (entry->irq != 0) {
> > - avail++;
> > - }
> > - }
> > -
> > - if (avail != 0)
> > - ret = avail;
> > - }
> > + if (ret < 0 && nr_entries > 0)
> > + ret = nr_entries;
> >
> > if (ret) {
> > msi_free_irqs(dev);
>
> I think this changes the logic badly.
> nr_entries is number of allocated entry, while avail is number of irq
> successfully allocated. I suppose usually nr_entries > avail.
Yeah that bit's broken. If the arch routine fails (< 0) then we'll
return nr_entries to the driver, which will then try again with nvec =
nr_entries.
And you're passing nvec to the arch routine even though
msix_setup_entries() might not have allocated that many entries - which
means the value of nvec and the contents of the entry list don't match.
The overall structure of the split looks good though.
cheers
Attachment:
signature.asc
Description: This is a digitally signed message part
