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

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

[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 <linuxram@us.ibm.com>
---
 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Oliver Hartkopp]

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 <linuxram@us.ibm.com>
> ---
>  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,

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Jesse Barnes]

On Thu, 30 Jun 2011 19:04:55 +0200
Oliver Hartkopp <socketcan@hartkopp.net> 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Oliver Hartkopp]

On 30.06.2011 19:07, Jesse Barnes wrote:
> On Thu, 30 Jun 2011 19:04:55 +0200
> Oliver Hartkopp <socketcan@hartkopp.net> 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Ram Pai]

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 <socketcan@hartkopp.net> 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Bjorn Helgaas]

On Thu, Jun 30, 2011 at 12:38 PM, Ram Pai <linuxram@us.ibm.com> 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 <socketcan@hartkopp.net> 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Jesse Barnes]

On Thu, 30 Jun 2011 14:24:24 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Thu, Jun 30, 2011 at 12:38 PM, Ram Pai <linuxram@us.ibm.com> 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 <socketcan@hartkopp.net> 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

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Ram Pai]

On Thu, Jun 30, 2011 at 01:09:39AM -0700, 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.

> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  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.

Linus/Jesse,

	Are we merging this patch in 3.0.0 ?

RP

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Jesse Barnes]

On Wed, 6 Jul 2011 02:06:38 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Thu, Jun 30, 2011 at 01:09:39AM -0700, 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.
> 
> > 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  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.
> 
> 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,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PCI: conditional resource-reallocation through kernel parameter pci=realloc

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

Hi Jesse,

On Wed, 6 Jul 2011 09:07:33 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> On Wed, 6 Jul 2011 02:06:38 -0700
> Ram Pai <linuxram@us.ibm.com> wrote:
> 
> > On Thu, Jun 30, 2011 at 01:09:39AM -0700, 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.
> > 
> > > 
> > > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > > ---
> > >  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.
> > 
> > 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.

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

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]