| Message ID | 20130104010038.GA11155@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 >
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
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
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
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
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 -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