| Message ID | 4A0CF699.2060904@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2009-05-15 at 13:59 +0900, Hidetoshi Seto wrote: > In current code it continues setup even if alloc_msi_entry for MSI-X > is failed due to lack of memory. It means arch_setup_msi_irqs() might > be called with msi_desc entries less than its argument nvec. > > At least x86's arch_setup_msi_irqs() uses list_for_each_entry() for > dev->msi_list that suspected to have entries same numbers as nvec, and > it doesn't check the number of allocated vectors and passed arg nvec. > Therefore it will result in success of pci_enable_msix(), with less > vectors allocated than requested. > > This patch fixes the error route to return the number of entries can be > allocated, instead of continuing the setup. It will let drivers to > retry pci_enable_msix() with less number of entries. > > Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> > --- > drivers/pci/msi.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index 6f2e629..952432a 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -445,8 +445,10 @@ static int msix_capability_init(struct pci_dev *dev, > /* MSI-X Table Initialization */ > for (i = 0; i < nvec; i++) { > entry = alloc_msi_entry(dev); > - if (!entry) > - break; > + if (!entry) { > + msi_free_irqs(dev); > + return i; > + } > > j = entries[i].entry; > entry->msi_attrib.is_msix = 1; That looks right to me. Have you tested it? By faking allocation failure in alloc_msi_entry(). As requested: Acked-by: Michael Ellerman <michael@ellerman.id.au> I guess this would be a nice fix for 30, but no one's hit it yet so it's not urgent and could wait for 31. cheers
Michael Ellerman wrote: > On Fri, 2009-05-15 at 13:59 +0900, Hidetoshi Seto wrote: >> @@ -445,8 +445,10 @@ static int msix_capability_init(struct pci_dev *dev, >> /* MSI-X Table Initialization */ >> for (i = 0; i < nvec; i++) { >> entry = alloc_msi_entry(dev); >> - if (!entry) >> - break; >> + if (!entry) { >> + msi_free_irqs(dev); >> + return i; >> + } >> >> j = entries[i].entry; >> entry->msi_attrib.is_msix = 1; > > That looks right to me. Have you tested it? By faking allocation failure > in alloc_msi_entry(). Oops, my test found a bug... we should return -ENOMEM if it fails with i==0. I'll post v2 with my Tested-by soon. Sorry, please review it again. > I guess this would be a nice fix for 30, but no one's hit it yet so it's > not urgent and could wait for 31. I agree with it. Thanks, H.Seto -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 6f2e629..952432a 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -445,8 +445,10 @@ static int msix_capability_init(struct pci_dev *dev, /* MSI-X Table Initialization */ for (i = 0; i < nvec; i++) { entry = alloc_msi_entry(dev); - if (!entry) - break; + if (!entry) { + msi_free_irqs(dev); + return i; + } j = entries[i].entry; entry->msi_attrib.is_msix = 1;
In current code it continues setup even if alloc_msi_entry for MSI-X is failed due to lack of memory. It means arch_setup_msi_irqs() might be called with msi_desc entries less than its argument nvec. At least x86's arch_setup_msi_irqs() uses list_for_each_entry() for dev->msi_list that suspected to have entries same numbers as nvec, and it doesn't check the number of allocated vectors and passed arg nvec. Therefore it will result in success of pci_enable_msix(), with less vectors allocated than requested. This patch fixes the error route to return the number of entries can be allocated, instead of continuing the setup. It will let drivers to retry pci_enable_msix() with less number of entries. Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> --- drivers/pci/msi.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)