Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1397494 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code. "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-05-10 01:00 +0200
    Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge  device companion assignment to core code. Bjorn Helgaas <helgaas@kernel.org> - 2016-05-10 04:00 +0200
      Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge  device companion assignment to core code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-05-10 12:10 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web