| Message ID | 1427095334-7430-1-git-send-email-jiang.liu@linux.intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" 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/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)
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(-)