[PATCH RFC] bcma: simplify freeing cores (internal devices structs)

[PATCH RFC] bcma: simplify freeing cores (internal devices structs)

[Rafał Miłecki]

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
This code was introduced by Saul in
ee91592711ed90a1abfbb1b2ceadded11d685164
bcma: don't leak memory for PCIE, MIPS, GBIT cores

I don't really see reason for making it in so complicated way.
I tested my patch for crashes, but didn't really try kmemleak.
Is my simple solution OK? Or am I missing something? Anyone?
---
 drivers/bcma/main.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
index c3c5e0a..58dd582 100644
--- a/drivers/bcma/main.c
+++ b/drivers/bcma/main.c
@@ -371,6 +371,8 @@ static void bcma_unregister_cores(struct bcma_bus *bus)
 		list_del(&core->list);
 		if (core->dev_registered)
 			device_unregister(&core->dev);
+		else
+			kfree(core);
 	}
 	if (bus->hosttype == BCMA_HOSTTYPE_SOC)
 		platform_device_unregister(bus->drv_cc.watchdog);
@@ -467,7 +469,6 @@ int bcma_bus_register(struct bcma_bus *bus)
 
 void bcma_bus_unregister(struct bcma_bus *bus)
 {
-	struct bcma_device *cores[3];
 	int err;
 
 	err = bcma_gpio_unregister(&bus->drv_cc);
@@ -478,15 +479,7 @@ void bcma_bus_unregister(struct bcma_bus *bus)
 
 	bcma_core_chipcommon_b_free(&bus->drv_cc_b);
 
-	cores[0] = bcma_find_core(bus, BCMA_CORE_MIPS_74K);
-	cores[1] = bcma_find_core(bus, BCMA_CORE_PCIE);
-	cores[2] = bcma_find_core(bus, BCMA_CORE_4706_MAC_GBIT_COMMON);
-
 	bcma_unregister_cores(bus);
-
-	kfree(cores[2]);
-	kfree(cores[1]);
-	kfree(cores[0]);
 }
 
 /*
-- 
1.8.4.5

Re: [PATCH RFC] bcma: simplify freeing cores (internal devices structs)

[Hauke Mehrtens]

On 01/16/2015 10:47 PM, Rafał Miłecki wrote:
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> This code was introduced by Saul in
> ee91592711ed90a1abfbb1b2ceadded11d685164
> bcma: don't leak memory for PCIE, MIPS, GBIT cores
> 
> I don't really see reason for making it in so complicated way.
> I tested my patch for crashes, but didn't really try kmemleak.
> Is my simple solution OK? Or am I missing something? Anyone?

Are you sure no device driver accesses the chipcommon, pcie or mips core
in the unregister part? If some device driver does something in his
remove function with e.g. chipcommon it will cause problems when the
chipcommon core was already freed.

Hauke

> ---
>  drivers/bcma/main.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
> index c3c5e0a..58dd582 100644
> --- a/drivers/bcma/main.c
> +++ b/drivers/bcma/main.c
> @@ -371,6 +371,8 @@ static void bcma_unregister_cores(struct bcma_bus *bus)
>  		list_del(&core->list);
>  		if (core->dev_registered)
>  			device_unregister(&core->dev);
> +		else
> +			kfree(core);
>  	}
>  	if (bus->hosttype == BCMA_HOSTTYPE_SOC)
>  		platform_device_unregister(bus->drv_cc.watchdog);
> @@ -467,7 +469,6 @@ int bcma_bus_register(struct bcma_bus *bus)
>  
>  void bcma_bus_unregister(struct bcma_bus *bus)
>  {
> -	struct bcma_device *cores[3];
>  	int err;
>  
>  	err = bcma_gpio_unregister(&bus->drv_cc);
> @@ -478,15 +479,7 @@ void bcma_bus_unregister(struct bcma_bus *bus)
>  
>  	bcma_core_chipcommon_b_free(&bus->drv_cc_b);
>  
> -	cores[0] = bcma_find_core(bus, BCMA_CORE_MIPS_74K);
> -	cores[1] = bcma_find_core(bus, BCMA_CORE_PCIE);
> -	cores[2] = bcma_find_core(bus, BCMA_CORE_4706_MAC_GBIT_COMMON);
> -
>  	bcma_unregister_cores(bus);
> -
> -	kfree(cores[2]);
> -	kfree(cores[1]);
> -	kfree(cores[0]);
>  }
>  
>  /*
> 

Re: [PATCH RFC] bcma: simplify freeing cores (internal devices structs)

[Rafał Miłecki]

On 18 January 2015 at 19:26, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 01/16/2015 10:47 PM, Rafał Miłecki wrote:
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> This code was introduced by Saul in
>> ee91592711ed90a1abfbb1b2ceadded11d685164
>> bcma: don't leak memory for PCIE, MIPS, GBIT cores
>>
>> I don't really see reason for making it in so complicated way.
>> I tested my patch for crashes, but didn't really try kmemleak.
>> Is my simple solution OK? Or am I missing something? Anyone?
>
> Are you sure no device driver accesses the chipcommon, pcie or mips core
> in the unregister part? If some device driver does something in his
> remove function with e.g. chipcommon it will cause problems when the
> chipcommon core was already freed.

It didn't crash anything for me, but your point is correct. I'll
change the logic to first unregister and then free cores.

-- 
Rafał