Re: [Bugfix v5] x86/PCI/ACPI: Fix regression caused by commit 63f1789ec716

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2015/4/29 22:15, Rafael J. Wysocki wrote:
> On Wednesday, April 29, 2015 09:33:16 PM Jiang Liu wrote:
>> On 2015/4/29 21:20, Bjorn Helgaas wrote:
>>> On Tue, Apr 28, 2015 at 7:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Monday, April 20, 2015 11:08:58 AM Jiang Liu wrote:
>>>>> An IO port or MMIO resource assigned to a PCI host bridge may be
>>>>> consumed by the host bridge itself or available to its child
>>>>> bus/devices. On x86 and IA64 platforms, all IO port and MMIO
>>>>> resources are assumed to be available to child bus/devices
>>>>> except one special case:
>>>>>     IO port [0xCF8-0xCFF] is consumed by the host bridge itself
>>>>>     to access PCI configuration space.
>>>>>
>>>>> But the ACPI and PCI Firmware specifications haven't provided a method
>>>>> to tell whether a resource is consumed by the host bridge itself.
>>>>> So before commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource
>>>>> interfaces to simplify implementation"), arch/x86/pci/acpi.c ignored
>>>>> all IO port resources defined by acpi_resource_io and
>>>>> acpi_resource_fixed_io to filter out IO ports consumed by the host
>>>>> bridge itself.
>>>>>
>>>>> Commit 593669c2ac0f ("x86/PCI/ACPI: Use common ACPI resource interfaces
>>>>> to simplify implementation")started accepting all IO port and MMIO
>>>>> resources, which caused a regression that IO port resources consumed
>>>>> by the host bridge itself became available to its child devices.
>>>>>
>>>>> Then commit 63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>>>>> host bridge itself") ignored resources consumed by the host bridge
>>>>> itself by checking the IORESOURCE_WINDOW flag, which accidently removed
>>>>> MMIO resources defined by acpi_resource_memory24, acpi_resource_memory32
>>>>> and acpi_resource_fixed_memory32.
>>>>>
>>>>> So revert to the behavior before v3.19 to fix the regression.
>>>>>
>>>>> There is also a discussion about ignoring the Producer/Consumer flag on
>>>>> IA64 platforms at:
>>>>> http://patchwork.ozlabs.org/patch/461633/
>>>>>
>>>>> Related ACPI table are archived at:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
>>>>>
>>>>> Fixes: 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>>> Reported-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
>>>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>>>
>>>> Bjorn, Ingo, is anyone looking at this?  We're still having a regression in
>>>> this area ...
>>>
>>>>> ---
>>>>>  arch/x86/pci/acpi.c     |   25 ++++++++++++++++++++++---
>>>>>  drivers/acpi/resource.c |    6 +++++-
>>>>>  2 files changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>>>>> index e4695985f9de..fc2da98985c3 100644
>>>>> --- a/arch/x86/pci/acpi.c
>>>>> +++ b/arch/x86/pci/acpi.c
>>>>> @@ -332,12 +332,32 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>>  {
>>>>>       int ret;
>>>>>       struct resource_entry *entry, *tmp;
>>>>> +     unsigned long res_flags;
>>>>>
>>>>>       sprintf(info->name, "PCI Bus %04x:%02x", domain, busnum);
>>>>>       info->bridge = device;
>>>>> +
>>>>> +     /*
>>>>> +      * An IO or MMIO resource assigned to PCI host bridge may be consumed
>>>>> +      * by the host bridge itself or available to its child bus/devices.
>>>>> +      * On x86 and IA64 platforms, all IO and MMIO resources are assumed to
>>>>> +      * be available to child bus/devices except one special case:
>>>>> +      *      IO port [0xCF8-0xCFF] is consumed by host bridge itself to
>>>>> +      *      access PCI configuration space.
>>>>> +      *
>>>>> +      * Due to lack of specification to define resources consumed by host
>>>>> +      * bridge itself, all IO port resources defined by acpi_resource_io
>>>>> +      * and acpi_resource_fixed_io are ignored to filter out IO
>>>>> +      * port[0xCF8-0xCFF]. Seems this solution works with all BIOSes, though
>>>>> +      * it's not perfect.
>>>
>>> 1) I think it's misleading to say "the specs haven't provided a
>>> method."  As far as I can tell, the Producer/Consumer bit is intended
>>> precisely to distinguish resources consumed by a bridge from those
>>> forwarded to downstream devices.  It would be more accurate to say
>>> "the spec defines a bit, but firmware hasn't used that bit
>>> consistently, so we can't rely on it."
>>
>> Hi Bjorn,
>> 	Thanks for review, I will refine the words as suggested by you.
>>
>>> If you want to say "it's not perfect," it would be useful to mention
>>> the ways in which it is not perfect.  This code is still a candidate
>>> for unification with ia64 and arm64, so we should avoid x86-specific
>>> things here as much as possible.
>>
>> Yes, I have another pending patch set to consolidate IA64 and x86 code
>> for ACPI PCI root.
> 
> That's OK, but can we please fix the regression first before doing that
> unification?  Like to make life easier for the "stable" people and
> whoever wants to backport the fix?

Hi Rafael,
	Yes, I will fix the regression first before sending out the
pending patch set.
Thanks!
Gerry