On Fri, 2009-05-15 at 11:48 +0900, Hidetoshi Seto wrote:
> Michael Ellerman wrote:
> > 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.
>
> Hum, it seems it's an another bug. Then we need fix like this?
Ah geez you're right, never pays too look at the code too much :)
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -447,8 +447,10 @@ static int msix_capability_init(struct pci_dev *dev,
>
> for (i = 0; i < nvec; i++) {
> entry = alloc_msi_entry(dev);
> - if (!entry)
> - break;
> + if (!entry) {
> + msi_free_irqs(dev);
> + return -ENOMEM;
Should be:
return i;
> + }
>
> j = entries[i].entry;
> entry->msi_attrib.is_msix = 1;
>
> One concern is if we don't have enough memory to have number of entries
> requested at first time, the driver will get -ENOMEM and will not do retry
> even if we can have less number of entries.
That should be fixed by returning i, ie. the number of entries we had
memory to allocate.
cheers
Attachment:
signature.asc
Description: This is a digitally signed message part
