| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5729AE92.2090106@semihalf.com>
Date: Wed, 4 May 2016 10:10:58 +0200
From: Tomasz Nowicki <tn@...ihalf.com>
To: Bjorn Helgaas <helgaas@...nel.org>, rafael@...nel.org
Cc: arnd@...db.de, will.deacon@....com, catalin.marinas@....com,
hanjun.guo@...aro.org, Lorenzo.Pieralisi@....com,
okaya@...eaurora.org, jiang.liu@...ux.intel.com,
jchandra@...adcom.com, robert.richter@...iumnetworks.com,
mw@...ihalf.com, Liviu.Dudau@....com, ddaney@...iumnetworks.com,
wangyijing@...wei.com, Suravee.Suthikulpanit@....com,
msalter@...hat.com, linux-pci@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, linaro-acpi@...ts.linaro.org,
jcm@...hat.com
Subject: Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge
device companion assignment to core code.
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@....com>
>> Signed-off-by: Tomasz Nowicki <tn@...ihalf.com>
>> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
>> Tested-by: Duc Dang <dhdang@....com>
>> Tested-by: Dongdong Liu <liudongdong3@...wei.com>
>> Tested-by: Hanjun Guo <hanjun.guo@...aro.org>
>> Tested-by: Graeme Gregory <graeme.gregory@...aro.org>
>> Tested-by: Sinan Kaya <okaya@...eaurora.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
Powered by blists - more mailing lists