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