From patchwork Tue Feb 7 21:46:45 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 9561269 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 71C14602B1 for ; Tue, 7 Feb 2017 21:46:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 63CF128475 for ; Tue, 7 Feb 2017 21:46:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 569862848B; Tue, 7 Feb 2017 21:46:59 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9621928475 for ; Tue, 7 Feb 2017 21:46:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932442AbdBGVq4 (ORCPT ); Tue, 7 Feb 2017 16:46:56 -0500 Received: from mail.kernel.org ([198.145.29.136]:57720 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932416AbdBGVqz (ORCPT ); Tue, 7 Feb 2017 16:46:55 -0500 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8010F2038D; Tue, 7 Feb 2017 21:46:48 +0000 (UTC) Received: from localhost (unknown [69.55.156.165]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3887620384; Tue, 7 Feb 2017 21:46:47 +0000 (UTC) Date: Tue, 7 Feb 2017 15:46:45 -0600 From: Bjorn Helgaas To: Lorenzo Pieralisi Cc: Dongdong Liu , linux-pci@vger.kernel.org, wangzhou1@hisilicon.com, gabriele.paoloni@huawei.com, charles.chenxin@huawei.com, linuxarm@huawei.com, Tomasz Nowicki Subject: Re: [PATCH 3/3] PCI: generic: Fix the bug of pci_fixup_irqs() for arm64 platform. Message-ID: <20170207214645.GB11659@bhelgaas-glaptop.roam.corp.google.com> References: <1484202504-96686-1-git-send-email-liudongdong3@huawei.com> <1484202504-96686-4-git-send-email-liudongdong3@huawei.com> <20170203204530.GH10291@bhelgaas-glaptop.roam.corp.google.com> <20170206140827.GB14139@red-moon> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170206140827.GB14139@red-moon> User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 06, 2017 at 02:08:27PM +0000, Lorenzo Pieralisi wrote: > On Fri, Feb 03, 2017 at 02:45:30PM -0600, Bjorn Helgaas wrote: > > [+cc Tomasz, Lorenzo] > > > > On Thu, Jan 12, 2017 at 02:28:24PM +0800, Dongdong Liu wrote: > > > arch/arm64/pci.c pcibios_alloc_irq() has the same function as > > > pci_fixup_irqs(), so we add condition #ifndef CONFIG_ARM64 for > > > pci_fixup_irqs(). > > > > > > Signed-off-by: Dongdong Liu > > > Reviewed-by: Gabriele Paoloni > > > Reviewed-by: Zhou Wang > > > --- > > > drivers/pci/host/pci-host-common.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c > > > index e3c48b5..f160afc 100644 > > > --- a/drivers/pci/host/pci-host-common.c > > > +++ b/drivers/pci/host/pci-host-common.c > > > @@ -145,7 +145,9 @@ int pci_host_common_probe(struct platform_device *pdev, > > > return -ENODEV; > > > } > > > > > > +#ifndef CONFIG_ARM64 > > > pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > > +#endif > > > > d8ed75d59332 ("ARM64: PCI: ACPI support for legacy IRQs parsing and > > consolidation with DT code") added pcibios_alloc_irq() for arm64. > > > > arm64 is the only arch that implements pcibios_alloc_irq(). And now > > you want to add an ifdef here so arm64 is the only arch that doesn't > > call pci_fixup_irqs(). > > > > I don't remember the details of why arm64 is so special here. > > Obviously we'd prefer not to have the ifdef and not to have the > > arm64-specific pcibios_alloc_irq(). > > Well, I am not sure ARM64 is more special than other architectures > in this respect, actually I think that what ARM64 does is what we > will end up doing when Matthew Minter's patches are merged and > that's my aim for v4.12, pci_fixup_irqs() should not be used > any longer, at least on ARM/ARM64 and I know what to do to make > it disappear. ARM64 is different from x86, even though both arches support both ACPI and DT: on ARM64 we currently call of_irq_parse_and_map_pci() when we call the driver's probe() method, whereas on x86 we don't call it until the driver calls pci_enable_device(). Obviously it would be better if this were done at the same point on both arches. I don't know what that point *is*. I could imagine enumeration-time, probe-time, or enable-time. OK, back to the patch at hand. I think we're agreed that having pci_host_common_probe() call pci_fixup_irqs() is wrong on all architectures (it doesn't cover hot-added devices and it may clobber an IRQ after a driver has claimed the device (since it "fixes" all PCI devices). And we hope to fix this in the next cycle using Matt's work. So I think I'm OK with this transient ugliness. Although pci-host-common.c is currently only available on ARM and ARM64, in theory, it should be usable on x86. I would probably use "#ifdef ARM" instead of "#ifndef ARM64" so that when it is made available on x86, we won't call pci_fixup_irqs() there. What do you think of the following? commit 4e3538fb84dcba2de5d5b0990ced7f3f881fda1e Author: Dongdong Liu Date: Thu Jan 12 14:28:24 2017 +0800 PCI: generic: Call pci_fixup_irqs() only on ARM pci_fixup_irqs() is problematic because: - it's called when we enumerate a host bridge, so we don't fixup IRQs for hot-added PCI devices, and - it fixes up IRQs for all PCI devices in the system, so if we call it multiple times, e.g., if we have several host controllers, we may reallocate an IRQ for a device after a driver has already claimed it. We plan to replace pci_fixup_irqs() soon, but we still need it on ARM because we don't have any other generic method for doing this. On ARM64, we don't need pci_fixup_irqs() because we do IRQ setup when we bind a driver to the device (in the pci_device_probe() -> pcibios_alloc_irq() path). pci-host-common.c is currently only used on ARM and ARM64. In principle, it could be used on x86, and we wouldn't want pci_fixup_irqs() there either, because x86 does IRQ setup in the pci_enable_device() path. [bhelgaas: changelog, use #ifdef ARM, not #ifndef ARM64] Signed-off-by: Dongdong Liu Signed-off-by: Bjorn Helgaas Reviewed-by: Gabriele Paoloni Reviewed-by: Zhou Wang Acked-by: Lorenzo Pieralisi diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c index e3c48b5deb93..e9a53bae1c25 100644 --- a/drivers/pci/host/pci-host-common.c +++ b/drivers/pci/host/pci-host-common.c @@ -145,7 +145,9 @@ int pci_host_common_probe(struct platform_device *pdev, return -ENODEV; } +#ifdef CONFIG_ARM pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); +#endif /* * We insert PCI resources into the iomem_resource and