Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.

[Rafael J. Wysocki]

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@xxxxxxx>
> >> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> >> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> >> Tested-by: Duc Dang <dhdang@xxxxxxx>
> >> Tested-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> >> Tested-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> >> Tested-by: Graeme Gregory <graeme.gregory@xxxxxxxxxx>
> >> Tested-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
> >> ---
> >>   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