drivers: port PCIe designware to new DT parsing API

In the function of_pci_get_host_bridge_resources() if the parsing of ranges fails, previously allocated resources inclusive of bus_range are not freed and are not expected to be freed by the function caller on error return.

This patch fixes the issues by adding code that properly frees resources and bus_range before exiting the function with an error return value.

Cc Arnd Bergmann <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Rob Herring <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

---
 drivers/of/of_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/of/of_pci.c +1 -0

index 88471d3..6fbfe99 100644
@@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct of_pci_range_parser parser;
 	char range_type[4];
 	int err;
+	struct pci_host_bridge_window *window;
 
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;

Signature · 1 line

2.2.1

Quoted 9 lines

drivers/of/of_pci.c +1 -0

@@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct of_pci_range_parser parser;
 	char range_type[4];
 	int err;
+	struct pci_host_bridge_window *window;
 
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;

drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct device_node *dev, · line 235

Quoted 10 lines

drivers/of/of_pci.c +3 -0

@@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 conversion_failed:
 	kfree(res);
 parse_failed:
+	list_for_each_entry(window, resources, list)
+		kfree(window->res);
 	pci_free_resource_list(resources);
+	kfree(bus_range);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);

Hi Lorenzo et all,

Here is my personal view and I am happy to hear from others on the desired behaviour:

When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment.

The historical reason why the function was written that way was because at some moment after parsing I've had an additional step where arches could cleanup / veto the list and they could return an error value to signal their discontent. Also I was (am) not sure how lenient we could be with the device tree not being sane (at least one host bridge binding lists the config space as a range, which was accepted as broken).

So, from that point of view, I would NAK this patch, as the function works as intended. If others find this mode of operation too convoluted, then the patch should probably make clear that cleanup only needs to be done on function returning success.

Best regards,

Quoted 4 lines

Signature · 1 line

2.2.1

Signature · 8 lines

====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:

[...]

Quoted 25 lines

Quoted 9 lines

drivers/of/of_pci.c +1 -0

@@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct of_pci_range_parser parser;
 	char range_type[4];
 	int err;
+	struct pci_host_bridge_window *window;
 
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;

drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct device_node *dev, · line 235

Quoted 10 lines

drivers/of/of_pci.c +3 -0

@@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 conversion_failed:
 	kfree(res);
 parse_failed:
+	list_for_each_entry(window, resources, list)
+		kfree(window->res);
 	pci_free_resource_list(resources);
+	kfree(bus_range);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);

Hi Lorenzo et all,

Here is my personal view and I am happy to hear from others on the desired behaviour:

Quoted 6 lines

When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment.

That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them.

So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do).

Lorenzo

Quoted 19 lines

The historical reason why the function was written that way was because at some moment after parsing I've had an additional step where arches could cleanup / veto the list and they could return an error value to signal their discontent. Also I was (am) not sure how lenient we could be with the device tree not being sane (at least one host bridge binding lists the config space as a range, which was accepted as broken).

So, from that point of view, I would NAK this patch, as the function works as intended. If others find this mode of operation too convoluted, then the patch should probably make clear that cleanup only needs to be done on function returning success.

Best regards,

Quoted 4 lines

Signature · 1 line

2.2.1

Quoted 36 lines · Lorenzo Pieralisi

On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:

[...]

Quoted 31 lines

Quoted 9 lines

drivers/of/of_pci.c +1 -0

@@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct of_pci_range_parser parser;
 	char range_type[4];
 	int err;
+	struct pci_host_bridge_window *window;
 
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;

drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct device_node *dev, · line 235

Quoted 10 lines

drivers/of/of_pci.c +3 -0

@@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 conversion_failed:
 	kfree(res);
 parse_failed:
+	list_for_each_entry(window, resources, list)
+		kfree(window->res);
 	pci_free_resource_list(resources);
+	kfree(bus_range);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);

Hi Lorenzo et all,

Here is my personal view and I am happy to hear from others on the desired behaviour:

When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment.

Quoted 3 lines

That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them.

I stand corrected. Your patch is needed.

Quoted 37 lines · Liviu Dudau

Quoted 36 lines · Lorenzo Pieralisi

On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:

[...]

Quoted 31 lines

Quoted 9 lines

drivers/of/of_pci.c +1 -0

@@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct of_pci_range_parser parser;
 	char range_type[4];
 	int err;
+	struct pci_host_bridge_window *window;
 
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;

drivers/of/of_pci.c int of_pci_get_host_bridge_resources(struct device_node *dev, · line 235

Quoted 10 lines

drivers/of/of_pci.c +3 -0

@@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 conversion_failed:
 	kfree(res);
 parse_failed:
+	list_for_each_entry(window, resources, list)
+		kfree(window->res);
 	pci_free_resource_list(resources);
+	kfree(bus_range);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);

Hi Lorenzo et all,

Here is my personal view and I am happy to hear from others on the desired behaviour:

When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment.

Quoted 5 lines

Quoted 3 lines

That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them.

I stand corrected. Your patch is needed.

Mind acking it ? I will ask Bjorn to take it then.

Thanks,
Lorenzo

The introduction of the DT API:

of_pci_get_host_bridge_resources()

allows to retrieve PCI resources from the device tree in a generic way.

Some PCI host controllers, in addition to standard IO and MEM resources and corresponding CPU addresses, require to retrieve the addresses that are used for bus routing of IO/MEM regions as seen at the bus level corresponding to the bus segment the host controller is sitting on, so that the host controller can programme its inbound/outbound memory regions correctly to respond to bus transactions coming from the CPU on the host controller bus ports.

The DT generic layer provides functions that carry out the PCI ranges <-> resources translation from PCI addresses to CPU addresses (as seen at the CPU top level bus, through all required hierarchical bus layers), but there is no generic API to retrieve the PCI range CPU untranslated addresses, so PCI host controllers have to do it in a driver specific way through the DT property parsing methods.

This patchset converts the PCIe designware host controller driver to use the API:

of_pci_get_host_bridge_resources()

in order to retrieve the PCI regions resources. To achieve generality, it also augments the parsing API so that a new DT struct is created out of PCI ranges, that contains the PCI resource and the range parser itself so that it can be passed back to PCI host controllers drivers that can, if needed, use it to parse the CPU untranslated addresses corresponding to the resource in question.

The first patch in the series is a fix/update for the parsing function:

of_pci_get_host_bridge_resources()

It is part of the RFC series since it is a controversial fix to be discussed before getting merged.

Patch [2-3] augment the parsing API and convert the PCIe designware driver code.

The current API stays unchanged, so that the existing drivers using it do not need patching and changes are limited.

Tested on an iMX6 Sabrelite board.

Cc Arnd Bergmann <[email protected]>

Cc Liviu Dudau <[email protected]>

Cc Mohit Kumar <[email protected]>

Cc Jingoo Han <[email protected]>

Cc Bjorn Helgaas <[email protected]>

Cc Rob Herring <[email protected]>

Lorenzo Pieralisi (3):
drivers: of: fix resources freeing in
of_pci_get_host_bridge_resources()
drivers: of: of_pci_get_host_bridge_resources() range parsing update
drivers: pci: host: update the pcie designware driver to new range
parsing API

 drivers/of/of_pci.c                |  36 +++++++--
 drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++--------------------
 drivers/pci/host/pcie-designware.h |   5 +-
 include/linux/of_address.h         |   5 ++
 4 files changed, 104 insertions(+), 87 deletions(-)

--
2.2.1

I don't really like exposing ranges to host drivers. We've worked to not do that. So perhaps we need to rethink the API. I think we need to provide each range as a pair of resources which are the CPU address and PCI address. Perhaps an iterator is kind of pointless here. We do different things for each one. Are there cases with more than a single i/o space, non-prefetch memory and prefetch memory range? Perhaps we should just get the i/o and memory resources as separate calls. Just tossing out some ideas here.

Quoted 9 lines · On Monday 19 January 2015 10:40:39 Rob Herring wrote:

I don't really like exposing ranges to host drivers. We've worked to not do that. So perhaps we need to rethink the API. I think we need to provide each range as a pair of resources which are the CPU address and PCI address. Perhaps an iterator is kind of pointless here. We do different things for each one. Are there cases with more than a single i/o space, non-prefetch memory and prefetch memory range? Perhaps we should just get the i/o and memory resources as separate calls. Just tossing out some ideas here.

Nice idea, that could be similar to platform_get_resource().

We probably also need the distinction between CPU address and (parent) bus address here. In most drivers they are the same, but we actually need to program the latter one into the PCI host bridge registers.