From patchwork Fri May 15 02:48:54 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hidetoshi Seto X-Patchwork-Id: 23942 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n4F2oYgR027733 for ; Fri, 15 May 2009 02:50:34 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756928AbZEOCtv (ORCPT ); Thu, 14 May 2009 22:49:51 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755550AbZEOCtv (ORCPT ); Thu, 14 May 2009 22:49:51 -0400 Received: from fgwmail6.fujitsu.co.jp ([192.51.44.36]:37298 "EHLO fgwmail6.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759255AbZEOCtu (ORCPT ); Thu, 14 May 2009 22:49:50 -0400 Received: from m2.gw.fujitsu.co.jp ([10.0.50.72]) by fgwmail6.fujitsu.co.jp (Fujitsu Gateway) with ESMTP id n4F2nmpJ023270 for (envelope-from seto.hidetoshi@jp.fujitsu.com); Fri, 15 May 2009 11:49:48 +0900 Received: from smail (m2 [127.0.0.1]) by outgoing.m2.gw.fujitsu.co.jp (Postfix) with ESMTP id 12B7745DE55 for ; Fri, 15 May 2009 11:49:48 +0900 (JST) Received: from s2.gw.fujitsu.co.jp (s2.gw.fujitsu.co.jp [10.0.50.92]) by m2.gw.fujitsu.co.jp (Postfix) with ESMTP id CE00F45DD79 for ; Fri, 15 May 2009 11:49:47 +0900 (JST) Received: from s2.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1]) by s2.gw.fujitsu.co.jp (Postfix) with ESMTP id ADAC8E38003 for ; Fri, 15 May 2009 11:49:47 +0900 (JST) Received: from m107.s.css.fujitsu.com (m107.s.css.fujitsu.com [10.249.87.107]) by s2.gw.fujitsu.co.jp (Postfix) with ESMTP id 59751E18006 for ; Fri, 15 May 2009 11:49:47 +0900 (JST) Received: from m107.css.fujitsu.com (m107 [127.0.0.1]) by m107.s.css.fujitsu.com (Postfix) with ESMTP id 29A8C2C0109; Fri, 15 May 2009 11:49:47 +0900 (JST) Received: from [127.0.0.1] (unknown [10.124.100.141]) by m107.s.css.fujitsu.com (Postfix) with ESMTP id AEEEA2C00E2; Fri, 15 May 2009 11:49:46 +0900 (JST) Message-ID: <4A0CD816.7020500@jp.fujitsu.com> Date: Fri, 15 May 2009 11:48:54 +0900 From: Hidetoshi Seto User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: michael@ellerman.id.au CC: Matthew Wilcox , linux-pci@vger.kernel.org Subject: Re: [PATCH] incremental fix for NIU MSI-X problem References: <20090513214309.GL15360@parisc-linux.org> <1242272519.7408.26.camel@concordia> <4A0BCFFE.2060303@jp.fujitsu.com> <20090514134433.GQ15360@parisc-linux.org> <20090514134550.GR15360@parisc-linux.org> <4A0CBC06.4020808@jp.fujitsu.com> <1242351519.24951.64.camel@concordia> In-Reply-To: <1242351519.24951.64.camel@concordia> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org 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? 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. i.e. if the driver can handle its device with 32 or 16 or 8 vectors, and if there are only memory for 24 entries, and if there are only 30 vectors available, the best return value of pci_enable_msix() will be 24. It will let the driver to do retry with 16. In current code, the return value of pci_enable_msix() in the above case might be 0 even it allocates 24 vectors while requested is 32. Yes, it is broken. The above patch hunk will fix the broken behavior, but it cannot handle low-memory condition very well. However we could have another patch for that as improve later. 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 --- 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; + } j = entries[i].entry; entry->msi_attrib.is_msix = 1;