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

Groups > linux.kernel > #1240807 > unrolled thread

Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core

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 4/7] PCI/ACPI: Consolidate common PCI host bridge code  into ACPI core Bjorn Helgaas <helgaas@kernel.org> - 2015-10-06 19:50 +0200
    Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code  into ACPI core Jiang Liu <jiang.liu@linux.intel.com> - 2015-10-08 07:40 +0200
      Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code  into ACPI core Bjorn Helgaas <helgaas@kernel.org> - 2015-10-08 15:30 +0200
        Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code  into ACPI core Jiang Liu <jiang.liu@linux.intel.com> - 2015-10-09 10:20 +0200

#1240807 — Re: [Patch v6 4/7] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core

Hi Jiang,

Strictly speaking, this patch by itself doesn't actually "consolidate"
anything because it only adds acpi_pci_root_create() (which isn't called by
anything yet), but doesn't remove the original x86 copy.

On Mon, Sep 14, 2015 at 04:07:33PM +0800, Jiang Liu wrote:
> Introduce common interface acpi_pci_root_create() and related data
> structures to create PCI root bus for ACPI PCI host bridges. It will
> be used to kill duplicated arch specific code for IA64 and x86. It may
> also help ARM64 in future.
> 
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Some minor structuring comments below, but my ack is valid even if you
don't address them.

> ---
>  drivers/acpi/pci_root.c  |  204 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |   23 ++++++
>  2 files changed, 227 insertions(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 393706a5261b..48d27bd35b58 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -652,6 +652,210 @@ static void acpi_pci_root_remove(struct acpi_device *device)
>  	kfree(root);
>  }
>  
> +/*
> + * Following code to support acpi_pci_root_create() is copied from
> + * arch/x86/pci/acpi.c and modified so it could be reused by x86, IA64
> + * and ARM64.
> + */
> +static void acpi_pci_root_validate_resources(struct device *dev,
> +					     struct list_head *resources,
> +					     unsigned long type)
> +{
> +	LIST_HEAD(list);
> +	struct resource *res1, *res2, *root = NULL;
> +	struct resource_entry *tmp, *entry, *entry2;
> +
> +	BUG_ON((type & (IORESOURCE_MEM | IORESOURCE_IO)) == 0);
> +	root = (type & IORESOURCE_MEM) ? &iomem_resource : &ioport_resource;
> +
> +	list_splice_init(resources, &list);
> +	resource_list_for_each_entry_safe(entry, tmp, &list) {
> +		bool free = false;
> +		resource_size_t end;
> +
> +		res1 = entry->res;
> +		if (!(res1->flags & type))
> +			goto next;
> +
> +		/* Exclude non-addressable range or non-addressable portion */
> +		end = min(res1->end, root->end);
> +		if (end <= res1->start) {
> +			dev_info(dev, "host bridge window %pR (ignored, not CPU addressable)\n",
> +				 res1);
> +			free = true;
> +			goto next;
> +		} else if (res1->end != end) {
> +			dev_info(dev, "host bridge window %pR ([%#llx-%#llx] ignored, not CPU addressable)\n",
> +				 res1, (unsigned long long)end + 1,
> +				 (unsigned long long)res1->end);
> +			res1->end = end;
> +		}
> +
> +		resource_list_for_each_entry(entry2, resources) {
> +			res2 = entry2->res;
> +			if (!(res2->flags & type))
> +				continue;
> +
> +			/*
> +			 * I don't like throwing away windows because then
> +			 * our resources no longer match the ACPI _CRS, but
> +			 * the kernel resource tree doesn't allow overlaps.
> +			 */
> +			if (resource_overlaps(res1, res2)) {
> +				res2->start = min(res1->start, res2->start);
> +				res2->end = max(res1->end, res2->end);
> +				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
> +					 res2, res1);
> +				free = true;
> +				goto next;
> +			}
> +		}
> +
> +next:
> +		resource_list_del(entry);
> +		if (free)
> +			resource_list_free_entry(entry);
> +		else
> +			resource_list_add_tail(entry, resources);
> +	}
> +}
> +
> +static int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> +{
> +	int ret;
> +	struct list_head *list = &info->resources;
> +	struct acpi_device *device = info->bridge;
> +	struct resource_entry *entry, *tmp;
> +	unsigned long flags;
> +
> +	flags = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_MEM_8AND16BIT;
> +	ret = acpi_dev_get_resources(device, list,
> +				     acpi_dev_filter_resource_type_cb,
> +				     (void *)flags);
> +	if (ret < 0)
> +		dev_warn(&device->dev,
> +			 "failed to parse _CRS method, error code %d\n", ret);
> +	else if (ret == 0)
> +		dev_dbg(&device->dev,
> +			"no IO and memory resources present in _CRS\n");
> +	else {
> +		resource_list_for_each_entry_safe(entry, tmp, list) {
> +			if (entry->res->flags & IORESOURCE_DISABLED)
> +				resource_list_destroy_entry(entry);
> +			else
> +				entry->res->name = info->name;
> +		}
> +		acpi_pci_root_validate_resources(&device->dev, list,
> +						 IORESOURCE_MEM);
> +		acpi_pci_root_validate_resources(&device->dev, list,
> +						 IORESOURCE_IO);
> +	}
> +
> +	return ret;
> +}
> +
> +static void pci_acpi_root_add_resources(struct acpi_pci_root_info *info)
> +{
> +	struct resource_entry *entry, *tmp;
> +	struct resource *res, *conflict, *root = NULL;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
> +		res = entry->res;
> +		if (res->flags & IORESOURCE_MEM)
> +			root = &iomem_resource;
> +		else if (res->flags & IORESOURCE_IO)
> +			root = &ioport_resource;
> +		else
> +			continue;
> +
> +		conflict = insert_resource_conflict(root, res);
> +		if (conflict) {
> +			dev_info(&info->bridge->dev,
> +				 "ignoring host bridge window %pR (conflicts with %s %pR)\n",
> +				 res, conflict->name, conflict);
> +			resource_list_destroy_entry(entry);
> +		}
> +	}
> +}
> +
> +static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
> +{
> +	struct resource *res;
> +	struct resource_entry *entry, *tmp;
> +
> +	if (!info)
> +		return;
> +
> +	resource_list_for_each_entry_safe(entry, tmp, &info->resources) {
> +		res = entry->res;
> +		if (res->parent &&
> +		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +			release_resource(res);
> +		resource_list_destroy_entry(entry);
> +	}
> +
> +	info->ops->release_info(info);
> +}
> +
> +static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> +{
> +	struct resource *res;
> +	struct resource_entry *entry;
> +
> +	resource_list_for_each_entry(entry, &bridge->windows) {
> +		res = entry->res;
> +		if (res->parent &&
> +		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +			release_resource(res);
> +	}
> +	__acpi_pci_root_release_info(bridge->release_data);
> +}
> +
> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> +				     struct acpi_pci_root_ops *ops,
> +				     struct acpi_pci_root_info *info,
> +				     void *sysdata)
> +{
> +	int ret, busnum = root->secondary.start;
> +	struct acpi_device *device = root->device;
> +	int node = acpi_get_node(device->handle);
> +	struct pci_bus *bus;
> +
> +	info->root = root;
> +	info->bridge = device;
> +	info->ops = ops;
> +	INIT_LIST_HEAD(&info->resources);
> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> +		 root->segment, busnum);
> +
> +	if (ops->init_info && ops->init_info(info))
> +		goto out_release_info;
> +	ret = acpi_pci_probe_root_resources(info);
> +	if (ops->prepare_resources)
> +		ret = ops->prepare_resources(info, ret);
> +	if (ret < 0)
> +		goto out_release_info;
> +	else if (ret > 0)
> +		pci_acpi_root_add_resources(info);

This is unnecessarily complicated: you set "ret", then overwrite it if
ops->prepare_resources.  By the time you test "ret", it's messy to
figure out what it means.

Both ops->prepare_resources() and pci_acpi_root_add_resources()
should be able to deal with empty resource lists, so can you do the
following instead?

    ret = acpi_pci_probe_root_resources(info);
    if (ret < 0)
        goto out_release_info;
    if (ops->prepare_resources) {
        ret = ops->prepare_resources(info, ret);
        if (ret < 0)
            goto out_release_info;
    }
    pci_acpi_root_add_resources(info);

> +	pci_add_resource(&info->resources, &root->secondary);
> +
> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +				  sysdata, &info->resources);
> +	if (bus) {

    if (!bus)
        goto out_release_info;

Then it looks like the other error paths above, and you can un-indent
the following code, which is the normal path:

> +		pci_scan_child_bus(bus);
> +		pci_set_host_bridge_release(to_pci_host_bridge(bus->bridge),
> +					    acpi_pci_root_release_info, info);
> +		if (node != NUMA_NO_NODE)
> +			dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n",
> +				   node);
> +		return bus;
> +	}
> +
> +out_release_info:
> +	__acpi_pci_root_release_info(info);
> +	return NULL;
> +}
> +
>  void __init acpi_pci_root_init(void)
>  {
>  	acpi_hest_init();
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index a965efa52152..583af37c6030 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -52,6 +52,29 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>  	return ACPI_HANDLE(dev);
>  }
>  
> +struct acpi_pci_root;
> +struct acpi_pci_root_ops;
> +
> +struct acpi_pci_root_info {
> +	struct acpi_pci_root		*root;
> +	struct acpi_device		*bridge;
> +	struct acpi_pci_root_ops	*ops;
> +	struct list_head		resources;
> +	char				name[16];
> +};
> +
> +struct acpi_pci_root_ops {
> +	struct pci_ops *pci_ops;
> +	int (*init_info)(struct acpi_pci_root_info *info);
> +	void (*release_info)(struct acpi_pci_root_info *info);
> +	int (*prepare_resources)(struct acpi_pci_root_info *info, int status);
> +};
> +
> +extern struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> +					    struct acpi_pci_root_ops *ops,
> +					    struct acpi_pci_root_info *info,
> +					    void *sd);
> +
>  void acpi_pci_add_bus(struct pci_bus *bus);
>  void acpi_pci_remove_bus(struct pci_bus *bus);
>  
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241973

On 2015/10/7 1:47, Bjorn Helgaas wrote:
> Hi Jiang,
> 
> Strictly speaking, this patch by itself doesn't actually "consolidate"
> anything because it only adds acpi_pci_root_create() (which isn't called by
> anything yet), but doesn't remove the original x86 copy.
> 
<snit>
>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>> +				     struct acpi_pci_root_ops *ops,
>> +				     struct acpi_pci_root_info *info,
>> +				     void *sysdata)
>> +{
>> +	int ret, busnum = root->secondary.start;
>> +	struct acpi_device *device = root->device;
>> +	int node = acpi_get_node(device->handle);
>> +	struct pci_bus *bus;
>> +
>> +	info->root = root;
>> +	info->bridge = device;
>> +	info->ops = ops;
>> +	INIT_LIST_HEAD(&info->resources);
>> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
>> +		 root->segment, busnum);
>> +
>> +	if (ops->init_info && ops->init_info(info))
>> +		goto out_release_info;
>> +	ret = acpi_pci_probe_root_resources(info);
>> +	if (ops->prepare_resources)
>> +		ret = ops->prepare_resources(info, ret);
>> +	if (ret < 0)
>> +		goto out_release_info;
>> +	else if (ret > 0)
>> +		pci_acpi_root_add_resources(info);
> 
> This is unnecessarily complicated: you set "ret", then overwrite it if
> ops->prepare_resources.  By the time you test "ret", it's messy to
> figure out what it means.
> 
> Both ops->prepare_resources() and pci_acpi_root_add_resources()
> should be able to deal with empty resource lists, so can you do the
> following instead?
> 
>     ret = acpi_pci_probe_root_resources(info);
>     if (ret < 0)
>         goto out_release_info;
Hi Bjorn,
	Thanks for review:)
	The original code is used to handle a special case for x86,
where acpi_pci_probe_root_resources() fails but ops->prepare_resources()
succeeds. For x86, PCI host bridge resources may probed by means
other than ACPI when pci_use_crs is true (AMD and Broadcom hostbridges).
So we can't return failure when acpi_pci_probe_root_resources() fails.
+	ret = acpi_pci_probe_root_resources(info);
+	if (ops->prepare_resources)
+		ret = ops->prepare_resources(info, ret);
+	if (ret < 0)
+		goto out_release_info;

>     if (ops->prepare_resources) {
>         ret = ops->prepare_resources(info, ret);
>         if (ret < 0)
>             goto out_release_info;
>     }
>     pci_acpi_root_add_resources(info);
I will remove the redundant check of (ret > 0) in:
+	else if (ret > 0)
+		pci_acpi_root_add_resources(info);

> 
>> +	pci_add_resource(&info->resources, &root->secondary);
>> +
>> +	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>> +				  sysdata, &info->resources);
>> +	if (bus) {
> 
>     if (!bus)
>         goto out_release_info;
> 
> Then it looks like the other error paths above, and you can un-indent
> the following code, which is the normal path:
Will do this.
Thanks!
Gerry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1242344

On Thu, Oct 08, 2015 at 01:32:04PM +0800, Jiang Liu wrote:
> On 2015/10/7 1:47, Bjorn Helgaas wrote:
> >> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >> +				     struct acpi_pci_root_ops *ops,
> >> +				     struct acpi_pci_root_info *info,
> >> +				     void *sysdata)
> >> +{
> >> +	int ret, busnum = root->secondary.start;
> >> +	struct acpi_device *device = root->device;
> >> +	int node = acpi_get_node(device->handle);
> >> +	struct pci_bus *bus;
> >> +
> >> +	info->root = root;
> >> +	info->bridge = device;
> >> +	info->ops = ops;
> >> +	INIT_LIST_HEAD(&info->resources);
> >> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
> >> +		 root->segment, busnum);
> >> +
> >> +	if (ops->init_info && ops->init_info(info))
> >> +		goto out_release_info;
> >> +	ret = acpi_pci_probe_root_resources(info);
> >> +	if (ops->prepare_resources)
> >> +		ret = ops->prepare_resources(info, ret);
> >> +	if (ret < 0)
> >> +		goto out_release_info;
> >> +	else if (ret > 0)
> >> +		pci_acpi_root_add_resources(info);
> > 
> > This is unnecessarily complicated: you set "ret", then overwrite it if
> > ops->prepare_resources.  By the time you test "ret", it's messy to
> > figure out what it means.
> > 
> > Both ops->prepare_resources() and pci_acpi_root_add_resources()
> > should be able to deal with empty resource lists, so can you do the
> > following instead?
> > 
> >     ret = acpi_pci_probe_root_resources(info);
> >     if (ret < 0)
> >         goto out_release_info;
>
> 	The original code is used to handle a special case for x86,
> where acpi_pci_probe_root_resources() fails but ops->prepare_resources()
> succeeds. For x86, PCI host bridge resources may probed by means
> other than ACPI when pci_use_crs is true (AMD and Broadcom hostbridges).
> So we can't return failure when acpi_pci_probe_root_resources() fails.

That's even worse than I thought.  I take back my ack; I think this
really needs to be restructured so it does the right thing *and* reads
clearly.  Having convoluted generic code to deal with an arch-specific
special case is a recipe for breakage in the future.

Maybe you can move the non-ACPI resource probing from
prepare_resources() into acpi_pci_probe_root_resources() (you could
rename it to something more generic if that helps).

> +	ret = acpi_pci_probe_root_resources(info);
> +	if (ops->prepare_resources)
> +		ret = ops->prepare_resources(info, ret);
> +	if (ret < 0)
> +		goto out_release_info;
> 
> >     if (ops->prepare_resources) {
> >         ret = ops->prepare_resources(info, ret);
> >         if (ret < 0)
> >             goto out_release_info;
> >     }
> >     pci_acpi_root_add_resources(info);
> I will remove the redundant check of (ret > 0) in:
> +	else if (ret > 0)
> +		pci_acpi_root_add_resources(info);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1243107

On 2015/10/8 21:20, Bjorn Helgaas wrote:
> On Thu, Oct 08, 2015 at 01:32:04PM +0800, Jiang Liu wrote:
>> On 2015/10/7 1:47, Bjorn Helgaas wrote:
>>>> +struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>>> +				     struct acpi_pci_root_ops *ops,
>>>> +				     struct acpi_pci_root_info *info,
>>>> +				     void *sysdata)
>>>> +{
>>>> +	int ret, busnum = root->secondary.start;
>>>> +	struct acpi_device *device = root->device;
>>>> +	int node = acpi_get_node(device->handle);
>>>> +	struct pci_bus *bus;
>>>> +
>>>> +	info->root = root;
>>>> +	info->bridge = device;
>>>> +	info->ops = ops;
>>>> +	INIT_LIST_HEAD(&info->resources);
>>>> +	snprintf(info->name, sizeof(info->name), "PCI Bus %04x:%02x",
>>>> +		 root->segment, busnum);
>>>> +
>>>> +	if (ops->init_info && ops->init_info(info))
>>>> +		goto out_release_info;
>>>> +	ret = acpi_pci_probe_root_resources(info);
>>>> +	if (ops->prepare_resources)
>>>> +		ret = ops->prepare_resources(info, ret);
>>>> +	if (ret < 0)
>>>> +		goto out_release_info;
>>>> +	else if (ret > 0)
>>>> +		pci_acpi_root_add_resources(info);
>>>
>>> This is unnecessarily complicated: you set "ret", then overwrite it if
>>> ops->prepare_resources.  By the time you test "ret", it's messy to
>>> figure out what it means.
>>>
>>> Both ops->prepare_resources() and pci_acpi_root_add_resources()
>>> should be able to deal with empty resource lists, so can you do the
>>> following instead?
>>>
>>>     ret = acpi_pci_probe_root_resources(info);
>>>     if (ret < 0)
>>>         goto out_release_info;
>>
>> 	The original code is used to handle a special case for x86,
>> where acpi_pci_probe_root_resources() fails but ops->prepare_resources()
>> succeeds. For x86, PCI host bridge resources may probed by means
>> other than ACPI when pci_use_crs is true (AMD and Broadcom hostbridges).
>> So we can't return failure when acpi_pci_probe_root_resources() fails.
> 
> That's even worse than I thought.  I take back my ack; I think this
> really needs to be restructured so it does the right thing *and* reads
> clearly.  Having convoluted generic code to deal with an arch-specific
> special case is a recipe for breakage in the future.
> 
> Maybe you can move the non-ACPI resource probing from
> prepare_resources() into acpi_pci_probe_root_resources() (you could
> rename it to something more generic if that helps).
Hi Bjorn,
	How about this solution?
1) export acpi_pci_probe_root_resources() as a helper to arch code
2) change ACPI core code as below
        if (ops->init_info && ops->init_info(info))
                goto out_release_info;
        if (ops->prepare_resources)
                ret = ops->prepare_resources(info);
        else
                ret = acpi_pci_probe_root_resources(info);
        if (ret < 0)
                goto out_release_info;

        pci_acpi_root_add_resources(info);
        pci_add_resource(&info->resources, &root->secondary);
        bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
                                  sysdata, &info->resources);
        if (!bus)
                goto out_release_info;

By this way, if arch has special requirement, it could implement
prepare_resources callback and make use of
acpi_pci_probe_root_resources() if needed.
Thanks!
Gerry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web