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

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

[Jiang Liu]

Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
tries to ignore resources consumed by PCI host bridge itself by
checking IORESOURCE_WINDOW flag, which causes regression on some
platforms.

For example, PC Engines APU.1C platform defines PCI MMIO resources with
ACPI Memory32Fixed operator as below:
Name (CRES, ResourceTemplate ()
{
    ...
    WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
        0x0000,             // Granularity
        0x0D00,             // Range Minimum
        0xFFFF,             // Range Maximum
        0x0000,             // Translation Offset
        0xF300,             // Length
        ,, , TypeStatic)
    Memory32Fixed (ReadOnly,
        0x000A0000,         // Address Base
        0x00020000,         // Address Length
        )
    Memory32Fixed (ReadOnly,
        0x00000000,         // Address Base
        0x00000000,         // Address Length
        _Y00)
})

Memory32Fixed operator doesn't support concept of "producer/consumer"
and it will be treated as "consumer" by the ACPI resource parsing
interface, thus cause regression. So the fix is only to check
"producer/consumer" flag for resources having "producer/consumer" flag.

Another possible fix is to only ignore IO resource consumed by host
bridge and keep IOMEM resource consumed by host bridge, please refer to:
http://www.spinics.net/lists/linux-pci/msg39706.html

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>
---
Hi Bernhard,
	Could you please also help to test whether this patch works for
you too?
Thanks!
Gerry
---
 arch/x86/pci/acpi.c     |    5 ++---
 drivers/acpi/resource.c |    3 +++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index e4695985f9de..8c4b1201f340 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
 	info->bridge = device;
 	ret = acpi_dev_get_resources(device, list,
 				     acpi_dev_filter_resource_type_cb,
-				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
+				     (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
 	if (ret < 0)
 		dev_warn(&device->dev,
 			 "failed to parse _CRS method, error code %d\n", ret);
@@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
 			"no IO and memory resources present in _CRS\n");
 	else
 		resource_list_for_each_entry_safe(entry, tmp, list) {
-			if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
-			    (entry->res->flags & IORESOURCE_DISABLED))
+			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
 			else
 				entry->res->name = info->name;
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 5589a6e2a023..b0d3f2ceef06 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -606,6 +606,9 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
 	case ACPI_RESOURCE_TYPE_ADDRESS32:
 	case ACPI_RESOURCE_TYPE_ADDRESS64:
 	case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
+		if (((types & IORESOURCE_WINDOW) == 0) ^
+		    (ares->data.address.producer_consumer == ACPI_CONSUMER))
+			break;
 		if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
 			type = IORESOURCE_MEM;
 		else if (ares->data.address.resource_type == ACPI_IO_RANGE)
-- 
1.7.10.4

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

[Bjorn Helgaas]

On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
> tries to ignore resources consumed by PCI host bridge itself by
> checking IORESOURCE_WINDOW flag, which causes regression on some
> platforms.

"Do.  Or do not.  There is no try."
[http://www.starwars.com/video/do-or-do-not]

That commit doesn't *try* to do something.  It *does* something.  Just
explain what it does and what's wrong with what it does.

> For example, PC Engines APU.1C platform defines PCI MMIO resources with
> ACPI Memory32Fixed operator as below:
> Name (CRES, ResourceTemplate ()
> {
>     ...
>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>         0x0000,             // Granularity
>         0x0D00,             // Range Minimum
>         0xFFFF,             // Range Maximum
>         0x0000,             // Translation Offset
>         0xF300,             // Length
>         ,, , TypeStatic)
>     Memory32Fixed (ReadOnly,
>         0x000A0000,         // Address Base
>         0x00020000,         // Address Length
>         )
>     Memory32Fixed (ReadOnly,
>         0x00000000,         // Address Base
>         0x00000000,         // Address Length
>         _Y00)
> })
> 
> Memory32Fixed operator doesn't support concept of "producer/consumer"
> and it will be treated as "consumer" by the ACPI resource parsing
> interface, thus cause regression. So the fix is only to check
> "producer/consumer" flag for resources having "producer/consumer" flag.

Apparently the problem is with the Memory32Fixed resources above; it sounds
like we ignore them after 63f1789ec716?  I don't quite understand how this
fix works.  acpi_dev_filter_resource_type() has cases for both
ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but 
this patch only touches the latter, not the
ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.

Is it even legal to use Memory32Fixed for a bridge window?  Is this just a
BIOS bug?  If so, how do we know this workaround won't break something
else for BIOSes that use Memory32Fixed correctly?

Should this be a BIOS-specific quirk?

Incidentally, I also noticed this change:

  --- dmesg_3.18.0-rc5.txt        2015-03-23 10:49:25.064682404 -0500
  +++ dmesg_4.0.0-rc4.txt 2015-03-23 10:49:29.276630002 -0500
  -ACPI: PCI Interrupt Link [INTA] (IRQs 3 4 5 7 10 11 12 15) *0, disabled.
  +ACPI: PCI Interrupt Link [INTA] (IRQs 3 4 5 7 10 11 12 15) *0

Is it intentional that INTA was previously reported as disabled but isn't
any more?

And there's also this:

  acpi PNP0A03:00: [Firmware Bug]: no secondary bus range in _CRS

That isn't a change (it was there in 3.18, too), but that really is a
pretty basic BIOS bug and indicates that we shouldn't be too surprised if
it has other bugs.

> Another possible fix is to only ignore IO resource consumed by host
> bridge and keep IOMEM resource consumed by host bridge, please refer to:
> http://www.spinics.net/lists/linux-pci/msg39706.html

It'd be nice to have Bernhard's logs archived somewhere and referenced
here.  This seems like a dusty corner of the code that might have to be
revisited someday.

> 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>
> ---
> Hi Bernhard,
> 	Could you please also help to test whether this patch works for
> you too?
> Thanks!
> Gerry
> ---
>  arch/x86/pci/acpi.c     |    5 ++---
>  drivers/acpi/resource.c |    3 +++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index e4695985f9de..8c4b1201f340 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>  	info->bridge = device;
>  	ret = acpi_dev_get_resources(device, list,
>  				     acpi_dev_filter_resource_type_cb,
> -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
> +				     (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));

Tangent: I'm disappointed that ia64 didn't get reworked to track the x86
code here.  Is that coming soon?

>  	if (ret < 0)
>  		dev_warn(&device->dev,
>  			 "failed to parse _CRS method, error code %d\n", ret);
> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>  			"no IO and memory resources present in _CRS\n");
>  	else
>  		resource_list_for_each_entry_safe(entry, tmp, list) {
> -			if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
> -			    (entry->res->flags & IORESOURCE_DISABLED))
> +			if (entry->res->flags & IORESOURCE_DISABLED)
>  				resource_list_destroy_entry(entry);
>  			else
>  				entry->res->name = info->name;
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 5589a6e2a023..b0d3f2ceef06 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -606,6 +606,9 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>  	case ACPI_RESOURCE_TYPE_ADDRESS32:
>  	case ACPI_RESOURCE_TYPE_ADDRESS64:
>  	case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
> +		if (((types & IORESOURCE_WINDOW) == 0) ^
> +		    (ares->data.address.producer_consumer == ACPI_CONSUMER))
> +			break;
>  		if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
>  			type = IORESOURCE_MEM;
>  		else if (ares->data.address.resource_type == ACPI_IO_RANGE)
> -- 
> 1.7.10.4
> 

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

[Jiang Liu]

On 2015/3/24 0:48, Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
>> tries to ignore resources consumed by PCI host bridge itself by
>> checking IORESOURCE_WINDOW flag, which causes regression on some
>> platforms.
> 
> "Do.  Or do not.  There is no try."
> [http://www.starwars.com/video/do-or-do-not]
> 
> That commit doesn't *try* to do something.  It *does* something.  Just
> explain what it does and what's wrong with what it does.
> 
>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
>> ACPI Memory32Fixed operator as below:
>> Name (CRES, ResourceTemplate ()
>> {
>>     ...
>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>>         0x0000,             // Granularity
>>         0x0D00,             // Range Minimum
>>         0xFFFF,             // Range Maximum
>>         0x0000,             // Translation Offset
>>         0xF300,             // Length
>>         ,, , TypeStatic)
>>     Memory32Fixed (ReadOnly,
>>         0x000A0000,         // Address Base
>>         0x00020000,         // Address Length
>>         )
>>     Memory32Fixed (ReadOnly,
>>         0x00000000,         // Address Base
>>         0x00000000,         // Address Length
>>         _Y00)
>> })
>>
>> Memory32Fixed operator doesn't support concept of "producer/consumer"
>> and it will be treated as "consumer" by the ACPI resource parsing
>> interface, thus cause regression. So the fix is only to check
>> "producer/consumer" flag for resources having "producer/consumer" flag.
> 
> Apparently the problem is with the Memory32Fixed resources above; it sounds
> like we ignore them after 63f1789ec716?  I don't quite understand how this
> fix works.  acpi_dev_filter_resource_type() has cases for both
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but 
> this patch only touches the latter, not the
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
The idea is:
1) caller specifies IORESOURCE_WINDOW to query resources provided
   by the device, otherwise it's querying resources consumed by
   the device.
2) For resource descriptors having producer/consumer flag, such as
    ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
3) For resource descriptors not having producer/consumer flag, such
   as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
   producer/consumer flag.

> 
> Is it even legal to use Memory32Fixed for a bridge window?  Is this just a
> BIOS bug?  If so, how do we know this workaround won't break something
> else for BIOSes that use Memory32Fixed correctly?
> 
> Should this be a BIOS-specific quirk?
I have searched ACPI spec 5.0 and PCI firmware spec 3.1, but haven't
found any statement tells whether Memory32Fixed could be used for
PCI host bridge resources yet. So to be honest, I'm not sure it's
legal or illegal:(

> 
> Incidentally, I also noticed this change:
> 
>   --- dmesg_3.18.0-rc5.txt        2015-03-23 10:49:25.064682404 -0500
>   +++ dmesg_4.0.0-rc4.txt 2015-03-23 10:49:29.276630002 -0500
>   -ACPI: PCI Interrupt Link [INTA] (IRQs 3 4 5 7 10 11 12 15) *0, disabled.
>   +ACPI: PCI Interrupt Link [INTA] (IRQs 3 4 5 7 10 11 12 15) *0
> 
> Is it intentional that INTA was previously reported as disabled but isn't
> any more?
Thanks for reporting this, it's not intentional. Will check it.

> 
> And there's also this:
> 
>   acpi PNP0A03:00: [Firmware Bug]: no secondary bus range in _CRS
> 
> That isn't a change (it was there in 3.18, too), but that really is a
> pretty basic BIOS bug and indicates that we shouldn't be too surprised if
> it has other bugs.
> 
>> Another possible fix is to only ignore IO resource consumed by host
>> bridge and keep IOMEM resource consumed by host bridge, please refer to:
>> http://www.spinics.net/lists/linux-pci/msg39706.html
> 
> It'd be nice to have Bernhard's logs archived somewhere and referenced
> here.  This seems like a dusty corner of the code that might have to be
> revisited someday.
I have archived the acpidump 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>
>> ---
>> Hi Bernhard,
>> 	Could you please also help to test whether this patch works for
>> you too?
>> Thanks!
>> Gerry
>> ---
>>  arch/x86/pci/acpi.c     |    5 ++---
>>  drivers/acpi/resource.c |    3 +++
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index e4695985f9de..8c4b1201f340 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>  	info->bridge = device;
>>  	ret = acpi_dev_get_resources(device, list,
>>  				     acpi_dev_filter_resource_type_cb,
>> -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>> +				     (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> 
> Tangent: I'm disappointed that ia64 didn't get reworked to track the x86
> code here.  Is that coming soon?
I have checked IA64 when changing the resource parsing interface,
but there are obstacle to convert it to the new interface.
Will have another try.
Thanks!
Gerry
> 
>>  	if (ret < 0)
>>  		dev_warn(&device->dev,
>>  			 "failed to parse _CRS method, error code %d\n", ret);
>> @@ -346,8 +346,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>  			"no IO and memory resources present in _CRS\n");
>>  	else
>>  		resource_list_for_each_entry_safe(entry, tmp, list) {
>> -			if ((entry->res->flags & IORESOURCE_WINDOW) == 0 ||
>> -			    (entry->res->flags & IORESOURCE_DISABLED))
>> +			if (entry->res->flags & IORESOURCE_DISABLED)
>>  				resource_list_destroy_entry(entry);
>>  			else
>>  				entry->res->name = info->name;
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 5589a6e2a023..b0d3f2ceef06 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -606,6 +606,9 @@ int acpi_dev_filter_resource_type(struct acpi_resource *ares,
>>  	case ACPI_RESOURCE_TYPE_ADDRESS32:
>>  	case ACPI_RESOURCE_TYPE_ADDRESS64:
>>  	case ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64:
>> +		if (((types & IORESOURCE_WINDOW) == 0) ^
>> +		    (ares->data.address.producer_consumer == ACPI_CONSUMER))
>> +			break;
>>  		if (ares->data.address.resource_type == ACPI_MEMORY_RANGE)
>>  			type = IORESOURCE_MEM;
>>  		else if (ares->data.address.resource_type == ACPI_IO_RANGE)
>> -- 
>> 1.7.10.4
>>

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

[Bjorn Helgaas]

On Mon, Mar 23, 2015 at 9:22 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> On 2015/3/24 0:48, Bjorn Helgaas wrote:
>> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
>>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
>>> tries to ignore resources consumed by PCI host bridge itself by
>>> checking IORESOURCE_WINDOW flag, which causes regression on some
>>> platforms.
>>
>> "Do.  Or do not.  There is no try."
>> [http://www.starwars.com/video/do-or-do-not]
>>
>> That commit doesn't *try* to do something.  It *does* something.  Just
>> explain what it does and what's wrong with what it does.
>>
>>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
>>> ACPI Memory32Fixed operator as below:
>>> Name (CRES, ResourceTemplate ()
>>> {
>>>     ...
>>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>>>         0x0000,             // Granularity
>>>         0x0D00,             // Range Minimum
>>>         0xFFFF,             // Range Maximum
>>>         0x0000,             // Translation Offset
>>>         0xF300,             // Length
>>>         ,, , TypeStatic)
>>>     Memory32Fixed (ReadOnly,
>>>         0x000A0000,         // Address Base
>>>         0x00020000,         // Address Length
>>>         )
>>>     Memory32Fixed (ReadOnly,
>>>         0x00000000,         // Address Base
>>>         0x00000000,         // Address Length
>>>         _Y00)
>>> })
>>>
>>> Memory32Fixed operator doesn't support concept of "producer/consumer"
>>> and it will be treated as "consumer" by the ACPI resource parsing
>>> interface, thus cause regression. So the fix is only to check
>>> "producer/consumer" flag for resources having "producer/consumer" flag.
>>
>> Apparently the problem is with the Memory32Fixed resources above; it sounds
>> like we ignore them after 63f1789ec716?  I don't quite understand how this
>> fix works.  acpi_dev_filter_resource_type() has cases for both
>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but
>> this patch only touches the latter, not the
>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
> The idea is:
> 1) caller specifies IORESOURCE_WINDOW to query resources provided
>    by the device, otherwise it's querying resources consumed by
>    the device.
> 2) For resource descriptors having producer/consumer flag, such as
>     ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
> 3) For resource descriptors not having producer/consumer flag, such
>    as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
>    producer/consumer flag.

I figured out that much by reading the code.  But I think the code is
very hard to read, and I still don't really understand how it works.

Before this fix, we ignore Memory32Fixed resources.  After this fix,
we use Memory32Fixed as a window.  I think that's an incorrect
interpretation of Memory32Fixed.

>> Is it even legal to use Memory32Fixed for a bridge window?  Is this just a
>> BIOS bug?  If so, how do we know this workaround won't break something
>> else for BIOSes that use Memory32Fixed correctly?
>>
>> Should this be a BIOS-specific quirk?
> I have searched ACPI spec 5.0 and PCI firmware spec 3.1, but haven't
> found any statement tells whether Memory32Fixed could be used for
> PCI host bridge resources yet. So to be honest, I'm not sure it's
> legal or illegal:(

I think it could certainly be used for host bridge register space, if
the bridge supplied a _HID that a device-specific driver could claim.
But I think a generic driver like pci_root.c has to rely on the
producer/consumer bit to differentiate windows ("produced" space) from
device registers ("consumed" space), so I think Memory32Fixed for a
window makes no sense.

>> It'd be nice to have Bernhard's logs archived somewhere and referenced
>> here.  This seems like a dusty corner of the code that might have to be
>> revisited someday.
> I have archived the acpidump at:
> https://bugzilla.kernel.org/show_bug.cgi?id=94221

Yes, I noticed that.  Unfortunately, there is no link to the bugzilla
in your changelog.

>>> --- a/arch/x86/pci/acpi.c
>>> +++ b/arch/x86/pci/acpi.c
>>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>      info->bridge = device;
>>>      ret = acpi_dev_get_resources(device, list,
>>>                                   acpi_dev_filter_resource_type_cb,
>>> -                                 (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>> +                                 (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
>>
>> Tangent: I'm disappointed that ia64 didn't get reworked to track the x86
>> code here.  Is that coming soon?
> I have checked IA64 when changing the resource parsing interface,
> but there are obstacle to convert it to the new interface.
> Will have another try.

Please do.  I think it's extremely important to keep these arches
aligned.  And arm64, when similar code gets added to it.  Most of the
code in pci_acpi_scan_root() is actually arch-independent, and it
should be pulled up into pci_root.c someday.

Bjorn

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

[Jiang Liu]

On 2015/3/24 10:42, Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 9:22 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>> On 2015/3/24 0:48, Bjorn Helgaas wrote:
>>> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
>>>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>> tries to ignore resources consumed by PCI host bridge itself by
>>>> checking IORESOURCE_WINDOW flag, which causes regression on some
>>>> platforms.
>>>
>>> "Do.  Or do not.  There is no try."
>>> [http://www.starwars.com/video/do-or-do-not]
>>>
>>> That commit doesn't *try* to do something.  It *does* something.  Just
>>> explain what it does and what's wrong with what it does.
>>>
>>>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
>>>> ACPI Memory32Fixed operator as below:
>>>> Name (CRES, ResourceTemplate ()
>>>> {
>>>>     ...
>>>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>>>>         0x0000,             // Granularity
>>>>         0x0D00,             // Range Minimum
>>>>         0xFFFF,             // Range Maximum
>>>>         0x0000,             // Translation Offset
>>>>         0xF300,             // Length
>>>>         ,, , TypeStatic)
>>>>     Memory32Fixed (ReadOnly,
>>>>         0x000A0000,         // Address Base
>>>>         0x00020000,         // Address Length
>>>>         )
>>>>     Memory32Fixed (ReadOnly,
>>>>         0x00000000,         // Address Base
>>>>         0x00000000,         // Address Length
>>>>         _Y00)
>>>> })
>>>>
>>>> Memory32Fixed operator doesn't support concept of "producer/consumer"
>>>> and it will be treated as "consumer" by the ACPI resource parsing
>>>> interface, thus cause regression. So the fix is only to check
>>>> "producer/consumer" flag for resources having "producer/consumer" flag.
>>>
>>> Apparently the problem is with the Memory32Fixed resources above; it sounds
>>> like we ignore them after 63f1789ec716?  I don't quite understand how this
>>> fix works.  acpi_dev_filter_resource_type() has cases for both
>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but
>>> this patch only touches the latter, not the
>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
>> The idea is:
>> 1) caller specifies IORESOURCE_WINDOW to query resources provided
>>    by the device, otherwise it's querying resources consumed by
>>    the device.
>> 2) For resource descriptors having producer/consumer flag, such as
>>     ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
>> 3) For resource descriptors not having producer/consumer flag, such
>>    as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
>>    producer/consumer flag.
> 
> I figured out that much by reading the code.  But I think the code is
> very hard to read, and I still don't really understand how it works.
> 
> Before this fix, we ignore Memory32Fixed resources.  After this fix,
> we use Memory32Fixed as a window.  I think that's an incorrect
> interpretation of Memory32Fixed.
Hi Bjorn,
	I think it's illegal to use Memory32Fixed for PCI host
bridge resource window too. The fix is to solve the regression
by relaxing constraints, but that may not be the right solution.
	How about waiting for a while to see whether there are more
bug reports related to this. If only limited platform affected,
we could treat it as BIOS bugs and use quirk to handle it.
Otherwise we may need to relax the constraint.
Thanks!
Gerry
> 
>>> Is it even legal to use Memory32Fixed for a bridge window?  Is this just a
>>> BIOS bug?  If so, how do we know this workaround won't break something
>>> else for BIOSes that use Memory32Fixed correctly?
>>>
>>> Should this be a BIOS-specific quirk?
>> I have searched ACPI spec 5.0 and PCI firmware spec 3.1, but haven't
>> found any statement tells whether Memory32Fixed could be used for
>> PCI host bridge resources yet. So to be honest, I'm not sure it's
>> legal or illegal:(
> 
> I think it could certainly be used for host bridge register space, if
> the bridge supplied a _HID that a device-specific driver could claim.
> But I think a generic driver like pci_root.c has to rely on the
> producer/consumer bit to differentiate windows ("produced" space) from
> device registers ("consumed" space), so I think Memory32Fixed for a
> window makes no sense.
> 
>>> It'd be nice to have Bernhard's logs archived somewhere and referenced
>>> here.  This seems like a dusty corner of the code that might have to be
>>> revisited someday.
>> I have archived the acpidump at:
>> https://bugzilla.kernel.org/show_bug.cgi?id=94221
> 
> Yes, I noticed that.  Unfortunately, there is no link to the bugzilla
> in your changelog.
> 
>>>> --- a/arch/x86/pci/acpi.c
>>>> +++ b/arch/x86/pci/acpi.c
>>>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>>>      info->bridge = device;
>>>>      ret = acpi_dev_get_resources(device, list,
>>>>                                   acpi_dev_filter_resource_type_cb,
>>>> -                                 (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>>>> +                                 (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
>>>
>>> Tangent: I'm disappointed that ia64 didn't get reworked to track the x86
>>> code here.  Is that coming soon?
>> I have checked IA64 when changing the resource parsing interface,
>> but there are obstacle to convert it to the new interface.
>> Will have another try.
> 
> Please do.  I think it's extremely important to keep these arches
> aligned.  And arm64, when similar code gets added to it.  Most of the
> code in pci_acpi_scan_root() is actually arch-independent, and it
> should be pulled up into pci_root.c someday.
> 
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Bjorn Helgaas]

On Mon, Mar 23, 2015 at 9:59 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> On 2015/3/24 10:42, Bjorn Helgaas wrote:
>> On Mon, Mar 23, 2015 at 9:22 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>>> On 2015/3/24 0:48, Bjorn Helgaas wrote:
>>>> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
>>>>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
>>>>> tries to ignore resources consumed by PCI host bridge itself by
>>>>> checking IORESOURCE_WINDOW flag, which causes regression on some
>>>>> platforms.
>>>>
>>>> "Do.  Or do not.  There is no try."
>>>> [http://www.starwars.com/video/do-or-do-not]
>>>>
>>>> That commit doesn't *try* to do something.  It *does* something.  Just
>>>> explain what it does and what's wrong with what it does.
>>>>
>>>>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
>>>>> ACPI Memory32Fixed operator as below:
>>>>> Name (CRES, ResourceTemplate ()
>>>>> {
>>>>>     ...
>>>>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>>>>>         0x0000,             // Granularity
>>>>>         0x0D00,             // Range Minimum
>>>>>         0xFFFF,             // Range Maximum
>>>>>         0x0000,             // Translation Offset
>>>>>         0xF300,             // Length
>>>>>         ,, , TypeStatic)
>>>>>     Memory32Fixed (ReadOnly,
>>>>>         0x000A0000,         // Address Base
>>>>>         0x00020000,         // Address Length
>>>>>         )
>>>>>     Memory32Fixed (ReadOnly,
>>>>>         0x00000000,         // Address Base
>>>>>         0x00000000,         // Address Length
>>>>>         _Y00)
>>>>> })
>>>>>
>>>>> Memory32Fixed operator doesn't support concept of "producer/consumer"
>>>>> and it will be treated as "consumer" by the ACPI resource parsing
>>>>> interface, thus cause regression. So the fix is only to check
>>>>> "producer/consumer" flag for resources having "producer/consumer" flag.
>>>>
>>>> Apparently the problem is with the Memory32Fixed resources above; it sounds
>>>> like we ignore them after 63f1789ec716?  I don't quite understand how this
>>>> fix works.  acpi_dev_filter_resource_type() has cases for both
>>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but
>>>> this patch only touches the latter, not the
>>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
>>> The idea is:
>>> 1) caller specifies IORESOURCE_WINDOW to query resources provided
>>>    by the device, otherwise it's querying resources consumed by
>>>    the device.
>>> 2) For resource descriptors having producer/consumer flag, such as
>>>     ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
>>> 3) For resource descriptors not having producer/consumer flag, such
>>>    as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
>>>    producer/consumer flag.
>>
>> I figured out that much by reading the code.  But I think the code is
>> very hard to read, and I still don't really understand how it works.
>>
>> Before this fix, we ignore Memory32Fixed resources.  After this fix,
>> we use Memory32Fixed as a window.  I think that's an incorrect
>> interpretation of Memory32Fixed.
> Hi Bjorn,
>         I think it's illegal to use Memory32Fixed for PCI host
> bridge resource window too. The fix is to solve the regression
> by relaxing constraints, but that may not be the right solution.

Relaxing the constraint for all platforms is only a solution if we're
confident that it works for all platforms.  I'm not confident because
I don't know what effect this patch has on systems that use
Memory32Fixed correctly.

It's possible that you can analyze the behavior and explain that if
you relax the constraint, the Memory32Fixed handling (both for bridge
and non-bridge devices) will be the same as it was before
63f1789ec716.  If you can do that, I think it would be reasonable
because you'd be preserving the previous, known-working behavior.

If you go that route, I'd like to see a patch that explicitly touches
the Memory32Fixed handling.  The current patch claims to change the
way we handle Memory32Fixed, but nothing in the code change is
directly related to Memory32Fixed, so it's very confusing.

>         How about waiting for a while to see whether there are more
> bug reports related to this. If only limited platform affected,
> we could treat it as BIOS bugs and use quirk to handle it.
> Otherwise we may need to relax the constraint.

I don't think waiting is a good strategy.  We know we have a
regression, and I think we need to fix that ASAP without waiting for
more failure reports.

Bjorn

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

[Rafael J. Wysocki]

On Tuesday, March 24, 2015 08:18:35 AM Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 9:59 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> > On 2015/3/24 10:42, Bjorn Helgaas wrote:
> >> On Mon, Mar 23, 2015 at 9:22 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> >>> On 2015/3/24 0:48, Bjorn Helgaas wrote:
> >>>> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
> >>>>> Commit 63f1789ec716("Ignore resources consumed by host bridge itself")
> >>>>> tries to ignore resources consumed by PCI host bridge itself by
> >>>>> checking IORESOURCE_WINDOW flag, which causes regression on some
> >>>>> platforms.
> >>>>
> >>>> "Do.  Or do not.  There is no try."
> >>>> [http://www.starwars.com/video/do-or-do-not]
> >>>>
> >>>> That commit doesn't *try* to do something.  It *does* something.  Just
> >>>> explain what it does and what's wrong with what it does.
> >>>>
> >>>>> For example, PC Engines APU.1C platform defines PCI MMIO resources with
> >>>>> ACPI Memory32Fixed operator as below:
> >>>>> Name (CRES, ResourceTemplate ()
> >>>>> {
> >>>>>     ...
> >>>>>     WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> >>>>>         0x0000,             // Granularity
> >>>>>         0x0D00,             // Range Minimum
> >>>>>         0xFFFF,             // Range Maximum
> >>>>>         0x0000,             // Translation Offset
> >>>>>         0xF300,             // Length
> >>>>>         ,, , TypeStatic)
> >>>>>     Memory32Fixed (ReadOnly,
> >>>>>         0x000A0000,         // Address Base
> >>>>>         0x00020000,         // Address Length
> >>>>>         )
> >>>>>     Memory32Fixed (ReadOnly,
> >>>>>         0x00000000,         // Address Base
> >>>>>         0x00000000,         // Address Length
> >>>>>         _Y00)
> >>>>> })
> >>>>>
> >>>>> Memory32Fixed operator doesn't support concept of "producer/consumer"
> >>>>> and it will be treated as "consumer" by the ACPI resource parsing
> >>>>> interface, thus cause regression. So the fix is only to check
> >>>>> "producer/consumer" flag for resources having "producer/consumer" flag.
> >>>>
> >>>> Apparently the problem is with the Memory32Fixed resources above; it sounds
> >>>> like we ignore them after 63f1789ec716?  I don't quite understand how this
> >>>> fix works.  acpi_dev_filter_resource_type() has cases for both
> >>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 and ACPI_RESOURCE_TYPE_ADDRESSxx, but
> >>>> this patch only touches the latter, not the
> >>>> ACPI_RESOURCE_TYPE_FIXED_MEMORY32 case.
> >>> The idea is:
> >>> 1) caller specifies IORESOURCE_WINDOW to query resources provided
> >>>    by the device, otherwise it's querying resources consumed by
> >>>    the device.
> >>> 2) For resource descriptors having producer/consumer flag, such as
> >>>     ACPI_RESOURCE_TYPE_ADDRESSxx, we check the producer/consumer flag.
> >>> 3) For resource descriptors not having producer/consumer flag, such
> >>>    as ACPI_RESOURCE_TYPE_FIXED_MEMORY32, we skip checking the
> >>>    producer/consumer flag.
> >>
> >> I figured out that much by reading the code.  But I think the code is
> >> very hard to read, and I still don't really understand how it works.
> >>
> >> Before this fix, we ignore Memory32Fixed resources.  After this fix,
> >> we use Memory32Fixed as a window.  I think that's an incorrect
> >> interpretation of Memory32Fixed.
> > Hi Bjorn,
> >         I think it's illegal to use Memory32Fixed for PCI host
> > bridge resource window too. The fix is to solve the regression
> > by relaxing constraints, but that may not be the right solution.
> 
> Relaxing the constraint for all platforms is only a solution if we're
> confident that it works for all platforms.  I'm not confident because
> I don't know what effect this patch has on systems that use
> Memory32Fixed correctly.
> 
> It's possible that you can analyze the behavior and explain that if
> you relax the constraint, the Memory32Fixed handling (both for bridge
> and non-bridge devices) will be the same as it was before
> 63f1789ec716.  If you can do that, I think it would be reasonable
> because you'd be preserving the previous, known-working behavior.
> 
> If you go that route, I'd like to see a patch that explicitly touches
> the Memory32Fixed handling.  The current patch claims to change the
> way we handle Memory32Fixed, but nothing in the code change is
> directly related to Memory32Fixed, so it's very confusing.
> 
> >         How about waiting for a while to see whether there are more
> > bug reports related to this. If only limited platform affected,
> > we could treat it as BIOS bugs and use quirk to handle it.
> > Otherwise we may need to relax the constraint.
> 
> I don't think waiting is a good strategy.  We know we have a
> regression, and I think we need to fix that ASAP without waiting for
> more failure reports.

Agreed on all points.

Rafael

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

[Jiang Liu]

On 2015/3/24 0:48, Bjorn Helgaas wrote:
> On Mon, Mar 23, 2015 at 03:22:14PM +0800, Jiang Liu wrote:
<snit>
>> ---
>>  arch/x86/pci/acpi.c     |    5 ++---
>>  drivers/acpi/resource.c |    3 +++
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index e4695985f9de..8c4b1201f340 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -337,7 +337,7 @@ static void probe_pci_root_info(struct pci_root_info *info,
>>  	info->bridge = device;
>>  	ret = acpi_dev_get_resources(device, list,
>>  				     acpi_dev_filter_resource_type_cb,
>> -				     (void *)(IORESOURCE_IO | IORESOURCE_MEM));
>> +				     (void *)(IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_WINDOW));
> 
> Tangent: I'm disappointed that ia64 didn't get reworked to track the x86
> code here.  Is that coming soon?
Hi Bjorn,
	Recall the memory now, IA64 may have mutiple 64K IO space, which
also support SPARSE attribute. Currently ACPI resource parsing interface
have no support of these two requirements yet. Will try to improve it.
Thanks!
Gerry

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

[Bernhard Thaler]

I tested this patch applied to 4.0-rc5. APU.1C network interfaces are
working correctly again.