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(-)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fd248a3..aa47be7 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 5fe7502..9645290 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -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;
}
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 40e94d5..a65c2ad 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -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;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f39d894..08b9af0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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,
--
1.7.0.4
On 30.06.2011 10:09, Ram Pai wrote:
> 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
>
> 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(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index fd248a3..aa47be7 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -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.
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 5fe7502..9645290 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -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;
> }
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 40e94d5..a65c2ad 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -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;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f39d894..08b9af0 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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,
On Thu, 30 Jun 2011 19:04:55 +0200
Oliver Hartkopp <[email protected]> wrote:
> On 30.06.2011 10:09, Ram Pai wrote:
> > 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 ...
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.
--
Jesse Barnes, Intel Open Source Technology Center
On 30.06.2011 19:07, Jesse Barnes wrote:
> On Thu, 30 Jun 2011 19:04:55 +0200
> Oliver Hartkopp <[email protected]> wrote:
>
>> On 30.06.2011 10:09, Ram Pai wrote:
>>> 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 ...
>
> 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
On Thu, Jun 30, 2011 at 07:12:38PM +0200, Oliver Hartkopp wrote:
> On 30.06.2011 19:07, Jesse Barnes wrote:
> > On Thu, 30 Jun 2011 19:04:55 +0200
> > Oliver Hartkopp <[email protected]> wrote:
> >
> >> On 30.06.2011 10:09, Ram Pai wrote:
> >>> 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 ...
> >
> > 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.
RP
On Thu, Jun 30, 2011 at 12:38 PM, Ram Pai <[email protected]> wrote:
> On Thu, Jun 30, 2011 at 07:12:38PM +0200, Oliver Hartkopp wrote:
>> On 30.06.2011 19:07, Jesse Barnes wrote:
>> > On Thu, 30 Jun 2011 19:04:55 +0200
>> > Oliver Hartkopp <[email protected]> wrote:
>> >
>> >> On 30.06.2011 10:09, Ram Pai wrote:
>> >>> 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 ...
>> >
>> > 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?"
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
On Thu, 30 Jun 2011 14:24:24 -0600
Bjorn Helgaas <[email protected]> wrote:
> On Thu, Jun 30, 2011 at 12:38 PM, Ram Pai <[email protected]> wrote:
> > On Thu, Jun 30, 2011 at 07:12:38PM +0200, Oliver Hartkopp wrote:
> >> On 30.06.2011 19:07, Jesse Barnes wrote:
> >> > On Thu, 30 Jun 2011 19:04:55 +0200
> >> > Oliver Hartkopp <[email protected]> wrote:
> >> >
> >> >> On 30.06.2011 10:09, Ram Pai wrote:
> >> >>> 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 ...
> >> >
> >> > 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?"
>
> 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.
--
Jesse Barnes, Intel Open Source Technology Center