diff mbox

[v5,4/6] PCI/ACPI: Consolidate common PCI host bridge code into ACPI core

Message ID 1433780448-18636-5-git-send-email-jiang.liu@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jiang Liu June 8, 2015, 4:20 p.m. UTC
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.

Tested-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/acpi/pci_root.c  |  198 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h |   23 ++++++
 2 files changed, 221 insertions(+)

Comments

Lorenzo Pieralisi June 9, 2015, 4:12 p.m. UTC | #1
On Mon, Jun 08, 2015 at 05:20:46PM +0100, Jiang Liu wrote:

[...]

> +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;

Is IORESOURCE_MEM_8AND16BIT required because of some pending patches
that will change ACPI resource filtering ? It does not seem to make
a difference in the mainline code, AFAICT.

> +	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);
> +	}

It is a question: is this loop necessary given that we are already
releasing resources in __acpi_pci_root_release_info() ?

> +	__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 segment, int node)

I do not think you need to pass segment and node, they clutter the
function signature when you can retrieve them from root, I would
make them local variables and use root->segment and acpi_get_node
in the function body to retrieve them.

> +{
> +	int ret, busnum = root->secondary.start;
> +	struct acpi_device *device = root->device;
> +	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",
> +		 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);

You go through this ret passing song and dance because we may want to
call prepare_resources even if acpi_pci_probe_root_resource failed (on
x86), correct ? I will have a further look at x86 and ia64 if we
can consolidate these ops function hooks even further.

Thanks,
Lorenzo

> +	if (ret < 0)
> +		goto out_release_info;
> +	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) {
> +		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..a76cb6f24ca1 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, int seg, int node);
> +
>  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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu June 9, 2015, 4:58 p.m. UTC | #2
On 2015/6/10 0:12, Lorenzo Pieralisi wrote:
> On Mon, Jun 08, 2015 at 05:20:46PM +0100, Jiang Liu wrote:
> 
> [...]
> 
>> +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;
> 
> Is IORESOURCE_MEM_8AND16BIT required because of some pending patches
> that will change ACPI resource filtering ? It does not seem to make
> a difference in the mainline code, AFAICT.
Hi Lorenzo,
	Sorry, the 'IORESOURCE_MEM_8AND16BIT' is leaked into this patch
from another bugfix patch. It should be removed.

>> +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);
>> +	}
> 
> It is a question: is this loop necessary given that we are already
> releasing resources in __acpi_pci_root_release_info() ?
Function pci_create_root_bus() moves resources from info->resources list
onto bridge->windows list, so an ACPI resource will be either on
info->resources or bridge->windows. Thus we need to deal with both
info->resources and bridge->windows.

> 
>> +	__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 segment, int node)
> 
> I do not think you need to pass segment and node, they clutter the
> function signature when you can retrieve them from root, I would
> make them local variables and use root->segment and acpi_get_node
> in the function body to retrieve them.
On x86, node and segment may be overridden under certain conditions.
For example, segment will always be 0 if 'pci_ignore_seg' is set.

>> +{
>> +	int ret, busnum = root->secondary.start;
>> +	struct acpi_device *device = root->device;
>> +	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",
>> +		 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);
> 
> You go through this ret passing song and dance because we may want to
> call prepare_resources even if acpi_pci_probe_root_resource failed (on
> x86), correct ? I will have a further look at x86 and ia64 if we
> can consolidate these ops function hooks even further.
Yes. X86 uses flag 'pci_use_crs' to choose ACPI parsed resources or
other method parsed resources. This provides user a way to work
around some bios issues.
Thanks!
Gerry

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi June 10, 2015, 4:48 p.m. UTC | #3
On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote:
> On 2015/6/10 0:12, Lorenzo Pieralisi 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 segment, int node)
> > 
> > I do not think you need to pass segment and node, they clutter the
> > function signature when you can retrieve them from root, I would
> > make them local variables and use root->segment and acpi_get_node
> > in the function body to retrieve them.
> On x86, node and segment may be overridden under certain conditions.
> For example, segment will always be 0 if 'pci_ignore_seg' is set.

Ok, so the question would be then why do you not override the value
in root->segment then (actually, is it *correct* to leave the segment
value in root-> unchanged even if it is overriden) ?

Anyway, node is just used for a printk, why do not you add segment and
node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you
simplify the code, it is not easy to parse.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu June 10, 2015, 5:19 p.m. UTC | #4
On 2015/6/11 0:48, Lorenzo Pieralisi wrote:
> On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote:
>> On 2015/6/10 0:12, Lorenzo Pieralisi 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 segment, int node)
>>>
>>> I do not think you need to pass segment and node, they clutter the
>>> function signature when you can retrieve them from root, I would
>>> make them local variables and use root->segment and acpi_get_node
>>> in the function body to retrieve them.
>> On x86, node and segment may be overridden under certain conditions.
>> For example, segment will always be 0 if 'pci_ignore_seg' is set.
> 
> Ok, so the question would be then why do you not override the value
> in root->segment then (actually, is it *correct* to leave the segment
> value in root-> unchanged even if it is overriden) ?
> 
> Anyway, node is just used for a printk, why do not you add segment and
> node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you
> simplify the code, it is not easy to parse.
Hi Lorenzo,
	I used the complex prototype to explicitly reminder callers
that you need to prepare all these information. If simpler interface
is preferred, we could pack all ops, sysdata, segment, node, root
into info, that there's will be only one parameter "info" left.
The drawback is that it will add several lines of code to each arch
using this interface. So prefer simpler interface or less lines of code?:)
Thanks!
Gerry

> 
> Thanks,
> Lorenzo
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi June 11, 2015, 4:18 p.m. UTC | #5
Hi Gerry,

On Wed, Jun 10, 2015 at 06:19:13PM +0100, Jiang Liu wrote:
> On 2015/6/11 0:48, Lorenzo Pieralisi wrote:
> > On Tue, Jun 09, 2015 at 05:58:15PM +0100, Jiang Liu wrote:
> >> On 2015/6/10 0:12, Lorenzo Pieralisi 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 segment, int node)
> >>>
> >>> I do not think you need to pass segment and node, they clutter the
> >>> function signature when you can retrieve them from root, I would
> >>> make them local variables and use root->segment and acpi_get_node
> >>> in the function body to retrieve them.
> >> On x86, node and segment may be overridden under certain conditions.
> >> For example, segment will always be 0 if 'pci_ignore_seg' is set.
> > 
> > Ok, so the question would be then why do you not override the value
> > in root->segment then (actually, is it *correct* to leave the segment
> > value in root-> unchanged even if it is overriden) ?
> > 
> > Anyway, node is just used for a printk, why do not you add segment and
> > node to acpi_pci_root_info ? Just cosmetic stuff, trying to help you
> > simplify the code, it is not easy to parse.
> Hi Lorenzo,
> 	I used the complex prototype to explicitly reminder callers
> that you need to prepare all these information. If simpler interface
> is preferred, we could pack all ops, sysdata, segment, node, root
> into info, that there's will be only one parameter "info" left.
> The drawback is that it will add several lines of code to each arch
> using this interface. So prefer simpler interface or less lines of code?:)

I was just referring to segment and node parameters, I think that having
them in the parameters list is overkill and they can be encapsulated.

With the changes discussed previously feel free to add my:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> Thanks!
> Gerry
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 29, 2015, 8:24 p.m. UTC | #6
On Tue, Jun 09, 2015 at 12:20:46AM +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.

I assume most of this code is copied from somewhere else.  You should
mention where it came from.

> Tested-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/acpi/pci_root.c  |  198 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-acpi.h |   23 ++++++
>  2 files changed, 221 insertions(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 1b5569c092c6..70f851dc0051 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -656,6 +656,204 @@ static void acpi_pci_root_remove(struct acpi_device *device)
>  	kfree(root);
>  }
>  
> +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);
> +	}

Please use this style to minimize indentation and return errors as soon as
possible:

    if (ret < 0) {
	dev_warn(...)
	return ret;
    }

    if (ret == 0) {
	dev_dbg(...);
	return 0;
    }

    resource_list_for_each_entry_safe(...) {
    }
    return ret;

> +	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;

In what circumstance could "info" be NULL?  As far as I can tell, the only
way it could be NULL is if we're called from acpi_pci_root_release_info()
and the bridge->release_data element got scribbled on after we called
pci_set_host_bridge_release().  In that case, I'd rather oops than silently
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 segment, int node)
> +{
> +	int ret, busnum = root->secondary.start;
> +	struct acpi_device *device = root->device;
> +	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",
> +		 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);

It's goofy to pass the return value from acpi_pci_probe_root_resources()
into prepare_resources(), especially since the local context gives no clue
about what "ret" means.

Neither the x86 nor the ia64 version of pci_acpi_root_prepare_resources()
actually does anything useful with "ret" except return it.

I think you should just make acpi_pci_probe_root_resources() void.  It will
create a list of zero or more resources from _CRS.  If _CRS fails, you
already print a diagnostic and the list will be empty.  If _CRS succeeds
and there are no windows, you also print a (debug) diagnostic and the list
will be empty.  The pci_acpi_root_prepare_resources() implementations will
deal with empty lists just fine.

I think the change from your code here is that we would create the root bus
and enumerate it even if _CRS fails.  I think that's OK: we'll find devices
and we'll learn what resources they want, but we won't have anything to
assign them.  I think that's OK and it will make this code simpler.

> +	if (ret < 0)
> +		goto out_release_info;
> +	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 this code can be un-indented:

> +		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..a76cb6f24ca1 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, int seg, int node);

Drop the "extern" here to match the declarations below.  I know there's
still a mix in this file, but my preference is to skip them in new code.

>  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-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas July 29, 2015, 8:54 p.m. UTC | #7
On Wed, Jun 10, 2015 at 12:58:15AM +0800, Jiang Liu wrote:
> On 2015/6/10 0:12, Lorenzo Pieralisi wrote:
> > On Mon, Jun 08, 2015 at 05:20:46PM +0100, Jiang Liu 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 segment, int node)
> > 
> > I do not think you need to pass segment and node, they clutter the
> > function signature when you can retrieve them from root, I would
> > make them local variables and use root->segment and acpi_get_node
> > in the function body to retrieve them.
> On x86, node and segment may be overridden under certain conditions.
> For example, segment will always be 0 if 'pci_ignore_seg' is set.

I agree we should drop the segment and node parameters.  Having the
information in two places means we can make a mistake, and we can easily
avoid that possibility by only keeping it in one place.

I think it is a bug that we don't set root->segment to zero when we have
pci_ignore_seg.  Sorry, that's my fault from 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300").

I don't understand the issue about overriding the node number.  Can you
explain it?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 1b5569c092c6..70f851dc0051 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -656,6 +656,204 @@  static void acpi_pci_root_remove(struct acpi_device *device)
 	kfree(root);
 }
 
+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 segment, int node)
+{
+	int ret, busnum = root->secondary.start;
+	struct acpi_device *device = root->device;
+	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",
+		 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);
+	pci_add_resource(&info->resources, &root->secondary);
+
+	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+				  sysdata, &info->resources);
+	if (bus) {
+		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..a76cb6f24ca1 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, int seg, int node);
+
 void acpi_pci_add_bus(struct pci_bus *bus);
 void acpi_pci_remove_bus(struct pci_bus *bus);