Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH 1/2] PCI: iproc: directly add pci resources
@ 2015-05-24 20:37 Hauke Mehrtens
  2015-05-24 20:37 ` [PATCH 2/2] PCI: iproc: free resource list after registration Hauke Mehrtens
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Hauke Mehrtens @ 2015-05-24 20:37 UTC (permalink / raw)
  To: rjui, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5,
	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 <hauke@hauke-m.de>
---
 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(-)

diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
index c318f19..f96b39e 100644
--- a/drivers/pci/host/pcie-iproc-bcma.c
+++ b/drivers/pci/host/pcie-iproc-bcma.c
@@ -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;
diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index c8aa06f..c5fe4c1 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -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;
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index cef31f6..d77481e 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -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;
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index a333d4b..ba0a108 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -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 */
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] PCI: iproc: free resource list after registration
  2015-05-24 20:37 [PATCH 1/2] PCI: iproc: directly add pci resources Hauke Mehrtens
@ 2015-05-24 20:37 ` Hauke Mehrtens
  2015-05-26  0:35   ` Ray Jui
  2015-05-25 17:10 ` [PATCH 1/2] PCI: iproc: directly add pci resources Ray Jui
  2015-05-27 23:35 ` Bjorn Helgaas
  2 siblings, 1 reply; 7+ messages in thread
From: Hauke Mehrtens @ 2015-05-24 20:37 UTC (permalink / raw)
  To: rjui, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5,
	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 <hauke@hauke-m.de>
---
 drivers/pci/host/pcie-iproc-bcma.c     | 8 ++++----
 drivers/pci/host/pcie-iproc-platform.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
index f96b39e..2e4f2d6 100644
--- a/drivers/pci/host/pcie-iproc-bcma.c
+++ b/drivers/pci/host/pcie-iproc-bcma.c
@@ -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)
diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index c5fe4c1..9aedc8e 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -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)
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] PCI: iproc: directly add pci resources
  2015-05-24 20:37 [PATCH 1/2] PCI: iproc: directly add pci resources Hauke Mehrtens
  2015-05-24 20:37 ` [PATCH 2/2] PCI: iproc: free resource list after registration Hauke Mehrtens
@ 2015-05-25 17:10 ` Ray Jui
  2015-05-25 22:48   ` Hauke Mehrtens
  2015-05-27 23:35 ` Bjorn Helgaas
  2 siblings, 1 reply; 7+ messages in thread
From: Ray Jui @ 2015-05-25 17:10 UTC (permalink / raw)
  To: Hauke Mehrtens, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5

Hi Hauke,

On 5/24/2015 1:37 PM, Hauke Mehrtens wrote:
> 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 <hauke@hauke-m.de>
> ---
>  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(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
> index c318f19..f96b39e 100644
> --- a/drivers/pci/host/pcie-iproc-bcma.c
> +++ b/drivers/pci/host/pcie-iproc-bcma.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index c8aa06f..c5fe4c1 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index cef31f6..d77481e 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a333d4b..ba0a108 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -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

>  	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 */
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] PCI: iproc: directly add pci resources
  2015-05-25 17:10 ` [PATCH 1/2] PCI: iproc: directly add pci resources Ray Jui
@ 2015-05-25 22:48   ` Hauke Mehrtens
  2015-05-26  0:32     ` Ray Jui
  0 siblings, 1 reply; 7+ messages in thread
From: Hauke Mehrtens @ 2015-05-25 22:48 UTC (permalink / raw)
  To: Ray Jui, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5

On 05/25/2015 07:10 PM, Ray Jui wrote:
> Hi Hauke,
> 
> On 5/24/2015 1:37 PM, Hauke Mehrtens wrote:
>> 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 <hauke@hauke-m.de>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
>> index c318f19..f96b39e 100644
>> --- a/drivers/pci/host/pcie-iproc-bcma.c
>> +++ b/drivers/pci/host/pcie-iproc-bcma.c
>> @@ -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;
>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>> index c8aa06f..c5fe4c1 100644
>> --- a/drivers/pci/host/pcie-iproc-platform.c
>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>> @@ -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;
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index cef31f6..d77481e 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -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;
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> index a333d4b..ba0a108 100644
>> --- a/drivers/pci/host/pcie-iproc.h
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -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?

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] PCI: iproc: directly add pci resources
  2015-05-25 22:48   ` Hauke Mehrtens
@ 2015-05-26  0:32     ` Ray Jui
  0 siblings, 0 replies; 7+ messages in thread
From: Ray Jui @ 2015-05-26  0:32 UTC (permalink / raw)
  To: Hauke Mehrtens, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5

Hi Hauke,

On 15-05-25 03:48 PM, Hauke Mehrtens wrote:
> On 05/25/2015 07:10 PM, Ray Jui wrote:
>> Hi Hauke,
>>
>> On 5/24/2015 1:37 PM, Hauke Mehrtens wrote:
>>> 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 <hauke@hauke-m.de>
>>> ---
>>>   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(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
>>> index c318f19..f96b39e 100644
>>> --- a/drivers/pci/host/pcie-iproc-bcma.c
>>> +++ b/drivers/pci/host/pcie-iproc-bcma.c
>>> @@ -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;
>>> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
>>> index c8aa06f..c5fe4c1 100644
>>> --- a/drivers/pci/host/pcie-iproc-platform.c
>>> +++ b/drivers/pci/host/pcie-iproc-platform.c
>>> @@ -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;
>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>> index cef31f6..d77481e 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -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;
>>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>>> index a333d4b..ba0a108 100644
>>> --- a/drivers/pci/host/pcie-iproc.h
>>> +++ b/drivers/pci/host/pcie-iproc.h
>>> @@ -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?
>
> 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 <rjui@broadcom.com>
Tested-by: Ray Jui <rjui@broadcom.com>

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] PCI: iproc: free resource list after registration
  2015-05-24 20:37 ` [PATCH 2/2] PCI: iproc: free resource list after registration Hauke Mehrtens
@ 2015-05-26  0:35   ` Ray Jui
  0 siblings, 0 replies; 7+ messages in thread
From: Ray Jui @ 2015-05-26  0:35 UTC (permalink / raw)
  To: Hauke Mehrtens, bhelgaas
  Cc: linux-pci, linux-arm-kernel, bcm-kernel-feedback-list, zajec5

Hi Hauke,

On 15-05-24 01:37 PM, Hauke Mehrtens wrote:
> 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 <hauke@hauke-m.de>
> ---
>   drivers/pci/host/pcie-iproc-bcma.c     | 8 ++++----
>   drivers/pci/host/pcie-iproc-platform.c | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
> index f96b39e..2e4f2d6 100644
> --- a/drivers/pci/host/pcie-iproc-bcma.c
> +++ b/drivers/pci/host/pcie-iproc-bcma.c
> @@ -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)
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index c5fe4c1..9aedc8e 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -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)
>

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

Thanks,

Reviewed-by: Ray Jui <rjui@broadcom.com>
Tested-by: Ray Jui <rjui@broadcom.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] PCI: iproc: directly add pci resources
  2015-05-24 20:37 [PATCH 1/2] PCI: iproc: directly add pci resources Hauke Mehrtens
  2015-05-24 20:37 ` [PATCH 2/2] PCI: iproc: free resource list after registration Hauke Mehrtens
  2015-05-25 17:10 ` [PATCH 1/2] PCI: iproc: directly add pci resources Ray Jui
@ 2015-05-27 23:35 ` Bjorn Helgaas
  2 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2015-05-27 23:35 UTC (permalink / raw)
  To: Hauke Mehrtens
  Cc: rjui, linux-pci, linux-arm-kernel, bcm-kernel-feedback-list,
	zajec5

On Sun, May 24, 2015 at 10:37:02PM +0200, Hauke Mehrtens wrote:
> 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 <hauke@hauke-m.de>

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

> ---
>  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(-)
> 
> diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
> index c318f19..f96b39e 100644
> --- a/drivers/pci/host/pcie-iproc-bcma.c
> +++ b/drivers/pci/host/pcie-iproc-bcma.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
> index c8aa06f..c5fe4c1 100644
> --- a/drivers/pci/host/pcie-iproc-platform.c
> +++ b/drivers/pci/host/pcie-iproc-platform.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index cef31f6..d77481e 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -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;
> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
> index a333d4b..ba0a108 100644
> --- a/drivers/pci/host/pcie-iproc.h
> +++ b/drivers/pci/host/pcie-iproc.h
> @@ -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 */
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-05-27 23:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-24 20:37 [PATCH 1/2] PCI: iproc: directly add pci resources Hauke Mehrtens
2015-05-24 20:37 ` [PATCH 2/2] PCI: iproc: free resource list after registration Hauke Mehrtens
2015-05-26  0:35   ` Ray Jui
2015-05-25 17:10 ` [PATCH 1/2] PCI: iproc: directly add pci resources Ray Jui
2015-05-25 22:48   ` Hauke Mehrtens
2015-05-26  0:32     ` Ray Jui
2015-05-27 23:35 ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox