PCI: conditional resource-reallocation through kernel parameter pci=realloc

PATCH In review Ram Pai 5 participants 10 messages Jun 30, 2011 – Jul 7, 2011

Subsystems (3)

Documentation x86 PCI

lore

Share & tools

lore

Ram Pai

author

2011-06-30 08:09 UTC #0 lore raw

PATCH 1/1 PCI: conditional resource-reallocation through kernel parameter pci=realloc

Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.

This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.

Signed-off-by Ram Pai <[email protected]>

---
 Documentation/kernel-parameters.txt |    2 ++
 arch/x86/pci/common.c               |    4 ++++
 drivers/pci/setup-bus.c             |    7 +++++++
 include/linux/pci.h                 |    2 ++
 4 files changed, 15 insertions(+), 0 deletions(-)

Documentation/kernel-parameters.txt +2 -0

index fd248a3..aa47be7 100644
@@ -2015,6 +2015,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
+		realloc		reallocate PCI resources if allocations done by BIOS
+				are erroneous.
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

arch/x86/pci/common.c +4 -0

index 5fe7502..9645290 100644
@@ -31,6 +31,7 @@ int noioapicreroute = 0;
 int noioapicreroute = 1;
 #endif
 int pcibios_last_bus = -1;
+int pci_realloc = 0;
 unsigned long pirq_table_addr;
 struct pci_bus *pci_root_bus;
 struct pci_raw_ops *raw_pci_ops;

@@ -602,6 +603,9 @@ char * __devinit  pcibios_setup(char *str)
 		if (noioapicreroute != -1)
 			noioapicreroute = 1;
 		return NULL;
+	} else if (!strcmp(str, "realloc")) {
+		pci_realloc = 1;
+		return NULL;
 	}
 	return str;
 }

drivers/pci/setup-bus.c +7 -0

index 40e94d5..a65c2ad 100644
@@ -1135,6 +1135,13 @@ again:
 	/* any device complain? */
 	if (!head.next)
 		goto enable_and_dump;
+
+	/* don't realloc if asked to do so */
+	if (!pci_realloc) {
+		free_list(resource_list_x, &head);
+		goto enable_and_dump;
+	}
+
 	failed_type = 0;
 	for (list = head.next; list;) {
 		failed_type |= list->flags;

include/linux/pci.h +2 -0

index f39d894..08b9af0 100644
@@ -1427,6 +1427,8 @@ extern u8 pci_cache_line_size;
 extern unsigned long pci_hotplug_io_size;
 extern unsigned long pci_hotplug_mem_size;
 
+extern int pci_realloc;
+
 int pcibios_add_platform_entries(struct pci_dev *dev);
 void pcibios_disable_device(struct pci_dev *dev);
 int pcibios_set_pcie_reset_state(struct pci_dev *dev,

Signature · 1 line

1.7.0.4

Oliver Hartkopp

2011-06-30 17:05 UTC #1 lore raw

Quoted 7 lines · Ram Pai

Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.

What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?

A kernel parameter needs to be observed, documented and set appropriately.

I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...

Btw. why don't you post your fix "[PATCH 0/4 v2] PCI: fix cardbus and sriov regressions" also on LKML that works great on my problematic laptop?

Regards,
Oliver

Quoted 75 lines

Signed-off-by Ram Pai <[email protected]>

---
 Documentation/kernel-parameters.txt |    2 ++
 arch/x86/pci/common.c               |    4 ++++
 drivers/pci/setup-bus.c             |    7 +++++++
 include/linux/pci.h                 |    2 ++
 4 files changed, 15 insertions(+), 0 deletions(-)

Documentation/kernel-parameters.txt +2 -0

index fd248a3..aa47be7 100644
@@ -2015,6 +2015,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
+		realloc		reallocate PCI resources if allocations done by BIOS
+				are erroneous.
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

arch/x86/pci/common.c +4 -0

index 5fe7502..9645290 100644
@@ -31,6 +31,7 @@ int noioapicreroute = 0;
 int noioapicreroute = 1;
 #endif
 int pcibios_last_bus = -1;
+int pci_realloc = 0;
 unsigned long pirq_table_addr;
 struct pci_bus *pci_root_bus;
 struct pci_raw_ops *raw_pci_ops;

@@ -602,6 +603,9 @@ char * __devinit  pcibios_setup(char *str)
 		if (noioapicreroute != -1)
 			noioapicreroute = 1;
 		return NULL;
+	} else if (!strcmp(str, "realloc")) {
+		pci_realloc = 1;
+		return NULL;
 	}
 	return str;
 }

drivers/pci/setup-bus.c +7 -0

index 40e94d5..a65c2ad 100644
@@ -1135,6 +1135,13 @@ again:
 	/* any device complain? */
 	if (!head.next)
 		goto enable_and_dump;
+
+	/* don't realloc if asked to do so */
+	if (!pci_realloc) {
+		free_list(resource_list_x, &head);
+		goto enable_and_dump;
+	}
+
 	failed_type = 0;
 	for (list = head.next; list;) {
 		failed_type |= list->flags;

include/linux/pci.h +2 -0

index f39d894..08b9af0 100644
@@ -1427,6 +1427,8 @@ extern u8 pci_cache_line_size;
 extern unsigned long pci_hotplug_io_size;
 extern unsigned long pci_hotplug_mem_size;
 
+extern int pci_realloc;
+
 int pcibios_add_platform_entries(struct pci_dev *dev);
 void pcibios_disable_device(struct pci_dev *dev);
 int pcibios_set_pcie_reset_state(struct pci_dev *dev,

Jesse Barnes

2011-06-30 17:07 UTC #2 lore raw

Quoted 9 lines · On Thu, 30 Jun 2011 19:04:55 +0200 Oliver Hartkopp <[email protected]>…

Quoted 7 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.

Quoted 6 lines

What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?
A kernel parameter needs to be observed, documented and set appropriately.
I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...

Yeah, we all want an automatic solution, but we still haven't been able to achieve one. My hope is that a parameter will let us keep the code upstream for Ram and others to keep fixing, then we can move to using it by default in some future release. Keeping the code upstream but behind a param should make development easier; at least that's the goal.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Oliver Hartkopp

2011-06-30 17:12 UTC #3 lore raw

Quoted 17 lines · Jesse Barnes

Quoted 14 lines · On Thu, 30 Jun 2011 19:04:55 +0200 Oliver Hartkopp <[email protected]>…
Quoted 7 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?
A kernel parameter needs to be observed, documented and set appropriately.

Quoted 7 lines

Quoted 1 line
I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...
Yeah, we all want an automatic solution, but we still haven't been able to achieve one. My hope is that a parameter will let us keep the code upstream for Ram and others to keep fixing, then we can move to using it by default in some future release. Keeping the code upstream but behind a param should make development easier; at least that's the goal.

What's wrong with the "[PATCH 0/4 v2] PCI: fix cardbus and sriov regressions"? To me it looked good - or don't you trust that fix right now?

Regards,
Oliver

Ram Pai

author

2011-06-30 18:38 UTC #4 lore raw

Quoted 20 lines · Oliver Hartkopp

Quoted 19 lines · Jesse Barnes
Quoted 15 lines · On Thu, 30 Jun 2011 19:04:55 +0200 Oliver Hartkopp <[email protected]>…
Quoted 7 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?
A kernel parameter needs to be observed, documented and set appropriately.
I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...

Quoted 8 lines

Quoted 5 lines
Yeah, we all want an automatic solution, but we still haven't been able to achieve one. My hope is that a parameter will let us keep the code upstream for Ram and others to keep fixing, then we can move to using it by default in some future release. Keeping the code upstream but behind a param should make development easier; at least that's the goal.
What's wrong with the "[PATCH 0/4 v2] PCI: fix cardbus and sriov regressions"? To me it looked good - or don't you trust that fix right now?

I trust the fix :).

Linus's concern was the wrong alignment, which I have fixed, but yet to resend the patchset. Will do today.

However Linus's other concern was "too late for 3.0.0, for such a large patch".

There is the other concern about "should cardbus resources be treated nice-to-have?"

Also we dont know yet, what other platforms we have broken in some unique way.

Bjorn Helgaas

maintainer

2011-06-30 20:24 UTC #5 lore raw

Quoted 30 lines · Ram Pai

Quoted 20 lines · Oliver Hartkopp
Quoted 19 lines · Jesse Barnes
Quoted 15 lines · On Thu, 30 Jun 2011 19:04:55 +0200 Oliver Hartkopp <[email protected]>…
Quoted 7 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?
A kernel parameter needs to be observed, documented and set appropriately.
I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...
Quoted 8 lines
Quoted 5 lines
Yeah, we all want an automatic solution, but we still haven't been able to achieve one. My hope is that a parameter will let us keep the code upstream for Ram and others to keep fixing, then we can move to using it by default in some future release. Keeping the code upstream but behind a param should make development easier; at least that's the goal.
What's wrong with the "[PATCH 0/4 v2] PCI: fix cardbus and sriov regressions"? To me it looked good - or don't you trust that fix right now?

Quoted 8 lines

I trust the fix :).
Linus's concern was the wrong alignment, which I have fixed, but yet to resend the patchset. Will do today.
However Linus's other concern was "too late for 3.0.0, for such a large patch".
There is the other concern about "should cardbus resources be treated nice-to-have?"

Somewhere along the way, can we get rid of the awkward "nice-to-have" language? I think "optional" conveys most of the intended meaning, perhaps lacking the shade that "we'll allocate them if we can." But the important part is that they are not *required*, and "optional" is a nice antonym for "required."

Bjorn

Jesse Barnes

2011-06-30 20:26 UTC #6 lore raw

Quoted 38 lines · On Thu, 30 Jun 2011 14:24:24 -0600 Bjorn Helgaas <[email protected]> wrote:

Quoted 37 lines · Ram Pai
Quoted 20 lines · Oliver Hartkopp
Quoted 19 lines · Jesse Barnes
Quoted 15 lines · On Thu, 30 Jun 2011 19:04:55 +0200 Oliver Hartkopp <[email protected]>…
Quoted 7 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
What is the advantage of creating an 'interim' kernel parameter instead of reverting the problematic commit and queue up a proper solution for 3.1 ?
A kernel parameter needs to be observed, documented and set appropriately.
I would prefer to have an automatic solution - if not in 3.0 then in 3.1 ...
Quoted 8 lines
Quoted 5 lines
Yeah, we all want an automatic solution, but we still haven't been able to achieve one. My hope is that a parameter will let us keep the code upstream for Ram and others to keep fixing, then we can move to using it by default in some future release. Keeping the code upstream but behind a param should make development easier; at least that's the goal.
What's wrong with the "[PATCH 0/4 v2] PCI: fix cardbus and sriov regressions"? To me it looked good - or don't you trust that fix right now?
I trust the fix :).
Linus's concern was the wrong alignment, which I have fixed, but yet to resend the patchset. Will do today.
However Linus's other concern was "too late for 3.0.0, for such a large patch".

Quoted 7 lines

Quoted 1 line
There is the other concern about "should cardbus resources be treated nice-to-have?"
Somewhere along the way, can we get rid of the awkward "nice-to-have" language? I think "optional" conveys most of the intended meaning, perhaps lacking the shade that "we'll allocate them if we can." But the important part is that they are not *required*, and "optional" is a nice antonym for "required."

I think there's a lot more that could be cleaned up in the re-alloc code; e.g. add_head isn't very descriptive as a way to pass around resources that we're tracking for potential re-allocation.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Ram Pai

author

2011-07-06 09:07 UTC #7 lore raw

In reply to Ram Pai

Quoted 22 lines · Ram Pai

Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
Signed-off-by Ram Pai <[email protected]>
---
 Documentation/kernel-parameters.txt |    2 ++
 arch/x86/pci/common.c               |    4 ++++
 drivers/pci/setup-bus.c             |    7 +++++++
 include/linux/pci.h                 |    2 ++
 4 files changed, 15 insertions(+), 0 deletions(-)
Documentation/kernel-parameters.txt +0 -0
index fd248a3..aa47be7 100644

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 2022

Quoted 8 lines

Documentation/kernel-parameters.txt +2 -0

@@ -2015,6 +2015,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
+		realloc		reallocate PCI resources if allocations done by BIOS
+				are erroneous.
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.

Linus/Jesse,

Are we merging this patch in 3.0.0 ?

Jesse Barnes

2011-07-06 16:07 UTC #8 lore raw

Quoted 29 lines · On Wed, 6 Jul 2011 02:06:38 -0700 Ram Pai <[email protected]> wrote:

Quoted 28 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
Signed-off-by Ram Pai <[email protected]>
---
 Documentation/kernel-parameters.txt |    2 ++
 arch/x86/pci/common.c               |    4 ++++
 drivers/pci/setup-bus.c             |    7 +++++++
 include/linux/pci.h                 |    2 ++
 4 files changed, 15 insertions(+), 0 deletions(-)
Documentation/kernel-parameters.txt +2 -0
index fd248a3..aa47be7 100644
@@ -2015,6 +2015,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
+		realloc		reallocate PCI resources if allocations done by BIOS
+				are erroneous.
 

Quoted 6 lines

Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 2022
Quoted 2 lines
Documentation/kernel-parameters.txt +0 -0
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
Linus/Jesse,
Are we merging this patch in 3.0.0 ?

Yeah I think we probably should, since the alternative is still pretty large.

I'll queue it up with any other small patches that are needed at this point and send them over to Linus.

Thanks,

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Stephen Rothwell

2011-07-07 01:00 UTC #9 lore raw

Hi Jesse,

Quoted 38 lines · On Wed, 6 Jul 2011 09:07:33 -0700 Jesse Barnes <[email protected]> wrote:

Quoted 34 lines · On Wed, 6 Jul 2011 02:06:38 -0700 Ram Pai <[email protected]> wrote:
Quoted 22 lines · Ram Pai
Multiple attempts to dynamically reallocate pci resources have unfortunately lead to regressions. Though we continue to fix the regressions and fine tune the dynamic-reallocation behavior, we have not reached a acceptable state yet.
This patch provides a interim solution. It disables dynamic-reallocation; by default, with the ability to enable it through pci=realloc kernel command line parameter.
Signed-off-by Ram Pai <[email protected]>
---
 Documentation/kernel-parameters.txt |    2 ++
 arch/x86/pci/common.c               |    4 ++++
 drivers/pci/setup-bus.c             |    7 +++++++
 include/linux/pci.h                 |    2 ++
 4 files changed, 15 insertions(+), 0 deletions(-)
Documentation/kernel-parameters.txt +0 -0
index fd248a3..aa47be7 100644
Documentation/kernel-parameters.txt bytes respectively. Such letter suffixes can also be entirely omitted. · line 2022
Quoted 8 lines
Documentation/kernel-parameters.txt +2 -0
@@ -2015,6 +2015,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the default.
 				off: Turn ECRC off
 				on: Turn ECRC on.
+		realloc		reallocate PCI resources if allocations done by BIOS
+				are erroneous.
 
 	pcie_aspm=	[PCIE] Forcibly enable or disable PCIe Active State Power
 			Management.
Linus/Jesse,

Quoted 7 lines

Quoted 1 line
Are we merging this patch in 3.0.0 ?
Yeah I think we probably should, since the alternative is still pretty large.
I'll queue it up with any other small patches that are needed at this point and send them over to Linus.

It breaks the powerpc build (see my other message).

Signature · 3 lines

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/