diff mbox

[Alternative,2] ACPI / PCI: Set root bridge ACPI handle in advance

Message ID 20130104010038.GA11155@google.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Jan. 4, 2013, 1 a.m. UTC 
On Thu, Jan 03, 2013 at 11:56:55PM +0100, Rafael J. Wysocki wrote:

> OK, I now have sent no less than three working version of the patch that fixes
> the current code which _is_ insane.  You haven't even responded to the last
> one, but for the first two the reason why you didn't like them was something
> similar to "it may conflict with some future changes I'm planning".  Well,
> that might be used to reject prety much any change and I'm not considering it
> as a good enough reason for blocking a fix.  Sorry about that.

I think your memory is faulty.  My response to the first
(https://lkml.org/lkml/2012/12/20/407) was "Thanks for cleaning this up, I
have an interface concern, here's an outline of a possible alternative."

My response to the second (https://lkml.org/lkml/2012/12/26/72) was "I like
this much better, Acked-by: Bjorn Helgaas."  Then Yinghai noticed the issue
with non-ACPI host bridges, and you abandoned that approach.

I took a few days of vacation, then spent the better part of yesterday
exploring the reasons why x86 and ia64 don't use the "parent" argument when
several other arches do, and worked up a patch
(https://lkml.org/lkml/2013/1/2/285).  It turned out to have a fatal flaw,
but was done in good faith.

It's true I haven't responded to the third one, posted about 12 hours ago.

I still like the approach of the second patch.  What would you think
of the following incremental change to it?  I did reproduce Yinghai's
issue with non-ACPI host bridges, and this change resolves it for me.


--
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

Comments

Rafael J. Wysocki Jan. 4, 2013, 11:38 a.m. UTC | #1 
On Thursday, January 03, 2013 06:00:38 PM Bjorn Helgaas wrote:
> On Thu, Jan 03, 2013 at 11:56:55PM +0100, Rafael J. Wysocki wrote:
> 
> > OK, I now have sent no less than three working version of the patch that fixes
> > the current code which _is_ insane.  You haven't even responded to the last
> > one, but for the first two the reason why you didn't like them was something
> > similar to "it may conflict with some future changes I'm planning".  Well,
> > that might be used to reject prety much any change and I'm not considering it
> > as a good enough reason for blocking a fix.  Sorry about that.
> 
> I think your memory is faulty.  My response to the first
> (https://lkml.org/lkml/2012/12/20/407) was "Thanks for cleaning this up, I
> have an interface concern, here's an outline of a possible alternative."
> 
> My response to the second (https://lkml.org/lkml/2012/12/26/72) was "I like
> this much better, Acked-by: Bjorn Helgaas."  Then Yinghai noticed the issue
> with non-ACPI host bridges, and you abandoned that approach.

I thought it was helpelss, but I was clearly wrong.  I should have spent more
time on figuring out why it failed, so thank for taking the time to do that.

> I took a few days of vacation, then spent the better part of yesterday
> exploring the reasons why x86 and ia64 don't use the "parent" argument when
> several other arches do, and worked up a patch
> (https://lkml.org/lkml/2013/1/2/285).  It turned out to have a fatal flaw,
> but was done in good faith.

I know.

> It's true I haven't responded to the third one, posted about 12 hours ago.

Oh, that's a simple patch. ;-)

But you're right, I should be more patient.  Sorry about that.

> I still like the approach of the second patch.  What would you think
> of the following incremental change to it?

I'm fine with it.

> I did reproduce Yinghai's
> issue with non-ACPI host bridges, and this change resolves it for me.

If you don't mind, I'll fold the patch below into

https://patchwork.kernel.org/patch/1910181/

and post the complete patch.

Thanks,
Rafael


> diff -u b/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> --- b/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -522,6 +522,7 @@
>  	sd = &info->sd;
>  	sd->domain = domain;
>  	sd->node = node;
> +	sd->acpi_handle = device->handle;
>  	/*
>  	 * Maybe the desired pci bus has been already scanned. In such case
>  	 * it is unnecessary to scan the pci bus with the given domain,busnum.
> @@ -596,9 +597,8 @@
>  int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>  {
>  	struct pci_sysdata *sd = bridge->bus->sysdata;
> -	struct pci_root_info *info = container_of(sd, struct pci_root_info, sd);
>  
> -	ACPI_HANDLE_SET(&bridge->dev, info->bridge->handle);
> +	ACPI_HANDLE_SET(&bridge->dev, sd->acpi_handle);
>  	return 0;
>  }
>  
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -14,6 +14,7 @@
>  struct pci_sysdata {
>  	int		domain;		/* PCI domain */
>  	int		node;		/* NUMA node */
> +	void		*acpi_handle;
>  #ifdef CONFIG_X86_64
>  	void		*iommu;		/* IOMMU private data */
>  #endif
>
Yinghai Lu Jan. 5, 2013, 12:03 a.m. UTC | #2 
On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> --- a/arch/x86/include/asm/pci.h
>> +++ b/arch/x86/include/asm/pci.h
>> @@ -14,6 +14,7 @@
>>  struct pci_sysdata {
>>       int             domain;         /* PCI domain */
>>       int             node;           /* NUMA node */
>> +     void            *acpi_handle;
>>  #ifdef CONFIG_X86_64
>>       void            *iommu;         /* IOMMU private data */
>>  #endif
>>

acpi_handle is not good name and it is confusing.
--
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
Rafael J. Wysocki Jan. 5, 2013, 12:14 a.m. UTC | #3 
On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> --- a/arch/x86/include/asm/pci.h
> >> +++ b/arch/x86/include/asm/pci.h
> >> @@ -14,6 +14,7 @@
> >>  struct pci_sysdata {
> >>       int             domain;         /* PCI domain */
> >>       int             node;           /* NUMA node */
> >> +     void            *acpi_handle;
> >>  #ifdef CONFIG_X86_64
> >>       void            *iommu;         /* IOMMU private data */
> >>  #endif
> >>
> 
> acpi_handle is not good name and it is confusing.

Well, what would be a better name in your opinion?

I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
calling it acpi_data?

Rafael
Yinghai Lu Jan. 5, 2013, 12:19 a.m. UTC | #4 
On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> --- a/arch/x86/include/asm/pci.h
>> >> +++ b/arch/x86/include/asm/pci.h
>> >> @@ -14,6 +14,7 @@
>> >>  struct pci_sysdata {
>> >>       int             domain;         /* PCI domain */
>> >>       int             node;           /* NUMA node */
>> >> +     void            *acpi_handle;
>> >>  #ifdef CONFIG_X86_64
>> >>       void            *iommu;         /* IOMMU private data */
>> >>  #endif
>> >>
>>
>> acpi_handle is not good name and it is confusing.
>
> Well, what would be a better name in your opinion?
>
> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
> calling it acpi_data?

yes, with #ifdef, you can use acpi_handle type directly.

it is acpi handle for pci_root.

so would call int pci_root_acpi_handle ?

Thanks

Yinghai
--
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
Bjorn Helgaas Jan. 5, 2013, 12:36 a.m. UTC | #5 
On Fri, Jan 4, 2013 at 5:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
>>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> >> --- a/arch/x86/include/asm/pci.h
>>> >> +++ b/arch/x86/include/asm/pci.h
>>> >> @@ -14,6 +14,7 @@
>>> >>  struct pci_sysdata {
>>> >>       int             domain;         /* PCI domain */
>>> >>       int             node;           /* NUMA node */
>>> >> +     void            *acpi_handle;
>>> >>  #ifdef CONFIG_X86_64
>>> >>       void            *iommu;         /* IOMMU private data */
>>> >>  #endif
>>> >>
>>>
>>> acpi_handle is not good name and it is confusing.
>>
>> Well, what would be a better name in your opinion?
>>
>> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
>> calling it acpi_data?
>
> yes, with #ifdef, you can use acpi_handle type directly.
>
> it is acpi handle for pci_root.
>
> so would call int pci_root_acpi_handle ?

I just copied the name from the corresponding ia64 code.  I don't care
if you want to change it, but I think there is *some* value in keeping
the x86 and ia64 code as similar as possible because it would be nice
to converge it some day.

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
Rafael J. Wysocki Jan. 5, 2013, 12:54 a.m. UTC | #6 
On Friday, January 04, 2013 05:36:55 PM Bjorn Helgaas wrote:
> On Fri, Jan 4, 2013 at 5:19 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Fri, Jan 4, 2013 at 4:14 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Friday, January 04, 2013 04:03:01 PM Yinghai Lu wrote:
> >>> On Fri, Jan 4, 2013 at 3:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>> >> --- a/arch/x86/include/asm/pci.h
> >>> >> +++ b/arch/x86/include/asm/pci.h
> >>> >> @@ -14,6 +14,7 @@
> >>> >>  struct pci_sysdata {
> >>> >>       int             domain;         /* PCI domain */
> >>> >>       int             node;           /* NUMA node */
> >>> >> +     void            *acpi_handle;
> >>> >>  #ifdef CONFIG_X86_64
> >>> >>       void            *iommu;         /* IOMMU private data */
> >>> >>  #endif
> >>> >>
> >>>
> >>> acpi_handle is not good name and it is confusing.
> >>
> >> Well, what would be a better name in your opinion?
> >>
> >> I was going to put that into a #ifdef CONFIG_ACPI / #endif, so what about
> >> calling it acpi_data?
> >
> > yes, with #ifdef, you can use acpi_handle type directly.
> >
> > it is acpi handle for pci_root.
> >
> > so would call int pci_root_acpi_handle ?
> 
> I just copied the name from the corresponding ia64 code.  I don't care
> if you want to change it, but I think there is *some* value in keeping
> the x86 and ia64 code as similar as possible because it would be nice
> to converge it some day.

Well, the corresponding data structure for ia64 is called struct pci_controller,
so it is quite obvious what acpi_handle in there means. :-)

Since the data structure for x86 is called struct pci_sysdata and the data
type for the field in question may be acpi_handle, perhaps we can call that
field simply "root_handle"?

Alternatively, in analogy with the iommu we could use void * as its data
type and call it simply "acpi".

That said I'm fine with using just "void *acpi_handle" as you did, but I would
do the #ifdef CONFIG_ACPI / #endif around it anyway.

I wonder what Peter thinks?

Thanks,
Rafael
diff mbox

Patch

diff -u b/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
--- b/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -522,6 +522,7 @@ 
 	sd = &info->sd;
 	sd->domain = domain;
 	sd->node = node;
+	sd->acpi_handle = device->handle;
 	/*
 	 * Maybe the desired pci bus has been already scanned. In such case
 	 * it is unnecessary to scan the pci bus with the given domain,busnum.
@@ -596,9 +597,8 @@ 
 int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
 {
 	struct pci_sysdata *sd = bridge->bus->sysdata;
-	struct pci_root_info *info = container_of(sd, struct pci_root_info, sd);
 
-	ACPI_HANDLE_SET(&bridge->dev, info->bridge->handle);
+	ACPI_HANDLE_SET(&bridge->dev, sd->acpi_handle);
 	return 0;
 }
 
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -14,6 +14,7 @@ 
 struct pci_sysdata {
 	int		domain;		/* PCI domain */
 	int		node;		/* NUMA node */
+	void		*acpi_handle;
 #ifdef CONFIG_X86_64
 	void		*iommu;		/* IOMMU private data */
 #endif