From patchwork Mon Nov 21 16:47:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 9439799 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 4829C60469 for ; Mon, 21 Nov 2016 16:47:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3C58028A3F for ; Mon, 21 Nov 2016 16:47:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2EFE128A90; Mon, 21 Nov 2016 16:47:12 +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 6BCC828A3F for ; Mon, 21 Nov 2016 16:47:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753936AbcKUQrJ (ORCPT ); Mon, 21 Nov 2016 11:47:09 -0500 Received: from mail.kernel.org ([198.145.29.136]:53688 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752988AbcKUQrH (ORCPT ); Mon, 21 Nov 2016 11:47:07 -0500 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AEF3B20114; Mon, 21 Nov 2016 16:47:05 +0000 (UTC) Received: from localhost (unknown [69.71.4.155]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9486620107; Mon, 21 Nov 2016 16:47:03 +0000 (UTC) Date: Mon, 21 Nov 2016 10:47:01 -0600 From: Bjorn Helgaas To: Gabriele Paoloni Cc: Bjorn Helgaas , "linux-pci@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linaro-acpi@lists.linaro.org" Subject: Re: [PATCH] PCI: Add information about describing PCI in ACPI Message-ID: <20161121164700.GL25762@bhelgaas-glaptop.roam.corp.google.com> References: <20161117175938.17465.45820.stgit@bhelgaas-glaptop.roam.corp.google.com> <20161118175404.GI25762@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Nov 21, 2016 at 08:52:52AM +0000, Gabriele Paoloni wrote: > Hi Bjorn > > > -----Original Message----- > > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > > Sent: 18 November 2016 17:54 > > To: Gabriele Paoloni > > Cc: Bjorn Helgaas; linux-pci@vger.kernel.org; linux- > > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linaro-acpi@lists.linaro.org > > Subject: Re: [PATCH] PCI: Add information about describing PCI in ACPI > > > > On Fri, Nov 18, 2016 at 05:17:34PM +0000, Gabriele Paoloni wrote: > > > > -----Original Message----- > > > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > > > > owner@vger.kernel.org] On Behalf Of Bjorn Helgaas > > > > Sent: 17 November 2016 18:00 > > > > > > +Static tables like MCFG, HPET, ECDT, etc., are *not* mechanisms > > for > > > > +reserving address space! The static tables are for things the OS > > > > +needs to know early in boot, before it can parse the ACPI > > namespace. > > > > +If a new table is defined, an old OS needs to operate correctly > > even > > > > +though it ignores the table. _CRS allows that because it is > > generic > > > > +and understood by the old OS; a static table does not. > > > > > > Right so if my understanding is correct you are saying that resources > > > described in the MCFG table should also be declared in PNP0C02 > > devices > > > so that the PNP driver can reserve these resources. > > > > Yes. > > > > > On the other side the PCI Root bridge driver should not reserve such > > > resources. > > > > > > Well if my understanding is correct I think we have a problem here: > > > http://lxr.free-electrons.com/source/drivers/pci/ecam.c#L74 > > > > > > As you can see pci_ecam_create() will conflict with the pnp driver > > > as it will try to reserve the resources from the MCFG table... > > > > > > Maybe we need to rework pci_ecam_create() ? > > > > I think it's OK as it is. > > > > The pnp/system.c driver does try to reserve PNP0C02 resources, and it > > marks them as "not busy". That way they appear in /proc/iomem and > > won't be allocated for anything else, but they can still be requested > > by drivers, e.g., pci/ecam.c, which will mark them "busy". > > > > This is analogous to what the PCI core does in pci_claim_resource(). > > This is really a function of the ACPI/PNP *core*, which should reserve > > all _CRS resources for all devices (not just PNP0C02 devices). But > > it's done by pnp/system.c, and only for PNP0C02, because there's a > > bunch of historical baggage there. > > > > You'll also notice that in this case, things are out of order: > > logically the pnp/system.c reservation should happen first, but in > > fact the pci/ecam.c request happens *before* the pnp/system.c one. > > That means the pnp/system.c one might fail and complain "[mem ...] > > could not be reserved". > > Correct me if I am wrong... > > So currently we are relying on the fact that pci_ecam_create() is called > before the pnp driver. > If the pnp driver came first we would end up in pci_ecam_create() failing > here: > http://lxr.free-electrons.com/source/drivers/pci/ecam.c#L76 > > I am not sure but it seems to me like a bit weak condition to rely on... > what about removing the error condition in pci_ecam_create() and logging > just a dev_info()? Huh. I'm confused. I *thought* it would be safe to reverse the order, which would effectively be this: system_pnp_probe reserve_resources_of_dev reserve_range request_mem_region([mem 0xb0000000-0xb1ffffff]) ... pci_ecam_create request_resource_conflict([mem 0xb0000000-0xb1ffffff]) but I experimented with the patch below on qemu, and it failed as you predicted: ** res test ** requested [mem 0xa0000000-0xafffffff] can't claim ECAM area [mem 0xa0000000-0xafffffff]: conflict with ECAM PNP [mem 0xa0000000-0xafffffff] I expected the request_resource_conflict() to succeed since it's completely contained in the "ECAM PNP" region. But I guess I don't understand kernel/resource.c well enough. I'm not sure we need to fix anything yet, since we currently do the ecam.c request before the system.c one, and any change there would be a long ways off. If/when that *does* change, I think the correct fix would be to change ecam.c so its request succeeds (by changing the way it does the request, fixing kernel/resource.c, or whatever) rather than to reduce the log level and ignore the failure. Bjorn --- To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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/arch/x86/pci/init.c b/arch/x86/pci/init.c index adb62aa..5a35638 100644 --- a/arch/x86/pci/init.c +++ b/arch/x86/pci/init.c @@ -7,6 +7,8 @@ in the right sequence from here. */ static __init int pci_arch_init(void) { + struct resource *res, *conflict; + static struct resource cfg; #ifdef CONFIG_PCI_DIRECT int type = 0; @@ -39,6 +41,26 @@ static __init int pci_arch_init(void) dmi_check_skip_isa_align(); + printk("\n** res test **\n"); + + res = request_mem_region(0xa0000000, 0x10000000, "ECAM PNP"); + printk("requested %pR\n", res); + if (!res) + return 0; + res->flags &= ~IORESOURCE_BUSY; + + cfg.start = 0xa0000000; + cfg.end = 0xafffffff; + cfg.flags = IORESOURCE_MEM | IORESOURCE_BUSY; + cfg.name = "PCI ECAM"; + + conflict = request_resource_conflict(&iomem_resource, &cfg); + if (conflict) + printk("can't claim ECAM area %pR: conflict with %s %pR\n", + &cfg, conflict->name, conflict); + + printk("\n"); + return 0; } arch_initcall(pci_arch_init);