Skip to contentSkip to conversation

PCI: iproc: directly add pci resources

PATCH New Hauke Mehrtens 3 participants 7 messages May 24, 2015 – May 27, 2015

Subsystems (2)
PCI Broadcom iproc Arm architecture
lore
Share & tools
lore

Hauke Mehrtens

author
#0 lore raw

PATCH 1/2 PCI: iproc: directly add pci resources

The resources member in the struct was pointing to a stack variable and is invalid after the the registration function returned. Remove this pointer and add it a a parameter to the function.

Signed-off-by Hauke Mehrtens <[email protected]>
---
 drivers/pci/host/pcie-iproc-bcma.c     | 4 +---
 drivers/pci/host/pcie-iproc-platform.c | 4 +---
 drivers/pci/host/pcie-iproc.c          | 4 ++--
 drivers/pci/host/pcie-iproc.h          | 3 +--
 4 files changed, 5 insertions(+), 10 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +1 -3
index c318f19..f96b39e 100644
@@ -62,11 +62,9 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 	res_mem.flags = IORESOURCE_MEM;
 	pci_add_resource(&res, &res_mem);
 
-	pcie->resources = &res;
-
 	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc-platform.c +1 -3
index c8aa06f..c5fe4c1 100644
@@ -69,11 +69,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pcie->resources = &res;
-
 	pcie->map_irq = of_irq_parse_and_map_pci;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc.c +2 -2
index cef31f6..d77481e 100644
@@ -183,7 +183,7 @@ static void iproc_pcie_enable(struct iproc_pcie *pcie)
 	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
 }
 
-int iproc_pcie_setup(struct iproc_pcie *pcie)
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	int ret;
 	struct pci_bus *bus;

@@ -211,7 +211,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
 	pcie->sysdata.private_data = pcie;
 
 	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, pcie->resources);
+				  &pcie->sysdata, res);
 	if (!bus) {
 		dev_err(pcie->dev, "unable to create PCI root bus\n");
 		ret = -ENOMEM;
drivers/pci/host/pcie-iproc.h +1 -2
index a333d4b..ba0a108 100644
@@ -29,7 +29,6 @@
 struct iproc_pcie {
 	struct device *dev;
 	void __iomem *base;
-	struct list_head *resources;
 	struct pci_sys_data sysdata;
 	struct pci_bus *root_bus;
 	struct phy *phy;

@@ -37,7 +36,7 @@ struct iproc_pcie {
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 };
 
-int iproc_pcie_setup(struct iproc_pcie *pcie);
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
 
 #endif /* _PCIE_IPROC_H */
Signature · 1 line
2.1.4
author
#1 lore raw

PATCH 2/2 PCI: iproc: free resource list after registration

The resource list is only used in the setup process and was never freed. pci_add_resource() allocates a memory area to store the list item. This patch fixes the memory leak.

Signed-off-by Hauke Mehrtens <[email protected]>
---
 drivers/pci/host/pcie-iproc-bcma.c     | 8 ++++----
 drivers/pci/host/pcie-iproc-platform.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +4 -4
index f96b39e..2e4f2d6 100644
@@ -65,12 +65,12 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
 	ret = iproc_pcie_setup(pcie, &res);
-	if (ret) {
+	if (ret)
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
-		return ret;
-	}
 
-	return 0;
+	pci_free_resource_list(&res);
+
+	return ret;
 }
 
 static void iproc_pcie_bcma_remove(struct bcma_device *bdev)
drivers/pci/host/pcie-iproc-platform.c +4 -4
index c5fe4c1..9aedc8e 100644
@@ -72,12 +72,12 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 	pcie->map_irq = of_irq_parse_and_map_pci;
 
 	ret = iproc_pcie_setup(pcie, &res);
-	if (ret) {
+	if (ret)
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
-		return ret;
-	}
 
-	return 0;
+	pci_free_resource_list(&res);
+
+	return ret;
 }
 
 static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
Signature · 1 line
2.1.4
maintainer
#2 lore raw

PATCH 2/2 PCI: iproc: free resource list after registration

Hi Hauke,

Quoted 43 lines · Hauke Mehrtens

The resource list is only used in the setup process and was never freed. pci_add_resource() allocates a memory area to store the list item. This patch fixes the memory leak.

Signed-off-by Hauke Mehrtens <[email protected]>
---
  drivers/pci/host/pcie-iproc-bcma.c     | 8 ++++----
  drivers/pci/host/pcie-iproc-platform.c | 8 ++++----
  2 files changed, 8 insertions(+), 8 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +4 -4
index f96b39e..2e4f2d6 100644
@@ -65,12 +65,12 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
  	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
  	ret = iproc_pcie_setup(pcie, &res);
-	if (ret) {
+	if (ret)
  		dev_err(pcie->dev, "PCIe controller setup failed\n");
-		return ret;
-	}
 
-	return 0;
+	pci_free_resource_list(&res);
+
+	return ret;
  }
 
  static void iproc_pcie_bcma_remove(struct bcma_device *bdev)
drivers/pci/host/pcie-iproc-platform.c +1 -1
index c5fe4c1..9aedc8e 100644
@@ -72,12 +72,12 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
  	pcie->map_irq = of_irq_parse_and_map_pci;
 
  	ret = iproc_pcie_setup(pcie, &res);
-	if (ret) {
+	if (ret)
  		dev_err(pcie->dev, "PCIe controller setup failed\n");

drivers/pci/host/pcie-iproc-platform.c static int iproc_pcie_pltfm_probe(struct platform_device *pdev) · line 83

Quoted 10 lines
drivers/pci/host/pcie-iproc-platform.c +3 -3
-		return ret;
-	}
 
-	return 0;
+	pci_free_resource_list(&res);
+
+	return ret;
  }
 
  static int iproc_pcie_pltfm_remove(struct platform_device *pdev)

This looks good to me and I sanity tested it on a BCM958300K Cygnus combo board.

Thanks,

Reviewed-by Ray Jui <[email protected]>
Tested-by Ray Jui <[email protected]>
maintainer
#3 lore raw

PATCH 1/2 PCI: iproc: directly add pci resources

In reply to Hauke Mehrtens

Hi Hauke,

Quoted 73 lines · Hauke Mehrtens

The resources member in the struct was pointing to a stack variable and is invalid after the the registration function returned. Remove this pointer and add it a a parameter to the function.

Signed-off-by Hauke Mehrtens <[email protected]>
---
 drivers/pci/host/pcie-iproc-bcma.c     | 4 +---
 drivers/pci/host/pcie-iproc-platform.c | 4 +---
 drivers/pci/host/pcie-iproc.c          | 4 ++--
 drivers/pci/host/pcie-iproc.h          | 3 +--
 4 files changed, 5 insertions(+), 10 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +1 -3
index c318f19..f96b39e 100644
@@ -62,11 +62,9 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 	res_mem.flags = IORESOURCE_MEM;
 	pci_add_resource(&res, &res_mem);
 
-	pcie->resources = &res;
-
 	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc-platform.c +1 -3
index c8aa06f..c5fe4c1 100644
@@ -69,11 +69,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pcie->resources = &res;
-
 	pcie->map_irq = of_irq_parse_and_map_pci;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc.c +2 -2
index cef31f6..d77481e 100644
@@ -183,7 +183,7 @@ static void iproc_pcie_enable(struct iproc_pcie *pcie)
 	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
 }
 
-int iproc_pcie_setup(struct iproc_pcie *pcie)
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	int ret;
 	struct pci_bus *bus;

@@ -211,7 +211,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
 	pcie->sysdata.private_data = pcie;
 
 	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, pcie->resources);
+				  &pcie->sysdata, res);
 	if (!bus) {
 		dev_err(pcie->dev, "unable to create PCI root bus\n");
 		ret = -ENOMEM;
drivers/pci/host/pcie-iproc.h +0 -0
index a333d4b..ba0a108 100644

drivers/pci/host/pcie-iproc.h line 32

Quoted 4 lines
drivers/pci/host/pcie-iproc.h +0 -1
@@ -29,7 +29,6 @@
 struct iproc_pcie {
 	struct device *dev;
 	void __iomem *base;
-	struct list_head *resources;

This means we do not want to keep a copy of the resources. In the future, if we need to add support to explicitly set up the inbound/outbound mapping window, we need to do it in iproc_pcie_setup. Is that the intention?

Thanks,

Ray

Quoted 13 lines
drivers/pci/host/pcie-iproc.h +1 -1
 	struct pci_sys_data sysdata;
 	struct pci_bus *root_bus;
 	struct phy *phy;

@@ -37,7 +36,7 @@ struct iproc_pcie {
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 };
 
-int iproc_pcie_setup(struct iproc_pcie *pcie);
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
 
 #endif /* _PCIE_IPROC_H */
author
#4 lore raw

PATCH 1/2 PCI: iproc: directly add pci resources

Quoted 77 lines · Ray Jui

Hi Hauke,

Quoted 74 lines · Hauke Mehrtens

The resources member in the struct was pointing to a stack variable and is invalid after the the registration function returned. Remove this pointer and add it a a parameter to the function.

Signed-off-by Hauke Mehrtens <[email protected]>
---
 drivers/pci/host/pcie-iproc-bcma.c     | 4 +---
 drivers/pci/host/pcie-iproc-platform.c | 4 +---
 drivers/pci/host/pcie-iproc.c          | 4 ++--
 drivers/pci/host/pcie-iproc.h          | 3 +--
 4 files changed, 5 insertions(+), 10 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +1 -3
index c318f19..f96b39e 100644
@@ -62,11 +62,9 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 	res_mem.flags = IORESOURCE_MEM;
 	pci_add_resource(&res, &res_mem);
 
-	pcie->resources = &res;
-
 	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc-platform.c +1 -3
index c8aa06f..c5fe4c1 100644
@@ -69,11 +69,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pcie->resources = &res;
-
 	pcie->map_irq = of_irq_parse_and_map_pci;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc.c +2 -2
index cef31f6..d77481e 100644
@@ -183,7 +183,7 @@ static void iproc_pcie_enable(struct iproc_pcie *pcie)
 	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
 }
 
-int iproc_pcie_setup(struct iproc_pcie *pcie)
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	int ret;
 	struct pci_bus *bus;

@@ -211,7 +211,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
 	pcie->sysdata.private_data = pcie;
 
 	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, pcie->resources);
+				  &pcie->sysdata, res);
 	if (!bus) {
 		dev_err(pcie->dev, "unable to create PCI root bus\n");
 		ret = -ENOMEM;
drivers/pci/host/pcie-iproc.h +0 -0
index a333d4b..ba0a108 100644
@@ -29,7 +29,6 @@
 struct iproc_pcie {
Quoted 8 lines

drivers/pci/host/pcie-iproc.h line 32

Quoted 3 lines
drivers/pci/host/pcie-iproc.h +0 -1
 	struct device *dev;
 	void __iomem *base;
-	struct list_head *resources;

This means we do not want to keep a copy of the resources. In the future, if we need to add support to explicitly set up the inbound/outbound mapping window, we need to do it in iproc_pcie_setup. Is that the intention?

I haven't really thought about where to configure the memory mapping, but I thought it would be somewhere in iproc_pcie_setup() or an method called from there.
If you need it after iproc_pcie_setup() finished we should embed the list into the struct iproc_pcie, because currently this pointer points to the stack.

Hauke

maintainer
#5 lore raw

PATCH 1/2 PCI: iproc: directly add pci resources

Hi Hauke,

Quoted 87 lines · Hauke Mehrtens
Quoted 77 lines · Ray Jui

Hi Hauke,

Quoted 74 lines · Hauke Mehrtens

The resources member in the struct was pointing to a stack variable and is invalid after the the registration function returned. Remove this pointer and add it a a parameter to the function.

Signed-off-by Hauke Mehrtens <[email protected]>
---
  drivers/pci/host/pcie-iproc-bcma.c     | 4 +---
  drivers/pci/host/pcie-iproc-platform.c | 4 +---
  drivers/pci/host/pcie-iproc.c          | 4 ++--
  drivers/pci/host/pcie-iproc.h          | 3 +--
  4 files changed, 5 insertions(+), 10 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +1 -3
index c318f19..f96b39e 100644
@@ -62,11 +62,9 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
  	res_mem.flags = IORESOURCE_MEM;
  	pci_add_resource(&res, &res_mem);
 
-	pcie->resources = &res;
-
  	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
  	if (ret) {
  		dev_err(pcie->dev, "PCIe controller setup failed\n");
  		return ret;
drivers/pci/host/pcie-iproc-platform.c +1 -3
index c8aa06f..c5fe4c1 100644
@@ -69,11 +69,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
  		return ret;
  	}
 
-	pcie->resources = &res;
-
  	pcie->map_irq = of_irq_parse_and_map_pci;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
  	if (ret) {
  		dev_err(pcie->dev, "PCIe controller setup failed\n");
  		return ret;
drivers/pci/host/pcie-iproc.c +2 -2
index cef31f6..d77481e 100644
@@ -183,7 +183,7 @@ static void iproc_pcie_enable(struct iproc_pcie *pcie)
  	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
  }
 
-int iproc_pcie_setup(struct iproc_pcie *pcie)
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
  {
  	int ret;
  	struct pci_bus *bus;

@@ -211,7 +211,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
  	pcie->sysdata.private_data = pcie;
 
  	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, pcie->resources);
+				  &pcie->sysdata, res);
  	if (!bus) {
  		dev_err(pcie->dev, "unable to create PCI root bus\n");
  		ret = -ENOMEM;
drivers/pci/host/pcie-iproc.h +0 -0
index a333d4b..ba0a108 100644
@@ -29,7 +29,6 @@
  struct iproc_pcie {
Quoted 8 lines

drivers/pci/host/pcie-iproc.h line 32

Quoted 3 lines
drivers/pci/host/pcie-iproc.h +0 -1
  	struct device *dev;
  	void __iomem *base;
-	struct list_head *resources;

This means we do not want to keep a copy of the resources. In the future, if we need to add support to explicitly set up the inbound/outbound mapping window, we need to do it in iproc_pcie_setup. Is that the intention?

Quoted 9 lines

I haven't really thought about where to configure the memory mapping, but I thought it would be somewhere in iproc_pcie_setup() or an method called from there.
If you need it after iproc_pcie_setup() finished we should embed the list into the struct iproc_pcie, because currently this pointer points to the stack.

Hauke

I think most likely I'll be adding the memory mapping logic in iproc_pcie_setup(), so your current patch should be fine.

Reviewed-by Ray Jui <[email protected]>
Tested-by Ray Jui <[email protected]>

Thanks.

maintainer Applied
#6 lore raw

PATCH 1/2 PCI: iproc: directly add pci resources

In reply to Hauke Mehrtens
Quoted 5 lines · Hauke Mehrtens

The resources member in the struct was pointing to a stack variable and is invalid after the the registration function returned. Remove this pointer and add it a a parameter to the function.

Signed-off-by Hauke Mehrtens <[email protected]>

Applied both patches with Ray's Reviewed-by to pci/host-iproc for v4.2, thanks!

Quoted 87 lines
---
 drivers/pci/host/pcie-iproc-bcma.c     | 4 +---
 drivers/pci/host/pcie-iproc-platform.c | 4 +---
 drivers/pci/host/pcie-iproc.c          | 4 ++--
 drivers/pci/host/pcie-iproc.h          | 3 +--
 4 files changed, 5 insertions(+), 10 deletions(-)
drivers/pci/host/pcie-iproc-bcma.c +1 -3
index c318f19..f96b39e 100644
@@ -62,11 +62,9 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 	res_mem.flags = IORESOURCE_MEM;
 	pci_add_resource(&res, &res_mem);
 
-	pcie->resources = &res;
-
 	pcie->map_irq = iproc_pcie_bcma_map_irq;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc-platform.c +1 -3
index c8aa06f..c5fe4c1 100644
@@ -69,11 +69,9 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pcie->resources = &res;
-
 	pcie->map_irq = of_irq_parse_and_map_pci;
 
-	ret = iproc_pcie_setup(pcie);
+	ret = iproc_pcie_setup(pcie, &res);
 	if (ret) {
 		dev_err(pcie->dev, "PCIe controller setup failed\n");
 		return ret;
drivers/pci/host/pcie-iproc.c +2 -2
index cef31f6..d77481e 100644
@@ -183,7 +183,7 @@ static void iproc_pcie_enable(struct iproc_pcie *pcie)
 	writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
 }
 
-int iproc_pcie_setup(struct iproc_pcie *pcie)
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 {
 	int ret;
 	struct pci_bus *bus;

@@ -211,7 +211,7 @@ int iproc_pcie_setup(struct iproc_pcie *pcie)
 	pcie->sysdata.private_data = pcie;
 
 	bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
-				  &pcie->sysdata, pcie->resources);
+				  &pcie->sysdata, res);
 	if (!bus) {
 		dev_err(pcie->dev, "unable to create PCI root bus\n");
 		ret = -ENOMEM;
drivers/pci/host/pcie-iproc.h +1 -2
index a333d4b..ba0a108 100644
@@ -29,7 +29,6 @@
 struct iproc_pcie {
 	struct device *dev;
 	void __iomem *base;
-	struct list_head *resources;
 	struct pci_sys_data sysdata;
 	struct pci_bus *root_bus;
 	struct phy *phy;

@@ -37,7 +36,7 @@ struct iproc_pcie {
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 };
 
-int iproc_pcie_setup(struct iproc_pcie *pcie);
+int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
 int iproc_pcie_remove(struct iproc_pcie *pcie);
 
 #endif /* _PCIE_IPROC_H */
Signature · 1 line
2.1.4