From patchwork Tue Sep 8 16:49:34 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jiang Liu X-Patchwork-Id: 7142671 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 5DDAF9F1BF for ; Tue, 8 Sep 2015 16:50:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 67BF42083B for ; Tue, 8 Sep 2015 16:50:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 524B320834 for ; Tue, 8 Sep 2015 16:50:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755158AbbIHQt7 (ORCPT ); Tue, 8 Sep 2015 12:49:59 -0400 Received: from mga01.intel.com ([192.55.52.88]:46759 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754960AbbIHQt6 (ORCPT ); Tue, 8 Sep 2015 12:49:58 -0400 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 08 Sep 2015 09:49:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,491,1437462000"; d="scan'208";a="800195854" Received: from unknown (HELO [10.254.214.66]) ([10.254.214.66]) by orsmga002.jf.intel.com with ESMTP; 08 Sep 2015 09:49:35 -0700 Subject: Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers To: Bjorn Helgaas References: <55EE8106.6060100@internode.on.net> <1441697190-27993-1-git-send-email-jiang.liu@linux.intel.com> <20150908162735.GK829@google.com> Cc: Thomas Gleixner , Arthur Marsh , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-scsi@vger.kernel.org From: Jiang Liu Organization: Intel Message-ID: <55EF119E.5030905@linux.intel.com> Date: Wed, 9 Sep 2015 00:49:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20150908162735.GK829@google.com> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2015/9/9 0:27, Bjorn Helgaas wrote: > Hi Jiang, > > I object to subject lines like "Correctly do such and such." Nobody > writes code to do things *incorrectly*, so the word "correctly" takes > up space without contributing meaning. In this case, it's at least > debatable whether this is even the "correct" approach; see below. > > On Tue, Sep 08, 2015 at 03:26:29PM +0800, Jiang Liu wrote: >> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and >> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ >> for PCI devices on x86 platforms. Instead of allocating PCI legacy >> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq() >> will be called by pci_device_probe() to allocate PCI legacy IRQs >> when binding PCI drivers to PCI devices. >> >> But some device drivers, such as eata, directly access PCI devices >> without implementing corresponding PCI drivers, so pcibios_alloc_irq() >> won't be called for those PCI devices and wrong IRQ number may be >> used to manage the PCI device. > > I'm not sure this is wise. > > We normally call pcibios_alloc_irq() from pci_device_probe(), just > before we call the driver's .probe() method. > > The eata driver does not use pci_register_driver(), so there is no > .probe() method (also no .remove(), .suspend(), etc.) But eata *does* > use pci_enable_device() and other PCI interfaces. So this patch adds > code in the x86 pci_enable_device() path for this case. > > AFAICT, there's no real reason why eata doesn't register a PCI driver; > it's just a case of legacy code where nobody has been motivated to > update it. I'm not in favor of catering to code like that because > then we have random special cases like this that clutter up the core > code. > > I don't think we should necessarily expect the PCI core to support > calls to PCI interfaces when it hasn't had a chance to initialize > itself via driver registration. > >> So detect such a case in pcibios_enable_device() by checking >> pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI >> legacy IRQs. >> >> Signed-off-by: Jiang Liu >> --- >> arch/x86/pci/common.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c >> index 09d3afc0a181..60b237783582 100644 >> --- a/arch/x86/pci/common.c >> +++ b/arch/x86/pci/common.c >> @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev) >> >> int pcibios_enable_device(struct pci_dev *dev, int mask) >> { >> + /* >> + * By design, pcibios_alloc_irq() will be called by pci_device_probe() >> + * when binding a PCI device to a PCI driver. But some device drivers, >> + * such as eata, directly make use of PCI devices without implementing >> + * PCI device drivers, so pcibios_alloc_irq() won't be called for those >> + * PCI devices. >> + */ >> + if (!dev->driver) >> + pcibios_alloc_irq(dev); > > This is a point fix for x86 only, but I think eata can be built for > any architecture. Won't other architectures still have the same > problem? Hi Bjorn, We have used another draft version to fix this issue by changing eata driver as below. But that needs to export pcibios_alloc_irq. And I'm not sure whether there are other drivers having the same behavior. If we think it's a legacy behavior and only a few drivers may have such a behavior, I prefer changing drivers to fix the issue too. Thanks! Gerry --- drivers/pci/pci-driver.c | 1 + drivers/scsi/eata.c | 2 ++ 2 files changed, 3 insertions(+) > >> return pci_enable_resources(dev, mask); >> } >> >> -- >> 1.7.10.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" 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/pci-driver.c b/drivers/pci/pci-driver.c index 52a880ca1768..17d2a0b1de18 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -392,6 +392,7 @@ int __weak pcibios_alloc_irq(struct pci_dev *dev) { return 0; } +EXPORT_SYMBOL_GPL(pcibios_alloc_irq); void __weak pcibios_free_irq(struct pci_dev *dev) { diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c index 227dd2c2ec2f..7e6eaf867987 100644 --- a/drivers/scsi/eata.c +++ b/drivers/scsi/eata.c @@ -1061,6 +1061,7 @@ static void enable_pci_ports(void) driver_name, dev->bus->number, dev->devfn); #endif + pcibios_alloc_irq(dev); if (pci_enable_device(dev)) printk ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n", @@ -1520,6 +1521,7 @@ static void add_pci_ports(void) if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) break; + pcibios_alloc_irq(dev); if (pci_enable_device(dev)) { #if defined(DEBUG_PCI_DETECT) printk