[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>

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

[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>

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

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/of/of_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 88471d3..6fbfe99 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -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;
@@ -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);
-- 
2.2.1

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

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 <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/of/of_pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 88471d3..6fbfe99 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -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;
@@ -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);
-- 
2.2.1

[RFC PATCH 2/3] drivers: of: of_pci_get_host_bridge_resources() range parsing update

[Lorenzo Pieralisi]

Some host controllers require local bus physical addresses to
programme inbound/outbound requests from the bus hierarchy to be routed
properly through the PCI bus beyond the host controller. Owing
to bus address size conversion, the bus local addresses may be different
from the addresses as seen from the CPU (which are translated by DT core
code), so the PCI range parsing function:

of_pci_get_host_bridge_resources()

should be augmented in order to store the range parser along
with the parsed resource so that the CPU untranslated address can
be retrieved by the driver from the corresponding PCI range if needed.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/of/of_pci.c        | 38 +++++++++++++++++++++++++++++---------
 include/linux/of_address.h |  5 +++++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 6fbfe99..f0e576f 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -144,6 +144,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct resource *bus_range;
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct of_pci_resource *of_pci_res;
 	char range_type[4];
 	int err;
 	struct pci_host_bridge_window *window;
@@ -151,12 +152,14 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;
 
-	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-	if (!bus_range)
+	of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL);
+	if (!of_pci_res)
 		return -ENOMEM;
 
 	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
 
+	bus_range = &of_pci_res->res;
+
 	err = of_pci_parse_bus_range(dev, bus_range);
 	if (err) {
 		bus_range->start = busno;
@@ -195,17 +198,29 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
 			continue;
 
-		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
-		if (!res) {
+		of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL);
+		if (!of_pci_res) {
 			err = -ENOMEM;
 			goto parse_failed;
 		}
+		res = &of_pci_res->res;
 
 		err = of_pci_range_to_resource(&range, dev, res);
 		if (err)
 			goto conversion_failed;
 
-		if (resource_type(res) == IORESOURCE_IO) {
+		/* Stash the range parser */
+		of_pci_res->parser = parser;
+		/*
+		 * for_each_of_pci_range increments the range pointer
+		 * in the parser, so that it is ready to parse the
+		 * following range while looping; rewind the range pointer
+		 * to its current value to pass it to the drivers with its
+		 * initial value.
+		 */
+		of_pci_res->parser.range -= parser.np;
+
+		if  (resource_type(res) == IORESOURCE_IO) {
 			if (!io_base) {
 				pr_err("I/O range found for %s. Please provide an io_base pointer to save CPU base address\n",
 					dev->full_name);
@@ -224,12 +239,17 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	return 0;
 
 conversion_failed:
-	kfree(res);
+	kfree(of_pci_res);
 parse_failed:
-	list_for_each_entry(window, resources, list)
-		kfree(window->res);
+	list_for_each_entry(window, resources, list) {
+		of_pci_res = container_of(window->res, struct of_pci_resource,
+					  res);
+		kfree(of_pci_res);
+	}
 	pci_free_resource_list(resources);
-	kfree(bus_range);
+	of_pci_res = container_of(bus_range, struct of_pci_resource,
+				  res);
+	kfree(of_pci_res);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81b..e4005be 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -20,6 +20,11 @@ struct of_pci_range {
 	u32 flags;
 };
 
+struct of_pci_resource {
+	struct resource res;
+	struct of_pci_range_parser parser;
+};
+
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
-- 
2.2.1

[RFC PATCH 2/3] drivers: of: of_pci_get_host_bridge_resources() range parsing update

[Lorenzo Pieralisi]

Some host controllers require local bus physical addresses to
programme inbound/outbound requests from the bus hierarchy to be routed
properly through the PCI bus beyond the host controller. Owing
to bus address size conversion, the bus local addresses may be different
from the addresses as seen from the CPU (which are translated by DT core
code), so the PCI range parsing function:

of_pci_get_host_bridge_resources()

should be augmented in order to store the range parser along
with the parsed resource so that the CPU untranslated address can
be retrieved by the driver from the corresponding PCI range if needed.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/of/of_pci.c        | 38 +++++++++++++++++++++++++++++---------
 include/linux/of_address.h |  5 +++++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 6fbfe99..f0e576f 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -144,6 +144,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	struct resource *bus_range;
 	struct of_pci_range range;
 	struct of_pci_range_parser parser;
+	struct of_pci_resource *of_pci_res;
 	char range_type[4];
 	int err;
 	struct pci_host_bridge_window *window;
@@ -151,12 +152,14 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	if (io_base)
 		*io_base = (resource_size_t)OF_BAD_ADDR;
 
-	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-	if (!bus_range)
+	of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL);
+	if (!of_pci_res)
 		return -ENOMEM;
 
 	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
 
+	bus_range = &of_pci_res->res;
+
 	err = of_pci_parse_bus_range(dev, bus_range);
 	if (err) {
 		bus_range->start = busno;
@@ -195,17 +198,29 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
 			continue;
 
-		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
-		if (!res) {
+		of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL);
+		if (!of_pci_res) {
 			err = -ENOMEM;
 			goto parse_failed;
 		}
+		res = &of_pci_res->res;
 
 		err = of_pci_range_to_resource(&range, dev, res);
 		if (err)
 			goto conversion_failed;
 
-		if (resource_type(res) == IORESOURCE_IO) {
+		/* Stash the range parser */
+		of_pci_res->parser = parser;
+		/*
+		 * for_each_of_pci_range increments the range pointer
+		 * in the parser, so that it is ready to parse the
+		 * following range while looping; rewind the range pointer
+		 * to its current value to pass it to the drivers with its
+		 * initial value.
+		 */
+		of_pci_res->parser.range -= parser.np;
+
+		if  (resource_type(res) == IORESOURCE_IO) {
 			if (!io_base) {
 				pr_err("I/O range found for %s. Please provide an io_base pointer to save CPU base address\n",
 					dev->full_name);
@@ -224,12 +239,17 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
 	return 0;
 
 conversion_failed:
-	kfree(res);
+	kfree(of_pci_res);
 parse_failed:
-	list_for_each_entry(window, resources, list)
-		kfree(window->res);
+	list_for_each_entry(window, resources, list) {
+		of_pci_res = container_of(window->res, struct of_pci_resource,
+					  res);
+		kfree(of_pci_res);
+	}
 	pci_free_resource_list(resources);
-	kfree(bus_range);
+	of_pci_res = container_of(bus_range, struct of_pci_resource,
+				  res);
+	kfree(of_pci_res);
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81b..e4005be 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -20,6 +20,11 @@ struct of_pci_range {
 	u32 flags;
 };
 
+struct of_pci_resource {
+	struct resource res;
+	struct of_pci_range_parser parser;
+};
+
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
-- 
2.2.1

[RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API

[Lorenzo Pieralisi]

The new OF pci API of_pci_get_host_bridge_resources() implemented by:

'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")'

and subsequent changes allow to parse PCI DT ranges in a generic way
through a new API that can be used to consolidate the DT range parsing for
all PCIe controllers in the kernel that are configured through DT.

This patch updates the PCIe designware driver in order for it to use the
new OF range parsing API and remove driver specific parsing code.

Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
Cc: Mohit Kumar <mohit.kumar-qxv4g6HH51o@public.gmane.org>
Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++--------------------
 drivers/pci/host/pcie-designware.h |   5 +-
 2 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index df781cd..95cce40 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -69,8 +69,6 @@
 
 static struct hw_pci dw_pci;
 
-static unsigned long global_io_offset;
-
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
 	BUG_ON(!sys->private_data);
@@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
-	struct of_pci_range range;
-	struct of_pci_range_parser parser;
 	struct resource *cfg_res;
-	u32 val, na, ns;
+	u32 val, na;
 	const __be32 *addrp;
 	int i, index, ret;
+	resource_size_t io_base;
+	struct pci_host_bridge_window *win;
+	struct of_pci_resource *of_pci_res;
 
-	/* Find the address cell size and the number of cells in order to get
-	 * the untranslated address.
+	/*
+	 * Find the address cell size (ie must be 3 for DT PCI addresses) to
+	 * get the untranslated address.
 	 */
 	of_property_read_u32(np, "#address-cells", &na);
-	ns = of_n_size_cells(np);
+
+	INIT_LIST_HEAD(&pp->resources);
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
@@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 		/* Find the untranslated configuration space address */
 		index = of_property_match_string(np, "reg-names", "config");
 		addrp = of_get_address(np, index, NULL, NULL);
-		pp->cfg0_mod_base = of_read_number(addrp, ns);
+		pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np));
 		pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size;
 	} else {
 		dev_err(pp->dev, "missing *config* reg space\n");
 	}
 
-	if (of_pci_range_parser_init(&parser, np)) {
-		dev_err(pp->dev, "missing ranges property\n");
-		return -EINVAL;
-	}
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources,
+					       &io_base);
+	if (ret)
+		return ret;
 
-	/* Get the I/O and memory ranges from DT */
-	for_each_of_pci_range(&parser, &range) {
-		unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
-
-		if (restype == IORESOURCE_IO) {
-			of_pci_range_to_resource(&range, np, &pp->io);
-			pp->io.name = "I/O";
-			pp->io.start = max_t(resource_size_t,
-					     PCIBIOS_MIN_IO,
-					     range.pci_addr + global_io_offset);
-			pp->io.end = min_t(resource_size_t,
+	list_for_each_entry(win, &pp->resources, list) {
+		u64 pci_addr;
+		struct resource *res = win->res;
+
+		pci_addr = res->start - win->offset;
+		of_pci_res = container_of(res, struct of_pci_resource, res);
+
+		if (resource_type(res) == IORESOURCE_IO) {
+			res->name = "I/O";
+			res->start = max_t(resource_size_t, PCIBIOS_MIN_IO,
+					   pci_addr);
+			res->end = min_t(resource_size_t,
 					   IO_SPACE_LIMIT,
-					   range.pci_addr + range.size
-					   + global_io_offset - 1);
-			pp->io_size = resource_size(&pp->io);
-			pp->io_bus_addr = range.pci_addr;
-			pp->io_base = range.cpu_addr;
+					   pci_addr + resource_size(res) - 1);
+			pp->io_size = resource_size(res);
+			pp->io_bus_addr = pci_addr;
+			pp->io_base = io_base;
 
 			/* Find the untranslated IO space address */
-			pp->io_mod_base = of_read_number(parser.range -
-							 parser.np + na, ns);
+			pp->io_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
+			pci_remap_iospace(res, io_base);
 		}
-		if (restype == IORESOURCE_MEM) {
-			of_pci_range_to_resource(&range, np, &pp->mem);
-			pp->mem.name = "MEM";
-			pp->mem_size = resource_size(&pp->mem);
-			pp->mem_bus_addr = range.pci_addr;
+
+		if (resource_type(res) == IORESOURCE_MEM) {
+			res->name = "MEM";
+			pp->mem_size = resource_size(res);
+			pp->mem_bus_addr = pci_addr;
+			pp->mem_base = res->start;
 
 			/* Find the untranslated MEM space address */
-			pp->mem_mod_base = of_read_number(parser.range -
-							  parser.np + na, ns);
+			pp->mem_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 		}
-		if (restype == 0) {
-			of_pci_range_to_resource(&range, np, &pp->cfg);
-			pp->cfg0_size = resource_size(&pp->cfg)/2;
-			pp->cfg1_size = resource_size(&pp->cfg)/2;
-			pp->cfg0_base = pp->cfg.start;
-			pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
+
+		if (resource_type(res) == 0) {
+			pp->cfg0_size = resource_size(res)/2;
+			pp->cfg1_size = resource_size(res)/2;
+			pp->cfg0_base = res->start;
+			pp->cfg1_base = res->start + pp->cfg0_size;
 
 			/* Find the untranslated configuration space address */
-			pp->cfg0_mod_base = of_read_number(parser.range -
-							   parser.np + na, ns);
+			pp->cfg0_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 			pp->cfg1_mod_base = pp->cfg0_mod_base +
 					    pp->cfg0_size;
-		}
-	}
-
-	ret = of_pci_parse_bus_range(np, &pp->busn);
-	if (ret < 0) {
-		pp->busn.name = np->name;
-		pp->busn.start = 0;
-		pp->busn.end = 0xff;
-		pp->busn.flags = IORESOURCE_BUS;
-		dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n",
-			ret, &pp->busn);
-	}
 
-	if (!pp->dbi_base) {
-		pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
-					resource_size(&pp->cfg));
-		if (!pp->dbi_base) {
-			dev_err(pp->dev, "error with ioremap\n");
-			return -ENOMEM;
+			if (!pp->dbi_base) {
+				pp->dbi_base = devm_ioremap(pp->dev,
+						res->start,
+						resource_size(res));
+				if (!pp->dbi_base) {
+					dev_err(pp->dev, "error with ioremap\n");
+					return -ENOMEM;
+				}
+			}
 		}
-	}
 
-	pp->mem_base = pp->mem.start;
+	}
 
 	if (!pp->va_cfg0_base) {
 		pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
@@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = {
 static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct pcie_port *pp;
+	struct pci_host_bridge_window *win, *tmp;
 
 	pp = sys_to_pcie(sys);
+	/* remove the configuration aperture in case it is there */
+	list_for_each_entry_safe(win, tmp, &pp->resources, list)
+		if (!resource_type(win->res))
+			list_del(&win->list);
 
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);
+	list_splice(&pp->resources, &sys->resources);
 
 	return 1;
 }
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..2479e9c 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -42,10 +42,7 @@ struct pcie_port {
 	u64			mem_mod_base;
 	phys_addr_t		mem_bus_addr;
 	u32			mem_size;
-	struct resource		cfg;
-	struct resource		io;
-	struct resource		mem;
-	struct resource		busn;
+	struct list_head	resources;
 	int			irq;
 	u32			lanes;
 	struct pcie_host_ops	*ops;
-- 
2.2.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API

[Lorenzo Pieralisi]

The new OF pci API of_pci_get_host_bridge_resources() implemented by:

'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")'

and subsequent changes allow to parse PCI DT ranges in a generic way
through a new API that can be used to consolidate the DT range parsing for
all PCIe controllers in the kernel that are configured through DT.

This patch updates the PCIe designware driver in order for it to use the
new OF range parsing API and remove driver specific parsing code.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++--------------------
 drivers/pci/host/pcie-designware.h |   5 +-
 2 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index df781cd..95cce40 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -69,8 +69,6 @@
 
 static struct hw_pci dw_pci;
 
-static unsigned long global_io_offset;
-
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
 	BUG_ON(!sys->private_data);
@@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
-	struct of_pci_range range;
-	struct of_pci_range_parser parser;
 	struct resource *cfg_res;
-	u32 val, na, ns;
+	u32 val, na;
 	const __be32 *addrp;
 	int i, index, ret;
+	resource_size_t io_base;
+	struct pci_host_bridge_window *win;
+	struct of_pci_resource *of_pci_res;
 
-	/* Find the address cell size and the number of cells in order to get
-	 * the untranslated address.
+	/*
+	 * Find the address cell size (ie must be 3 for DT PCI addresses) to
+	 * get the untranslated address.
 	 */
 	of_property_read_u32(np, "#address-cells", &na);
-	ns = of_n_size_cells(np);
+
+	INIT_LIST_HEAD(&pp->resources);
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
@@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 		/* Find the untranslated configuration space address */
 		index = of_property_match_string(np, "reg-names", "config");
 		addrp = of_get_address(np, index, NULL, NULL);
-		pp->cfg0_mod_base = of_read_number(addrp, ns);
+		pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np));
 		pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size;
 	} else {
 		dev_err(pp->dev, "missing *config* reg space\n");
 	}
 
-	if (of_pci_range_parser_init(&parser, np)) {
-		dev_err(pp->dev, "missing ranges property\n");
-		return -EINVAL;
-	}
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources,
+					       &io_base);
+	if (ret)
+		return ret;
 
-	/* Get the I/O and memory ranges from DT */
-	for_each_of_pci_range(&parser, &range) {
-		unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
-
-		if (restype == IORESOURCE_IO) {
-			of_pci_range_to_resource(&range, np, &pp->io);
-			pp->io.name = "I/O";
-			pp->io.start = max_t(resource_size_t,
-					     PCIBIOS_MIN_IO,
-					     range.pci_addr + global_io_offset);
-			pp->io.end = min_t(resource_size_t,
+	list_for_each_entry(win, &pp->resources, list) {
+		u64 pci_addr;
+		struct resource *res = win->res;
+
+		pci_addr = res->start - win->offset;
+		of_pci_res = container_of(res, struct of_pci_resource, res);
+
+		if (resource_type(res) == IORESOURCE_IO) {
+			res->name = "I/O";
+			res->start = max_t(resource_size_t, PCIBIOS_MIN_IO,
+					   pci_addr);
+			res->end = min_t(resource_size_t,
 					   IO_SPACE_LIMIT,
-					   range.pci_addr + range.size
-					   + global_io_offset - 1);
-			pp->io_size = resource_size(&pp->io);
-			pp->io_bus_addr = range.pci_addr;
-			pp->io_base = range.cpu_addr;
+					   pci_addr + resource_size(res) - 1);
+			pp->io_size = resource_size(res);
+			pp->io_bus_addr = pci_addr;
+			pp->io_base = io_base;
 
 			/* Find the untranslated IO space address */
-			pp->io_mod_base = of_read_number(parser.range -
-							 parser.np + na, ns);
+			pp->io_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
+			pci_remap_iospace(res, io_base);
 		}
-		if (restype == IORESOURCE_MEM) {
-			of_pci_range_to_resource(&range, np, &pp->mem);
-			pp->mem.name = "MEM";
-			pp->mem_size = resource_size(&pp->mem);
-			pp->mem_bus_addr = range.pci_addr;
+
+		if (resource_type(res) == IORESOURCE_MEM) {
+			res->name = "MEM";
+			pp->mem_size = resource_size(res);
+			pp->mem_bus_addr = pci_addr;
+			pp->mem_base = res->start;
 
 			/* Find the untranslated MEM space address */
-			pp->mem_mod_base = of_read_number(parser.range -
-							  parser.np + na, ns);
+			pp->mem_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 		}
-		if (restype == 0) {
-			of_pci_range_to_resource(&range, np, &pp->cfg);
-			pp->cfg0_size = resource_size(&pp->cfg)/2;
-			pp->cfg1_size = resource_size(&pp->cfg)/2;
-			pp->cfg0_base = pp->cfg.start;
-			pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
+
+		if (resource_type(res) == 0) {
+			pp->cfg0_size = resource_size(res)/2;
+			pp->cfg1_size = resource_size(res)/2;
+			pp->cfg0_base = res->start;
+			pp->cfg1_base = res->start + pp->cfg0_size;
 
 			/* Find the untranslated configuration space address */
-			pp->cfg0_mod_base = of_read_number(parser.range -
-							   parser.np + na, ns);
+			pp->cfg0_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 			pp->cfg1_mod_base = pp->cfg0_mod_base +
 					    pp->cfg0_size;
-		}
-	}
-
-	ret = of_pci_parse_bus_range(np, &pp->busn);
-	if (ret < 0) {
-		pp->busn.name = np->name;
-		pp->busn.start = 0;
-		pp->busn.end = 0xff;
-		pp->busn.flags = IORESOURCE_BUS;
-		dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n",
-			ret, &pp->busn);
-	}
 
-	if (!pp->dbi_base) {
-		pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
-					resource_size(&pp->cfg));
-		if (!pp->dbi_base) {
-			dev_err(pp->dev, "error with ioremap\n");
-			return -ENOMEM;
+			if (!pp->dbi_base) {
+				pp->dbi_base = devm_ioremap(pp->dev,
+						res->start,
+						resource_size(res));
+				if (!pp->dbi_base) {
+					dev_err(pp->dev, "error with ioremap\n");
+					return -ENOMEM;
+				}
+			}
 		}
-	}
 
-	pp->mem_base = pp->mem.start;
+	}
 
 	if (!pp->va_cfg0_base) {
 		pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
@@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = {
 static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct pcie_port *pp;
+	struct pci_host_bridge_window *win, *tmp;
 
 	pp = sys_to_pcie(sys);
+	/* remove the configuration aperture in case it is there */
+	list_for_each_entry_safe(win, tmp, &pp->resources, list)
+		if (!resource_type(win->res))
+			list_del(&win->list);
 
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);
+	list_splice(&pp->resources, &sys->resources);
 
 	return 1;
 }
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..2479e9c 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -42,10 +42,7 @@ struct pcie_port {
 	u64			mem_mod_base;
 	phys_addr_t		mem_bus_addr;
 	u32			mem_size;
-	struct resource		cfg;
-	struct resource		io;
-	struct resource		mem;
-	struct resource		busn;
+	struct list_head	resources;
 	int			irq;
 	u32			lanes;
 	struct pcie_host_ops	*ops;
-- 
2.2.1

[RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API

[Lorenzo Pieralisi]

The new OF pci API of_pci_get_host_bridge_resources() implemented by:

'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")'

and subsequent changes allow to parse PCI DT ranges in a generic way
through a new API that can be used to consolidate the DT range parsing for
all PCIe controllers in the kernel that are configured through DT.

This patch updates the PCIe designware driver in order for it to use the
new OF range parsing API and remove driver specific parsing code.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mohit Kumar <mohit.kumar@st.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++--------------------
 drivers/pci/host/pcie-designware.h |   5 +-
 2 files changed, 69 insertions(+), 81 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index df781cd..95cce40 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -69,8 +69,6 @@
 
 static struct hw_pci dw_pci;
 
-static unsigned long global_io_offset;
-
 static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 {
 	BUG_ON(!sys->private_data);
@@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 {
 	struct device_node *np = pp->dev->of_node;
 	struct platform_device *pdev = to_platform_device(pp->dev);
-	struct of_pci_range range;
-	struct of_pci_range_parser parser;
 	struct resource *cfg_res;
-	u32 val, na, ns;
+	u32 val, na;
 	const __be32 *addrp;
 	int i, index, ret;
+	resource_size_t io_base;
+	struct pci_host_bridge_window *win;
+	struct of_pci_resource *of_pci_res;
 
-	/* Find the address cell size and the number of cells in order to get
-	 * the untranslated address.
+	/*
+	 * Find the address cell size (ie must be 3 for DT PCI addresses) to
+	 * get the untranslated address.
 	 */
 	of_property_read_u32(np, "#address-cells", &na);
-	ns = of_n_size_cells(np);
+
+	INIT_LIST_HEAD(&pp->resources);
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
@@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
 		/* Find the untranslated configuration space address */
 		index = of_property_match_string(np, "reg-names", "config");
 		addrp = of_get_address(np, index, NULL, NULL);
-		pp->cfg0_mod_base = of_read_number(addrp, ns);
+		pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np));
 		pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size;
 	} else {
 		dev_err(pp->dev, "missing *config* reg space\n");
 	}
 
-	if (of_pci_range_parser_init(&parser, np)) {
-		dev_err(pp->dev, "missing ranges property\n");
-		return -EINVAL;
-	}
+	ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources,
+					       &io_base);
+	if (ret)
+		return ret;
 
-	/* Get the I/O and memory ranges from DT */
-	for_each_of_pci_range(&parser, &range) {
-		unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
-
-		if (restype == IORESOURCE_IO) {
-			of_pci_range_to_resource(&range, np, &pp->io);
-			pp->io.name = "I/O";
-			pp->io.start = max_t(resource_size_t,
-					     PCIBIOS_MIN_IO,
-					     range.pci_addr + global_io_offset);
-			pp->io.end = min_t(resource_size_t,
+	list_for_each_entry(win, &pp->resources, list) {
+		u64 pci_addr;
+		struct resource *res = win->res;
+
+		pci_addr = res->start - win->offset;
+		of_pci_res = container_of(res, struct of_pci_resource, res);
+
+		if (resource_type(res) == IORESOURCE_IO) {
+			res->name = "I/O";
+			res->start = max_t(resource_size_t, PCIBIOS_MIN_IO,
+					   pci_addr);
+			res->end = min_t(resource_size_t,
 					   IO_SPACE_LIMIT,
-					   range.pci_addr + range.size
-					   + global_io_offset - 1);
-			pp->io_size = resource_size(&pp->io);
-			pp->io_bus_addr = range.pci_addr;
-			pp->io_base = range.cpu_addr;
+					   pci_addr + resource_size(res) - 1);
+			pp->io_size = resource_size(res);
+			pp->io_bus_addr = pci_addr;
+			pp->io_base = io_base;
 
 			/* Find the untranslated IO space address */
-			pp->io_mod_base = of_read_number(parser.range -
-							 parser.np + na, ns);
+			pp->io_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
+			pci_remap_iospace(res, io_base);
 		}
-		if (restype == IORESOURCE_MEM) {
-			of_pci_range_to_resource(&range, np, &pp->mem);
-			pp->mem.name = "MEM";
-			pp->mem_size = resource_size(&pp->mem);
-			pp->mem_bus_addr = range.pci_addr;
+
+		if (resource_type(res) == IORESOURCE_MEM) {
+			res->name = "MEM";
+			pp->mem_size = resource_size(res);
+			pp->mem_bus_addr = pci_addr;
+			pp->mem_base = res->start;
 
 			/* Find the untranslated MEM space address */
-			pp->mem_mod_base = of_read_number(parser.range -
-							  parser.np + na, ns);
+			pp->mem_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 		}
-		if (restype == 0) {
-			of_pci_range_to_resource(&range, np, &pp->cfg);
-			pp->cfg0_size = resource_size(&pp->cfg)/2;
-			pp->cfg1_size = resource_size(&pp->cfg)/2;
-			pp->cfg0_base = pp->cfg.start;
-			pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
+
+		if (resource_type(res) == 0) {
+			pp->cfg0_size = resource_size(res)/2;
+			pp->cfg1_size = resource_size(res)/2;
+			pp->cfg0_base = res->start;
+			pp->cfg1_base = res->start + pp->cfg0_size;
 
 			/* Find the untranslated configuration space address */
-			pp->cfg0_mod_base = of_read_number(parser.range -
-							   parser.np + na, ns);
+			pp->cfg0_mod_base =
+				of_read_number(of_pci_res->parser.range + na,
+					       of_pci_res->parser.pna);
 			pp->cfg1_mod_base = pp->cfg0_mod_base +
 					    pp->cfg0_size;
-		}
-	}
-
-	ret = of_pci_parse_bus_range(np, &pp->busn);
-	if (ret < 0) {
-		pp->busn.name = np->name;
-		pp->busn.start = 0;
-		pp->busn.end = 0xff;
-		pp->busn.flags = IORESOURCE_BUS;
-		dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n",
-			ret, &pp->busn);
-	}
 
-	if (!pp->dbi_base) {
-		pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
-					resource_size(&pp->cfg));
-		if (!pp->dbi_base) {
-			dev_err(pp->dev, "error with ioremap\n");
-			return -ENOMEM;
+			if (!pp->dbi_base) {
+				pp->dbi_base = devm_ioremap(pp->dev,
+						res->start,
+						resource_size(res));
+				if (!pp->dbi_base) {
+					dev_err(pp->dev, "error with ioremap\n");
+					return -ENOMEM;
+				}
+			}
 		}
-	}
 
-	pp->mem_base = pp->mem.start;
+	}
 
 	if (!pp->va_cfg0_base) {
 		pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
@@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = {
 static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct pcie_port *pp;
+	struct pci_host_bridge_window *win, *tmp;
 
 	pp = sys_to_pcie(sys);
+	/* remove the configuration aperture in case it is there */
+	list_for_each_entry_safe(win, tmp, &pp->resources, list)
+		if (!resource_type(win->res))
+			list_del(&win->list);
 
-	if (global_io_offset < SZ_1M && pp->io_size > 0) {
-		sys->io_offset = global_io_offset - pp->io_bus_addr;
-		pci_ioremap_io(global_io_offset, pp->io_base);
-		global_io_offset += SZ_64K;
-		pci_add_resource_offset(&sys->resources, &pp->io,
-					sys->io_offset);
-	}
-
-	sys->mem_offset = pp->mem.start - pp->mem_bus_addr;
-	pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
-	pci_add_resource(&sys->resources, &pp->busn);
+	list_splice(&pp->resources, &sys->resources);
 
 	return 1;
 }
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..2479e9c 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -42,10 +42,7 @@ struct pcie_port {
 	u64			mem_mod_base;
 	phys_addr_t		mem_bus_addr;
 	u32			mem_size;
-	struct resource		cfg;
-	struct resource		io;
-	struct resource		mem;
-	struct resource		busn;
+	struct list_head	resources;
 	int			irq;
 	u32			lanes;
 	struct pcie_host_ops	*ops;
-- 
2.2.1

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Rob Herring]

On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi
<lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> 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.

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.

> The first patch in the series is a fix/update for the parsing function:
>
> of_pci_get_host_bridge_resources()

This one stands on its own and should go to stable perhap? If so, send
it separately.

Rob

>
> 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 <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> Cc: Mohit Kumar <mohit.kumar-qxv4g6HH51o@public.gmane.org>
> Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Rob Herring]

On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> 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.

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.

> The first patch in the series is a fix/update for the parsing function:
>
> of_pci_get_host_bridge_resources()

This one stands on its own and should go to stable perhap? If so, send
it separately.

Rob

>
> 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 <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
>
> 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
>

[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Rob Herring]

On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> 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.

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.

> The first patch in the series is a fix/update for the parsing function:
>
> of_pci_get_host_bridge_resources()

This one stands on its own and should go to stable perhap? If so, send
it separately.

Rob

>
> 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 <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
>
> 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
>

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Arnd Bergmann]

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.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Arnd Bergmann]

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.

	Arnd

[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Arnd Bergmann]

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.

	Arnd

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Wed, Jan 07, 2015 at 03:29:29PM +0000, Lorenzo Pieralisi wrote:
> 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 <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  drivers/of/of_pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 88471d3..6fbfe99 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -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;
> @@ -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,

> -- 
> 2.2.1
> 
> 

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

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Wed, Jan 07, 2015 at 03:29:29PM +0000, Lorenzo Pieralisi wrote:
> 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 <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  drivers/of/of_pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 88471d3..6fbfe99 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -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;
> @@ -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,

> -- 
> 2.2.1
> 
> 

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

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Lorenzo Pieralisi]

On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote:
> 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().

I like the idea too, it should simplify the API implementation.

> 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.

Yes, CPU untranslated addresses are a pain in the back. I wrote
the series at it is to avoid changing the API, but I agree that's
a bit convoluted, which means we should refactor it.

I think the API should always return a pair of CPU-PCI resources as Rob
said, and provide an "on-demand" request for untranslated addresses, since
their usage is not that common (at the moment). I need to think about
that, we might even return a triplet, I hate doing that since I fear
it might be a one-off need for PCIe designware.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Lorenzo Pieralisi]

On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote:
> 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().

I like the idea too, it should simplify the API implementation.

> 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.

Yes, CPU untranslated addresses are a pain in the back. I wrote
the series at it is to avoid changing the API, but I agree that's
a bit convoluted, which means we should refactor it.

I think the API should always return a pair of CPU-PCI resources as Rob
said, and provide an "on-demand" request for untranslated addresses, since
their usage is not that common (at the moment). I need to think about
that, we might even return a triplet, I hate doing that since I fear
it might be a one-off need for PCIe designware.

Lorenzo

[RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API

[Lorenzo Pieralisi]

On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote:
> 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().

I like the idea too, it should simplify the API implementation.

> 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.

Yes, CPU untranslated addresses are a pain in the back. I wrote
the series at it is to avoid changing the API, but I agree that's
a bit convoluted, which means we should refactor it.

I think the API should always return a pair of CPU-PCI resources as Rob
said, and provide an "on-demand" request for untranslated addresses, since
their usage is not that common (at the moment). I need to think about
that, we might even return a triplet, I hate doing that since I fear
it might be a one-off need for PCIe designware.

Lorenzo

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

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

[...]

> > @@ -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;
> > @@ -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.

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

> 
> 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,
> 
> > -- 
> > 2.2.1
> > 
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

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

[...]

> > @@ -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;
> > @@ -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.

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

> 
> 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,
> 
> > -- 
> > 2.2.1
> > 
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

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

[...]

> > @@ -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;
> > @@ -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.

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

> 
> 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,
> 
> > -- 
> > 2.2.1
> > 
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ?\_(?)_/?

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> 
> [...]
> 
> > > @@ -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;
> > > @@ -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.
> 
> 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.

Thanks,
Liviu

> 
> 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
> 
> > 
> > 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,
> > 
> > > -- 
> > > 2.2.1
> > > 
> > > 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> 
> [...]
> 
> > > @@ -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;
> > > @@ -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.
> 
> 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.

Thanks,
Liviu

> 
> 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
> 
> > 
> > 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,
> > 
> > > -- 
> > > 2.2.1
> > > 
> > > 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> 
> [...]
> 
> > > @@ -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;
> > > @@ -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.
> 
> 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.

Thanks,
Liviu

> 
> 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
> 
> > 
> > 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,
> > 
> > > -- 
> > > 2.2.1
> > > 
> > > 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > 
> > [...]
> > 
> > > > @@ -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;
> > > > @@ -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.
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > 
> > [...]
> > 
> > > > @@ -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;
> > > > @@ -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.
> > 
> > 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

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Lorenzo Pieralisi]

On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > 
> > [...]
> > 
> > > > @@ -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;
> > > > @@ -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.
> > 
> > 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

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > > 
> > > [...]
> > > 
> > > > > @@ -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;
> > > > > @@ -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.
> > > 
> > > 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.

Sure. 

Acked-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>

> 
> Thanks,
> Lorenzo
> 

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > > 
> > > [...]
> > > 
> > > > > @@ -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;
> > > > > @@ -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.
> > > 
> > > 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.

Sure. 

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

> 
> Thanks,
> Lorenzo
> 

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

[RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources()

[Liviu Dudau]

On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote:
> > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote:
> > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote:
> > > 
> > > [...]
> > > 
> > > > > @@ -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;
> > > > > @@ -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.
> > > 
> > > 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.

Sure. 

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

> 
> Thanks,
> Lorenzo
> 

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