2015-01-16 21:47:20

by Rafał Miłecki

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

Signed-off-by: Rafał Miłecki <[email protected]>
---
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

2015-01-18 21:46:42

by Rafał Miłecki

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

On 18 January 2015 at 19:26, Hauke Mehrtens <[email protected]> wrote:
> On 01/16/2015 10:47 PM, Rafał Miłecki wrote:
>> Signed-off-by: Rafał Miłecki <[email protected]>
>> ---
>> 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ł

2015-01-18 18:26:25

by Hauke Mehrtens

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

On 01/16/2015 10:47 PM, Rafał Miłecki wrote:
> Signed-off-by: Rafał Miłecki <[email protected]>
> ---
> 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]);
> }
>
> /*
>