| Message ID | 1460740008-19489-2-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Tomasz,
[auto build test ERROR on pci/next]
[also build test ERROR on v4.6-rc3 next-20160415]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Support-for-generic-ACPI-based-PCI-host-controller/20160416-011935
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All errors (new ones prefixed by >>):
drivers/pci/hotplug/sgi_hotplug.c: In function 'enable_slot':
>> drivers/pci/hotplug/sgi_hotplug.c:412:61: error: 'struct pci_controller' has no member named 'companion'
phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
^
drivers/pci/hotplug/sgi_hotplug.c: In function 'disable_slot':
drivers/pci/hotplug/sgi_hotplug.c:491:32: error: 'struct pci_controller' has no member named 'companion'
PCI_CONTROLLER(slot->pci_bus)->companion) {
^
drivers/pci/hotplug/sgi_hotplug.c:500:61: error: 'struct pci_controller' has no member named 'companion'
phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
^
vim +412 drivers/pci/hotplug/sgi_hotplug.c
3e643e77 John Keller 2007-01-30 406 struct acpi_device *pdevice;
3e643e77 John Keller 2007-01-30 407 acpi_handle phandle;
3e643e77 John Keller 2007-01-30 408 acpi_handle chandle = NULL;
3e643e77 John Keller 2007-01-30 409 acpi_handle rethandle;
3e643e77 John Keller 2007-01-30 410 acpi_status ret;
3e643e77 John Keller 2007-01-30 411
7b199811 Rafael J. Wysocki 2013-11-11 @412 phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
3e643e77 John Keller 2007-01-30 413
3e643e77 John Keller 2007-01-30 414 if (acpi_bus_get_device(phandle, &pdevice)) {
227f0647 Ryan Desfosses 2014-04-18 415 dev_dbg(&slot->pci_bus->self->dev, "no parent device, assuming NULL\n");
:::::: The code at line 412 was first introduced by commit
:::::: 7b1998116bbb2f3e5dd6cb9a8ee6db479b0b50a9 ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node
:::::: TO: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Apr 16, 2016 at 04:41:45AM +0800, kbuild test robot wrote: > Hi Tomasz, > > [auto build test ERROR on pci/next] > [also build test ERROR on v4.6-rc3 next-20160415] > [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] > > url: https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Support-for-generic-ACPI-based-PCI-host-controller/20160416-011935 > base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next > config: ia64-allmodconfig (attached as .config) > reproduce: > wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=ia64 > > All errors (new ones prefixed by >>): > > drivers/pci/hotplug/sgi_hotplug.c: In function 'enable_slot': > >> drivers/pci/hotplug/sgi_hotplug.c:412:61: error: 'struct pci_controller' has no member named 'companion' > phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion); > ^ > drivers/pci/hotplug/sgi_hotplug.c: In function 'disable_slot': > drivers/pci/hotplug/sgi_hotplug.c:491:32: error: 'struct pci_controller' has no member named 'companion' > PCI_CONTROLLER(slot->pci_bus)->companion) { > ^ > drivers/pci/hotplug/sgi_hotplug.c:500:61: error: 'struct pci_controller' has no member named 'companion' > phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion); > ^ I assume somebody is fixing this? > > vim +412 drivers/pci/hotplug/sgi_hotplug.c > > 3e643e77 John Keller 2007-01-30 406 struct acpi_device *pdevice; > 3e643e77 John Keller 2007-01-30 407 acpi_handle phandle; > 3e643e77 John Keller 2007-01-30 408 acpi_handle chandle = NULL; > 3e643e77 John Keller 2007-01-30 409 acpi_handle rethandle; > 3e643e77 John Keller 2007-01-30 410 acpi_status ret; > 3e643e77 John Keller 2007-01-30 411 > 7b199811 Rafael J. Wysocki 2013-11-11 @412 phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion); > 3e643e77 John Keller 2007-01-30 413 > 3e643e77 John Keller 2007-01-30 414 if (acpi_bus_get_device(phandle, &pdevice)) { > 227f0647 Ryan Desfosses 2014-04-18 415 dev_dbg(&slot->pci_bus->self->dev, "no parent device, assuming NULL\n"); > > :::::: The code at line 412 was first introduced by commit > :::::: 7b1998116bbb2f3e5dd6cb9a8ee6db479b0b50a9 ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node > > :::::: TO: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > :::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation -- 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
[question for Rafael below] On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote: > Currently we have two platforms (x86 & ia64) capable of PCI ACPI host > bridge initialization. They both use arch-specific sysdata to pass down > parent device reference and both rely on NULL parent in pci_create_root_bus() > to validate sysdata content. > > It looks hacky and prevents us from getting some firmware specific > info for PCI host controller based on its acpi_device structure > in generic pci_create_root_bus() function. However, we overcome that > blocker by passing down parent device via pci_create_root_bus parameter > (as the ACPI device type). Then we use ACPI_COMPANION_SET in core code > for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all > cases DT, ACPI and DT&ACPI. > > Since now PCI core code is setting ACPI companion device for us, > x86 & ia64 specific ACPI companion device setting turns out to be dead now. > We can get rid of it, including related companion reference from > PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid > companion device anymore. Therefore we need to convert its usage to > ACPI_COMPANION. > > Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Tested-by: Duc Dang <dhdang@apm.com> > Tested-by: Dongdong Liu <liudongdong3@huawei.com> > Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > Tested-by: Graeme Gregory <graeme.gregory@linaro.org> > Tested-by: Sinan Kaya <okaya@codeaurora.org> > --- > arch/ia64/hp/common/sba_iommu.c | 2 +- > arch/ia64/include/asm/pci.h | 1 - > arch/ia64/pci/pci.c | 16 ---------------- > arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- > arch/x86/include/asm/pci.h | 3 --- > arch/x86/pci/acpi.c | 17 ----------------- > drivers/acpi/pci_root.c | 7 ++++++- > drivers/pci/probe.c | 2 ++ > 8 files changed, 11 insertions(+), 41 deletions(-) > > diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c > index a6d6190..78e4444 100644 > --- a/arch/ia64/hp/common/sba_iommu.c > +++ b/arch/ia64/hp/common/sba_iommu.c > @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) > if (PCI_CONTROLLER(bus)->iommu) > return; > > - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > if (!handle) > return; > > diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h > index c0835b0..12423f4 100644 > --- a/arch/ia64/include/asm/pci.h > +++ b/arch/ia64/include/asm/pci.h > @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, > #define pci_legacy_write platform_pci_legacy_write > > struct pci_controller { > - struct acpi_device *companion; > void *iommu; > int segment; > int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */ > diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c > index 8f6ac2f..978d6af 100644 > --- a/arch/ia64/pci/pci.c > +++ b/arch/ia64/pci/pci.c > @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > } > > info->controller.segment = root->segment; > - info->controller.companion = device; > info->controller.node = acpi_get_node(device->handle); > INIT_LIST_HEAD(&info->io_resources); > return acpi_pci_root_create(root, &pci_acpi_root_ops, > &info->common, &info->controller); > } > > -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > -{ > - /* > - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > - * here, pci_create_root_bus() has been called by someone else and > - * sysdata is likely to be different from what we expect. Let it go in > - * that case. > - */ > - if (!bridge->dev.parent) { > - struct pci_controller *controller = bridge->bus->sysdata; > - ACPI_COMPANION_SET(&bridge->dev, controller->companion); > - } > - return 0; > -} > - > void pcibios_fixup_device_resources(struct pci_dev *dev) > { > int idx; > diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c > index 231234c..e454492 100644 > --- a/arch/ia64/sn/kernel/io_acpi_init.c > +++ b/arch/ia64/sn/kernel/io_acpi_init.c > @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) > struct acpi_resource_vendor_typed *vendor; > > > - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, > &sn_uuid, &buffer); > if (ACPI_FAILURE(status)) { > @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, > acpi_status status; > struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > > - rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion); > + rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); > status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, > &segment); > if (ACPI_SUCCESS(status)) { > diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h > index 9ab7507..24de07d 100644 > --- a/arch/x86/include/asm/pci.h > +++ b/arch/x86/include/asm/pci.h > @@ -14,9 +14,6 @@ > struct pci_sysdata { > int domain; /* PCI domain */ > int node; /* NUMA node */ > -#ifdef CONFIG_ACPI > - struct acpi_device *companion; /* ACPI companion device */ > -#endif > #ifdef CONFIG_X86_64 > void *iommu; /* IOMMU private data */ > #endif > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c > index 3cd6983..f4ca17a 100644 > --- a/arch/x86/pci/acpi.c > +++ b/arch/x86/pci/acpi.c > @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > struct pci_sysdata sd = { > .domain = domain, > .node = node, > - .companion = root->device > }; > > memcpy(bus->sysdata, &sd, sizeof(sd)); > @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > else { > info->sd.domain = domain; > info->sd.node = node; > - info->sd.companion = root->device; > bus = acpi_pci_root_create(root, &acpi_pci_root_ops, > &info->common, &info->sd); > } > @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > return bus; > } > > -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > -{ > - /* > - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > - * here, pci_create_root_bus() has been called by someone else and > - * sysdata is likely to be different from what we expect. Let it go in > - * that case. > - */ > - if (!bridge->dev.parent) { > - struct pci_sysdata *sd = bridge->bus->sysdata; > - ACPI_COMPANION_SET(&bridge->dev, sd->companion); > - } > - return 0; > -} > - > int __init pci_acpi_init(void) > { > struct pci_dev *dev = NULL; > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index ae3fe4e..4581e0e 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > } > } > > + /* > + * pci_create_root_bus() needs to detect the parent device type, > + * so initialize its companion data accordingly. > + */ > + ACPI_COMPANION_SET(&device->dev, device); > root->device = device; > root->segment = segment & 0xFFFF; > strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME); > @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > > pci_acpi_root_add_resources(info); > pci_add_resource(&info->resources, &root->secondary); > - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, > + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, > sysdata, &info->resources); "device" here is a struct acpi_device *. Rafael, is that the right thing to do? I dimly recall proposing something similar long ago and that it turned out to be a bad idea. > if (!bus) > goto out_release_info; > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 8004f67..8087297 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2141,6 +2141,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > bridge->dev.parent = parent; > bridge->dev.release = pci_release_host_bridge_dev; > dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > + if (parent) > + ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent)); > error = pcibios_root_bridge_prepare(bridge); > if (error) { > kfree(bridge); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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 27.04.2016 00:36, Bjorn Helgaas wrote: > On Sat, Apr 16, 2016 at 04:41:45AM +0800, kbuild test robot wrote: >> Hi Tomasz, >> >> [auto build test ERROR on pci/next] >> [also build test ERROR on v4.6-rc3 next-20160415] >> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] >> >> url: https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Support-for-generic-ACPI-based-PCI-host-controller/20160416-011935 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next >> config: ia64-allmodconfig (attached as .config) >> reproduce: >> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> make.cross ARCH=ia64 >> >> All errors (new ones prefixed by >>): >> >> drivers/pci/hotplug/sgi_hotplug.c: In function 'enable_slot': >>>> drivers/pci/hotplug/sgi_hotplug.c:412:61: error: 'struct pci_controller' has no member named 'companion' >> phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion); >> ^ >> drivers/pci/hotplug/sgi_hotplug.c: In function 'disable_slot': >> drivers/pci/hotplug/sgi_hotplug.c:491:32: error: 'struct pci_controller' has no member named 'companion' >> PCI_CONTROLLER(slot->pci_bus)->companion) { >> ^ >> drivers/pci/hotplug/sgi_hotplug.c:500:61: error: 'struct pci_controller' has no member named 'companion' >> phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion); >> ^ > > I assume somebody is fixing this? Yes, it will be fixed in next version also I will ask Hanjun to re-test it on IA64 as in the past. Tomasz -- 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 27.04.2016 04:45, Bjorn Helgaas wrote: > [question for Rafael below] > > On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote: >> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host >> bridge initialization. They both use arch-specific sysdata to pass down >> parent device reference and both rely on NULL parent in pci_create_root_bus() >> to validate sysdata content. >> >> It looks hacky and prevents us from getting some firmware specific >> info for PCI host controller based on its acpi_device structure >> in generic pci_create_root_bus() function. However, we overcome that >> blocker by passing down parent device via pci_create_root_bus parameter >> (as the ACPI device type). Then we use ACPI_COMPANION_SET in core code >> for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all >> cases DT, ACPI and DT&ACPI. >> >> Since now PCI core code is setting ACPI companion device for us, >> x86 & ia64 specific ACPI companion device setting turns out to be dead now. >> We can get rid of it, including related companion reference from >> PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid >> companion device anymore. Therefore we need to convert its usage to >> ACPI_COMPANION. >> >> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Tested-by: Duc Dang <dhdang@apm.com> >> Tested-by: Dongdong Liu <liudongdong3@huawei.com> >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> >> Tested-by: Graeme Gregory <graeme.gregory@linaro.org> >> Tested-by: Sinan Kaya <okaya@codeaurora.org> >> --- >> arch/ia64/hp/common/sba_iommu.c | 2 +- >> arch/ia64/include/asm/pci.h | 1 - >> arch/ia64/pci/pci.c | 16 ---------------- >> arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- >> arch/x86/include/asm/pci.h | 3 --- >> arch/x86/pci/acpi.c | 17 ----------------- >> drivers/acpi/pci_root.c | 7 ++++++- >> drivers/pci/probe.c | 2 ++ >> 8 files changed, 11 insertions(+), 41 deletions(-) >> >> diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c >> index a6d6190..78e4444 100644 >> --- a/arch/ia64/hp/common/sba_iommu.c >> +++ b/arch/ia64/hp/common/sba_iommu.c >> @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) >> if (PCI_CONTROLLER(bus)->iommu) >> return; >> >> - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); >> + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); >> if (!handle) >> return; >> >> diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h >> index c0835b0..12423f4 100644 >> --- a/arch/ia64/include/asm/pci.h >> +++ b/arch/ia64/include/asm/pci.h >> @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, >> #define pci_legacy_write platform_pci_legacy_write >> >> struct pci_controller { >> - struct acpi_device *companion; >> void *iommu; >> int segment; >> int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */ >> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c >> index 8f6ac2f..978d6af 100644 >> --- a/arch/ia64/pci/pci.c >> +++ b/arch/ia64/pci/pci.c >> @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> } >> >> info->controller.segment = root->segment; >> - info->controller.companion = device; >> info->controller.node = acpi_get_node(device->handle); >> INIT_LIST_HEAD(&info->io_resources); >> return acpi_pci_root_create(root, &pci_acpi_root_ops, >> &info->common, &info->controller); >> } >> >> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) >> -{ >> - /* >> - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL >> - * here, pci_create_root_bus() has been called by someone else and >> - * sysdata is likely to be different from what we expect. Let it go in >> - * that case. >> - */ >> - if (!bridge->dev.parent) { >> - struct pci_controller *controller = bridge->bus->sysdata; >> - ACPI_COMPANION_SET(&bridge->dev, controller->companion); >> - } >> - return 0; >> -} >> - >> void pcibios_fixup_device_resources(struct pci_dev *dev) >> { >> int idx; >> diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c >> index 231234c..e454492 100644 >> --- a/arch/ia64/sn/kernel/io_acpi_init.c >> +++ b/arch/ia64/sn/kernel/io_acpi_init.c >> @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) >> struct acpi_resource_vendor_typed *vendor; >> >> >> - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); >> + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); >> status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, >> &sn_uuid, &buffer); >> if (ACPI_FAILURE(status)) { >> @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, >> acpi_status status; >> struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> >> - rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion); >> + rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); >> status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, >> &segment); >> if (ACPI_SUCCESS(status)) { >> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h >> index 9ab7507..24de07d 100644 >> --- a/arch/x86/include/asm/pci.h >> +++ b/arch/x86/include/asm/pci.h >> @@ -14,9 +14,6 @@ >> struct pci_sysdata { >> int domain; /* PCI domain */ >> int node; /* NUMA node */ >> -#ifdef CONFIG_ACPI >> - struct acpi_device *companion; /* ACPI companion device */ >> -#endif >> #ifdef CONFIG_X86_64 >> void *iommu; /* IOMMU private data */ >> #endif >> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c >> index 3cd6983..f4ca17a 100644 >> --- a/arch/x86/pci/acpi.c >> +++ b/arch/x86/pci/acpi.c >> @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> struct pci_sysdata sd = { >> .domain = domain, >> .node = node, >> - .companion = root->device >> }; >> >> memcpy(bus->sysdata, &sd, sizeof(sd)); >> @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> else { >> info->sd.domain = domain; >> info->sd.node = node; >> - info->sd.companion = root->device; >> bus = acpi_pci_root_create(root, &acpi_pci_root_ops, >> &info->common, &info->sd); >> } >> @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> return bus; >> } >> >> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) >> -{ >> - /* >> - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL >> - * here, pci_create_root_bus() has been called by someone else and >> - * sysdata is likely to be different from what we expect. Let it go in >> - * that case. >> - */ >> - if (!bridge->dev.parent) { >> - struct pci_sysdata *sd = bridge->bus->sysdata; >> - ACPI_COMPANION_SET(&bridge->dev, sd->companion); >> - } >> - return 0; >> -} >> - >> int __init pci_acpi_init(void) >> { >> struct pci_dev *dev = NULL; >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index ae3fe4e..4581e0e 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, >> } >> } >> >> + /* >> + * pci_create_root_bus() needs to detect the parent device type, >> + * so initialize its companion data accordingly. >> + */ >> + ACPI_COMPANION_SET(&device->dev, device); >> root->device = device; >> root->segment = segment & 0xFFFF; >> strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME); >> @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> >> pci_acpi_root_add_resources(info); >> pci_add_resource(&info->resources, &root->secondary); >> - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, >> + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, >> sysdata, &info->resources); > > "device" here is a struct acpi_device *. Rafael, is that the right > thing to do? I dimly recall proposing something similar long ago and > that it turned out to be a bad idea. > Joining Bjorn's question. Rafael, do you recall what was the issue here from the past? Thanks, Tomasz -- 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 Wednesday, May 04, 2016 10:10:58 AM Tomasz Nowicki wrote: > On 27.04.2016 04:45, Bjorn Helgaas wrote: > > [question for Rafael below] > > > > On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote: > >> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host > >> bridge initialization. They both use arch-specific sysdata to pass down > >> parent device reference and both rely on NULL parent in pci_create_root_bus() > >> to validate sysdata content. > >> > >> It looks hacky and prevents us from getting some firmware specific > >> info for PCI host controller based on its acpi_device structure > >> in generic pci_create_root_bus() function. However, we overcome that > >> blocker by passing down parent device via pci_create_root_bus parameter > >> (as the ACPI device type). Then we use ACPI_COMPANION_SET in core code > >> for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all > >> cases DT, ACPI and DT&ACPI. > >> > >> Since now PCI core code is setting ACPI companion device for us, > >> x86 & ia64 specific ACPI companion device setting turns out to be dead now. > >> We can get rid of it, including related companion reference from > >> PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid > >> companion device anymore. Therefore we need to convert its usage to > >> ACPI_COMPANION. > >> > >> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > >> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Tested-by: Duc Dang <dhdang@apm.com> > >> Tested-by: Dongdong Liu <liudongdong3@huawei.com> > >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > >> Tested-by: Graeme Gregory <graeme.gregory@linaro.org> > >> Tested-by: Sinan Kaya <okaya@codeaurora.org> > >> --- > >> arch/ia64/hp/common/sba_iommu.c | 2 +- > >> arch/ia64/include/asm/pci.h | 1 - > >> arch/ia64/pci/pci.c | 16 ---------------- > >> arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- > >> arch/x86/include/asm/pci.h | 3 --- > >> arch/x86/pci/acpi.c | 17 ----------------- > >> drivers/acpi/pci_root.c | 7 ++++++- > >> drivers/pci/probe.c | 2 ++ > >> 8 files changed, 11 insertions(+), 41 deletions(-) > >> > >> diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c > >> index a6d6190..78e4444 100644 > >> --- a/arch/ia64/hp/common/sba_iommu.c > >> +++ b/arch/ia64/hp/common/sba_iommu.c > >> @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) > >> if (PCI_CONTROLLER(bus)->iommu) > >> return; > >> > >> - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > >> + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > >> if (!handle) > >> return; > >> > >> diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h > >> index c0835b0..12423f4 100644 > >> --- a/arch/ia64/include/asm/pci.h > >> +++ b/arch/ia64/include/asm/pci.h > >> @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, > >> #define pci_legacy_write platform_pci_legacy_write > >> > >> struct pci_controller { > >> - struct acpi_device *companion; > >> void *iommu; > >> int segment; > >> int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */ > >> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c > >> index 8f6ac2f..978d6af 100644 > >> --- a/arch/ia64/pci/pci.c > >> +++ b/arch/ia64/pci/pci.c > >> @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >> } > >> > >> info->controller.segment = root->segment; > >> - info->controller.companion = device; > >> info->controller.node = acpi_get_node(device->handle); > >> INIT_LIST_HEAD(&info->io_resources); > >> return acpi_pci_root_create(root, &pci_acpi_root_ops, > >> &info->common, &info->controller); > >> } > >> > >> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > >> -{ > >> - /* > >> - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > >> - * here, pci_create_root_bus() has been called by someone else and > >> - * sysdata is likely to be different from what we expect. Let it go in > >> - * that case. > >> - */ > >> - if (!bridge->dev.parent) { > >> - struct pci_controller *controller = bridge->bus->sysdata; > >> - ACPI_COMPANION_SET(&bridge->dev, controller->companion); > >> - } > >> - return 0; > >> -} > >> - > >> void pcibios_fixup_device_resources(struct pci_dev *dev) > >> { > >> int idx; > >> diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c > >> index 231234c..e454492 100644 > >> --- a/arch/ia64/sn/kernel/io_acpi_init.c > >> +++ b/arch/ia64/sn/kernel/io_acpi_init.c > >> @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) > >> struct acpi_resource_vendor_typed *vendor; > >> > >> > >> - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > >> + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > >> status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, > >> &sn_uuid, &buffer); > >> if (ACPI_FAILURE(status)) { > >> @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, > >> acpi_status status; > >> struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > >> > >> - rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion); > >> + rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); > >> status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, > >> &segment); > >> if (ACPI_SUCCESS(status)) { > >> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h > >> index 9ab7507..24de07d 100644 > >> --- a/arch/x86/include/asm/pci.h > >> +++ b/arch/x86/include/asm/pci.h > >> @@ -14,9 +14,6 @@ > >> struct pci_sysdata { > >> int domain; /* PCI domain */ > >> int node; /* NUMA node */ > >> -#ifdef CONFIG_ACPI > >> - struct acpi_device *companion; /* ACPI companion device */ > >> -#endif > >> #ifdef CONFIG_X86_64 > >> void *iommu; /* IOMMU private data */ > >> #endif > >> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c > >> index 3cd6983..f4ca17a 100644 > >> --- a/arch/x86/pci/acpi.c > >> +++ b/arch/x86/pci/acpi.c > >> @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >> struct pci_sysdata sd = { > >> .domain = domain, > >> .node = node, > >> - .companion = root->device > >> }; > >> > >> memcpy(bus->sysdata, &sd, sizeof(sd)); > >> @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >> else { > >> info->sd.domain = domain; > >> info->sd.node = node; > >> - info->sd.companion = root->device; > >> bus = acpi_pci_root_create(root, &acpi_pci_root_ops, > >> &info->common, &info->sd); > >> } > >> @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >> return bus; > >> } > >> > >> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > >> -{ > >> - /* > >> - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > >> - * here, pci_create_root_bus() has been called by someone else and > >> - * sysdata is likely to be different from what we expect. Let it go in > >> - * that case. > >> - */ > >> - if (!bridge->dev.parent) { > >> - struct pci_sysdata *sd = bridge->bus->sysdata; > >> - ACPI_COMPANION_SET(&bridge->dev, sd->companion); > >> - } > >> - return 0; > >> -} > >> - > >> int __init pci_acpi_init(void) > >> { > >> struct pci_dev *dev = NULL; > >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > >> index ae3fe4e..4581e0e 100644 > >> --- a/drivers/acpi/pci_root.c > >> +++ b/drivers/acpi/pci_root.c > >> @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > >> } > >> } > >> > >> + /* > >> + * pci_create_root_bus() needs to detect the parent device type, > >> + * so initialize its companion data accordingly. > >> + */ > >> + ACPI_COMPANION_SET(&device->dev, device); > >> root->device = device; > >> root->segment = segment & 0xFFFF; > >> strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME); > >> @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > >> > >> pci_acpi_root_add_resources(info); > >> pci_add_resource(&info->resources, &root->secondary); > >> - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, > >> + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, > >> sysdata, &info->resources); > > > > "device" here is a struct acpi_device *. Rafael, is that the right > > thing to do? I dimly recall proposing something similar long ago and > > that it turned out to be a bad idea. > > > > Joining Bjorn's question. Rafael, do you recall what was the issue here > from the past? Yes, I do. Anything struct acpi_device doesn't represent a real device. It represents a firmware object that can be associated with a device. IOW, nothing under LNXSYSTM\:00/ should ever be used as the parent or sibling etc with respect to anything outside of that directory. In fact, the entire LNXSYSTM\:00/ should be located under /sys/firmware/acpi/ and it was a mistake to put it under /sys/devices/. One immediate consequence of the above change, AFAICS, would be that the whole PCI hierarchy would now hang under /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/ which would not make any sense whatever, because /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/physical_node already points to /sys/devices/pci0000\:00/. IOW, both PNP0A08\:00/ and pci0000\:00/ already represent the same thing, ie. the host bridge. If you want to have a parent for pci0000\:00/, you need a physical_node for LNXSYBUS\:00 and point to that as the parent from pci0000\:00/. That at least will lead to a sysfs hierarchy that makes sense, although it may break user space tools I suppose (which may be assuming that pci0000\:00/ will always be present directly under /sys/devices/). Thanks, Rafael -- 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, April 15, 2016 07:06:36 PM Tomasz Nowicki wrote: > Currently we have two platforms (x86 & ia64) capable of PCI ACPI host > bridge initialization. They both use arch-specific sysdata to pass down > parent device reference and both rely on NULL parent in pci_create_root_bus() > to validate sysdata content. No, this is not relied on for that. It is just NULL, because we don't have a physical parent device representation for the PCI host bridge. > It looks hacky and prevents us from getting some firmware specific > info for PCI host controller based on its acpi_device structure > in generic pci_create_root_bus() function. You can't get that info without adding ACPI-specific code to that function anyway. > However, we overcome that blocker by passing down parent device via > pci_create_root_bus parameter (as the ACPI device type). Which is completely wrong. > Then we use ACPI_COMPANION_SET in core code > for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all > cases DT, ACPI and DT&ACPI. > > Since now PCI core code is setting ACPI companion device for us, > x86 & ia64 specific ACPI companion device setting turns out to be dead now. > We can get rid of it, including related companion reference from > PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid > companion device anymore. Therefore we need to convert its usage to > ACPI_COMPANION. > > Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Tested-by: Duc Dang <dhdang@apm.com> > Tested-by: Dongdong Liu <liudongdong3@huawei.com> > Tested-by: Hanjun Guo <hanjun.guo@linaro.org> > Tested-by: Graeme Gregory <graeme.gregory@linaro.org> > Tested-by: Sinan Kaya <okaya@codeaurora.org> > --- > arch/ia64/hp/common/sba_iommu.c | 2 +- > arch/ia64/include/asm/pci.h | 1 - > arch/ia64/pci/pci.c | 16 ---------------- > arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- > arch/x86/include/asm/pci.h | 3 --- > arch/x86/pci/acpi.c | 17 ----------------- > drivers/acpi/pci_root.c | 7 ++++++- > drivers/pci/probe.c | 2 ++ > 8 files changed, 11 insertions(+), 41 deletions(-) > > diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c > index a6d6190..78e4444 100644 > --- a/arch/ia64/hp/common/sba_iommu.c > +++ b/arch/ia64/hp/common/sba_iommu.c > @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) > if (PCI_CONTROLLER(bus)->iommu) > return; > > - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > if (!handle) > return; > > diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h > index c0835b0..12423f4 100644 > --- a/arch/ia64/include/asm/pci.h > +++ b/arch/ia64/include/asm/pci.h > @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, > #define pci_legacy_write platform_pci_legacy_write > > struct pci_controller { > - struct acpi_device *companion; > void *iommu; > int segment; > int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */ > diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c > index 8f6ac2f..978d6af 100644 > --- a/arch/ia64/pci/pci.c > +++ b/arch/ia64/pci/pci.c > @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > } > > info->controller.segment = root->segment; > - info->controller.companion = device; > info->controller.node = acpi_get_node(device->handle); > INIT_LIST_HEAD(&info->io_resources); > return acpi_pci_root_create(root, &pci_acpi_root_ops, > &info->common, &info->controller); > } > > -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > -{ > - /* > - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > - * here, pci_create_root_bus() has been called by someone else and > - * sysdata is likely to be different from what we expect. Let it go in > - * that case. > - */ > - if (!bridge->dev.parent) { > - struct pci_controller *controller = bridge->bus->sysdata; > - ACPI_COMPANION_SET(&bridge->dev, controller->companion); > - } > - return 0; > -} > - > void pcibios_fixup_device_resources(struct pci_dev *dev) > { > int idx; > diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c > index 231234c..e454492 100644 > --- a/arch/ia64/sn/kernel/io_acpi_init.c > +++ b/arch/ia64/sn/kernel/io_acpi_init.c > @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) > struct acpi_resource_vendor_typed *vendor; > > > - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); > + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); > status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, > &sn_uuid, &buffer); > if (ACPI_FAILURE(status)) { > @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, > acpi_status status; > struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > > - rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion); > + rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); > status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, > &segment); > if (ACPI_SUCCESS(status)) { > diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h > index 9ab7507..24de07d 100644 > --- a/arch/x86/include/asm/pci.h > +++ b/arch/x86/include/asm/pci.h > @@ -14,9 +14,6 @@ > struct pci_sysdata { > int domain; /* PCI domain */ > int node; /* NUMA node */ > -#ifdef CONFIG_ACPI > - struct acpi_device *companion; /* ACPI companion device */ > -#endif > #ifdef CONFIG_X86_64 > void *iommu; /* IOMMU private data */ > #endif > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c > index 3cd6983..f4ca17a 100644 > --- a/arch/x86/pci/acpi.c > +++ b/arch/x86/pci/acpi.c > @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > struct pci_sysdata sd = { > .domain = domain, > .node = node, > - .companion = root->device > }; > > memcpy(bus->sysdata, &sd, sizeof(sd)); > @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > else { > info->sd.domain = domain; > info->sd.node = node; > - info->sd.companion = root->device; > bus = acpi_pci_root_create(root, &acpi_pci_root_ops, > &info->common, &info->sd); > } > @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > return bus; > } > > -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > -{ > - /* > - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL > - * here, pci_create_root_bus() has been called by someone else and > - * sysdata is likely to be different from what we expect. Let it go in > - * that case. > - */ > - if (!bridge->dev.parent) { > - struct pci_sysdata *sd = bridge->bus->sysdata; > - ACPI_COMPANION_SET(&bridge->dev, sd->companion); > - } > - return 0; > -} > - > int __init pci_acpi_init(void) > { > struct pci_dev *dev = NULL; > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index ae3fe4e..4581e0e 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > } > } > > + /* > + * pci_create_root_bus() needs to detect the parent device type, > + * so initialize its companion data accordingly. > + */ > + ACPI_COMPANION_SET(&device->dev, device); This is a complete nonsense, sorry. This just means that the ACPI firmware object will now be an ACPI companion of itself. Thanks, Rafael -- 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 Tue, May 10, 2016 at 12:56:37AM +0200, Rafael J. Wysocki wrote: > On Friday, April 15, 2016 07:06:36 PM Tomasz Nowicki wrote: > > Currently we have two platforms (x86 & ia64) capable of PCI ACPI host > > bridge initialization. They both use arch-specific sysdata to pass down > > parent device reference and both rely on NULL parent in pci_create_root_bus() > > to validate sysdata content. > > No, this is not relied on for that. It is just NULL, because we don't > have a physical parent device representation for the PCI host bridge. > > > It looks hacky and prevents us from getting some firmware specific > > info for PCI host controller based on its acpi_device structure > > in generic pci_create_root_bus() function. > > You can't get that info without adding ACPI-specific code to that function > anyway. > > > However, we overcome that blocker by passing down parent device via > > pci_create_root_bus parameter (as the ACPI device type). > > Which is completely wrong. > > > Then we use ACPI_COMPANION_SET in core code > > for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all > > cases DT, ACPI and DT&ACPI. > > > > Since now PCI core code is setting ACPI companion device for us, > > x86 & ia64 specific ACPI companion device setting turns out to be dead now. > > We can get rid of it, including related companion reference from > > PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid > > companion device anymore. Therefore we need to convert its usage to > > ACPI_COMPANION. I think getting rid of arch-specific sysdata is a good goal (at least for things like the domain and ACPI companion that are really not arch-specific). But I'm afraid I've been guilty of allowing the perfect to be the enemy of the good. I think it will be more useful in the end if we can get as much of this work in v4.7 as possible, even if there's more polishing we'd like to do later. What if we keep the x86 and ia64 handling of ACPI_COMPANION the way it is today, and do it the same way for arm64? Then later, after we merge Arnd's work to make a generic pci_register_host() or similar, I suspect we might be able to unify the ACPI_COMPANION stuff more easily. If the driver (acpi/pci_root.c) allocates the struct pci_host_bridge, then maybe it will be able to fill in the domain and set up the ACPI_COMPANION before it calls the scan interface. 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 Mon, May 09, 2016 at 08:53:36PM -0500, Bjorn Helgaas wrote: > On Tue, May 10, 2016 at 12:56:37AM +0200, Rafael J. Wysocki wrote: > > On Friday, April 15, 2016 07:06:36 PM Tomasz Nowicki wrote: > > > Currently we have two platforms (x86 & ia64) capable of PCI ACPI host > > > bridge initialization. They both use arch-specific sysdata to pass down > > > parent device reference and both rely on NULL parent in pci_create_root_bus() > > > to validate sysdata content. > > > > No, this is not relied on for that. It is just NULL, because we don't > > have a physical parent device representation for the PCI host bridge. > > > > > It looks hacky and prevents us from getting some firmware specific > > > info for PCI host controller based on its acpi_device structure > > > in generic pci_create_root_bus() function. > > > > You can't get that info without adding ACPI-specific code to that function > > anyway. > > > > > However, we overcome that blocker by passing down parent device via > > > pci_create_root_bus parameter (as the ACPI device type). > > > > Which is completely wrong. > > > > > Then we use ACPI_COMPANION_SET in core code > > > for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all > > > cases DT, ACPI and DT&ACPI. > > > > > > Since now PCI core code is setting ACPI companion device for us, > > > x86 & ia64 specific ACPI companion device setting turns out to be dead now. > > > We can get rid of it, including related companion reference from > > > PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid > > > companion device anymore. Therefore we need to convert its usage to > > > ACPI_COMPANION. > > I think getting rid of arch-specific sysdata is a good goal (at least > for things like the domain and ACPI companion that are really not > arch-specific). > > But I'm afraid I've been guilty of allowing the perfect to be the > enemy of the good. I think it will be more useful in the end if we > can get as much of this work in v4.7 as possible, even if there's more > polishing we'd like to do later. > > What if we keep the x86 and ia64 handling of ACPI_COMPANION the way it > is today, and do it the same way for arm64? Then later, after we > merge Arnd's work to make a generic pci_register_host() or similar, I > suspect we might be able to unify the ACPI_COMPANION stuff more > easily. If the driver (acpi/pci_root.c) allocates the struct > pci_host_bridge, then maybe it will be able to fill in the domain and > set up the ACPI_COMPANION before it calls the scan interface. Ok, JC already solved that problem like this: http://www.spinics.net/lists/arm-kernel/msg478167.html http://www.spinics.net/lists/arm-kernel/msg478169.html Since ARM64 relies on PCI_DOMAINS_GENERIC (DT) I do not see any other way of pulling it off quickly (and in a really generic way), sysdata is the only way of handling it in ACPI unless we start clutching at straws (which we already did with the ACPI companion hacks). Thanks, Lorenzo -- 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 Tue, May 10, 2016 at 12:18:59AM +0200, Rafael J. Wysocki wrote: [...] > > >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > > >> index ae3fe4e..4581e0e 100644 > > >> --- a/drivers/acpi/pci_root.c > > >> +++ b/drivers/acpi/pci_root.c > > >> @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, > > >> } > > >> } > > >> > > >> + /* > > >> + * pci_create_root_bus() needs to detect the parent device type, > > >> + * so initialize its companion data accordingly. > > >> + */ > > >> + ACPI_COMPANION_SET(&device->dev, device); > > >> root->device = device; > > >> root->segment = segment & 0xFFFF; > > >> strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME); > > >> @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > > >> > > >> pci_acpi_root_add_resources(info); > > >> pci_add_resource(&info->resources, &root->secondary); > > >> - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, > > >> + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, > > >> sysdata, &info->resources); > > > > > > "device" here is a struct acpi_device *. Rafael, is that the right > > > thing to do? I dimly recall proposing something similar long ago and > > > that it turned out to be a bad idea. > > > > > > > Joining Bjorn's question. Rafael, do you recall what was the issue here > > from the past? > > Yes, I do. > > Anything struct acpi_device doesn't represent a real device. It > represents a firmware object that can be associated with a device. > IOW, nothing under LNXSYSTM\:00/ should ever be used as the parent or > sibling etc with respect to anything outside of that directory. In > fact, the entire LNXSYSTM\:00/ should be located under > /sys/firmware/acpi/ and it was a mistake to put it under > /sys/devices/. > > One immediate consequence of the above change, AFAICS, would be that > the whole PCI hierarchy would now hang under > /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/ which would not > make any sense whatever, because > /sys/devices/LNXSYSTM\:00/LNXSYBUS\:00/PNP0A08\:00/physical_node > already points to /sys/devices/pci0000\:00/. > > IOW, both PNP0A08\:00/ and pci0000\:00/ already represent the same > thing, ie. the host bridge. > > If you want to have a parent for pci0000\:00/, you need a > physical_node for LNXSYBUS\:00 and point to that as the parent from > pci0000\:00/. That at least will lead to a sysfs hierarchy that makes > sense, although it may break user space tools I suppose (which may be > assuming that pci0000\:00/ will always be present directly under > /sys/devices/). Ok, I have a question though. As an example, DT based host controllers (that pass the parent pointer to pci_create_root_bus()), are already rooted at the respective host controller platform device sysfs path, so if user space can't cope with that, that is a problem *now* on some systems unless I am missing something. Anyway, thanks for clarifying the companion handling mechanism, we decidedly have to find a proper way to handle it instead of working around it. Lorenzo -- 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
diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c index a6d6190..78e4444 100644 --- a/arch/ia64/hp/common/sba_iommu.c +++ b/arch/ia64/hp/common/sba_iommu.c @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) if (PCI_CONTROLLER(bus)->iommu) return; - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); if (!handle) return; diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h index c0835b0..12423f4 100644 --- a/arch/ia64/include/asm/pci.h +++ b/arch/ia64/include/asm/pci.h @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, #define pci_legacy_write platform_pci_legacy_write struct pci_controller { - struct acpi_device *companion; void *iommu; int segment; int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */ diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 8f6ac2f..978d6af 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) } info->controller.segment = root->segment; - info->controller.companion = device; info->controller.node = acpi_get_node(device->handle); INIT_LIST_HEAD(&info->io_resources); return acpi_pci_root_create(root, &pci_acpi_root_ops, &info->common, &info->controller); } -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{ - /* - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL - * here, pci_create_root_bus() has been called by someone else and - * sysdata is likely to be different from what we expect. Let it go in - * that case. - */ - if (!bridge->dev.parent) { - struct pci_controller *controller = bridge->bus->sysdata; - ACPI_COMPANION_SET(&bridge->dev, controller->companion); - } - return 0; -} - void pcibios_fixup_device_resources(struct pci_dev *dev) { int idx; diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c index 231234c..e454492 100644 --- a/arch/ia64/sn/kernel/io_acpi_init.c +++ b/arch/ia64/sn/kernel/io_acpi_init.c @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) struct acpi_resource_vendor_typed *vendor; - handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion); + handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, &sn_uuid, &buffer); if (ACPI_FAILURE(status)) { @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, acpi_status status; struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL }; - rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion); + rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, &segment); if (ACPI_SUCCESS(status)) { diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507..24de07d 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -14,9 +14,6 @@ struct pci_sysdata { int domain; /* PCI domain */ int node; /* NUMA node */ -#ifdef CONFIG_ACPI - struct acpi_device *companion; /* ACPI companion device */ -#endif #ifdef CONFIG_X86_64 void *iommu; /* IOMMU private data */ #endif diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 3cd6983..f4ca17a 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) struct pci_sysdata sd = { .domain = domain, .node = node, - .companion = root->device }; memcpy(bus->sysdata, &sd, sizeof(sd)); @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) else { info->sd.domain = domain; info->sd.node = node; - info->sd.companion = root->device; bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &info->common, &info->sd); } @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) return bus; } -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{ - /* - * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL - * here, pci_create_root_bus() has been called by someone else and - * sysdata is likely to be different from what we expect. Let it go in - * that case. - */ - if (!bridge->dev.parent) { - struct pci_sysdata *sd = bridge->bus->sysdata; - ACPI_COMPANION_SET(&bridge->dev, sd->companion); - } - return 0; -} - int __init pci_acpi_init(void) { struct pci_dev *dev = NULL; diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index ae3fe4e..4581e0e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, } } + /* + * pci_create_root_bus() needs to detect the parent device type, + * so initialize its companion data accordingly. + */ + ACPI_COMPANION_SET(&device->dev, device); root->device = device; root->segment = segment & 0xFFFF; strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME); @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, pci_acpi_root_add_resources(info); pci_add_resource(&info->resources, &root->secondary); - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, sysdata, &info->resources); if (!bus) goto out_release_info; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 8004f67..8087297 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2141,6 +2141,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, bridge->dev.parent = parent; bridge->dev.release = pci_release_host_bridge_dev; dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); + if (parent) + ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent)); error = pcibios_root_bridge_prepare(bridge); if (error) { kfree(bridge);