From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20160309071406.GA19555@red-moon> References: <1455212827-9382-1-git-send-email-lorenzo.pieralisi@arm.com> <20160308222125.GG27132@localhost> <20160309053332.GA22228@localhost> <20160309071406.GA19555@red-moon> Date: Wed, 9 Mar 2016 15:20:32 +0100 Message-ID: Subject: Re: [PATCH v2] PCI: ACPI: IA64: fix IO port generic range check From: "Rafael J. Wysocki" To: Lorenzo Pieralisi Cc: Bjorn Helgaas , "Rafael J. Wysocki" , Linux PCI , ACPI Devel Maling List , "linux-ia64@vger.kernel.org" , Bjorn Helgaas , Hanjun Guo , Jiang Liu , Tony Luck , Tomasz Nowicki , Mark Salter , "Rafael J. Wysocki" Content-Type: text/plain; charset=UTF-8 Sender: linux-acpi-owner@vger.kernel.org List-ID: On Wed, Mar 9, 2016 at 8:14 AM, Lorenzo Pieralisi wrote: > On Tue, Mar 08, 2016 at 11:33:32PM -0600, Bjorn Helgaas wrote: >> On Tue, Mar 08, 2016 at 11:27:01PM +0100, Rafael J. Wysocki wrote: >> > On Tue, Mar 8, 2016 at 11:21 PM, Bjorn Helgaas wrote: >> > > On Thu, Feb 11, 2016 at 05:47:07PM +0000, Lorenzo Pieralisi wrote: >> > >> The [0 - 64k] ACPI PCI IO port resource boundary check in: >> > >> [cut] >> Wait a minute, this doesn't seem right to me. >> >> The problem we're trying to fix is that on ia64, we incorrectly >> discard the PCI host bridge window [io 0x1000000-0x100ffff window]. >> >> That window is currently discarded by the generic >> acpi_dev_ioresource_flags() function, where we're removing this code: >> >> - if (res->end >= 0x10003) >> - res->flags |= IORESOURCE_DISABLED | IORESOURCE_UNSET; >> >> and we're adding the "res->end >= 0x10003" check to >> arch/x86/pci/acpi.c. >> >> But the removal also affects other users of acpi_dev_ioresource_flags(), >> and that's broader than the scope of this patch. We might want to >> remove the check, but if we do, it should be in a separate patch by >> itself so it isn't a hidden side-effect of fixing this ia64 problem. >> >> The other users of acpi_dev_ioresource_flags() include: >> >> {acpi_lpss_create_device,acpi_create_platform_device,acpi_pci_probe_root_resources,acpi_default_enumeration,bcm_acpi_probe,tpm_tis_acpi_init,acpi_dma_parse_resource_group,acpi_dma_request_slave_chan_by_index,acpi_gpio_resource_lookup,acpi_gpio_count,acpi_i2c_add_device,inv_mpu_process_acpi_config,acpi_spi_add_device} >> acpi_dev_get_resources >> acpi_dev_process_resource >> acpi_dev_resource_io >> acpi_dev_get_ioresource >> acpi_dev_ioresource_flags >> >> {pnpacpi_add_device,resources_store} >> pnpacpi_parse_allocated_resource >> pnpacpi_allocated_resource >> acpi_dev_resource_io >> acpi_dev_get_ioresource >> acpi_dev_ioresource_flags >> >> I think the original test in acpi_dev_ioresource_flags() isn't quite >> correct because it's generic code, but it enforces an arch-specific >> 64K limit. > > Yes, I was about to write to you I noticed the same issue. > > That (>=0x10003) check in generic code is an x86ism, it is wrong but > it is there and given that there are other potential > users of acpi_dev_ioresource_flags() this patch should > be dropped I do not want to trigger x86 regressions because > some IO resources are not filtered. > > I am travelling, so can't have a proper look till next week, > Rafael, please drop this patch. > >> Maybe acpi_dev_ioresource_flags() should instead check res->end >> against ioport_resource.end, as we already do in >> acpi_pci_root_validate_resources()? Each arch already sets its own >> ioport_resource.end using IO_SPACE_LIMIT: >> >> arch/x86/include/asm/io.h #define IO_SPACE_LIMIT 0xffff >> arch/ia64/include/asm/io.h #define IO_SPACE_LIMIT 0xffffffffffffffffUL > > We can't do that, it may work on IA64 but the ioport_resource is a > chunk of address space on IA64/ARM64 that has nothing to do with > the physical address at which the root bridges decode IO space (which > is what's contained in the resource). > > I will have a look next week, please drop this patch. OK, dropping. Thanks, Rafael