[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[Dave Littell]

Hi all,

I've seen an issue with my 1.3.1-based port of U-Boot where a relatively
large number of pings issued from the command line will ultimately fail
with the message:

Cannot allocate private hw data for eth_device...

I looked around this message and it seems that ppc_4xx_eth_initialize()
malloc()?s a EMAC_4XX_HW_PST structure and assigns it to dev->priv.
However, nothing ever frees this buffer so standing on a ping will
ultimately pull down all the available memory.

One fix might be to free( dev->priv ) at the end of ppc_4xx_eth_halt().
 I tried this today and noted no ill effects.


Thanks,
Dave

[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[Stefan Roese]

Dave,

On Saturday 15 March 2008, Dave Littell wrote:
> I've seen an issue with my 1.3.1-based port of U-Boot where a relatively
> large number of pings issued from the command line will ultimately fail
> with the message:
>
> Cannot allocate private hw data for eth_device...
>
> I looked around this message and it seems that ppc_4xx_eth_initialize()
> malloc()?s a EMAC_4XX_HW_PST structure and assigns it to dev->priv.
> However, nothing ever frees this buffer so standing on a ping will
> ultimately pull down all the available memory.

Hmmm. IIRC ppc_4xx_eth_initialize() is only called once upon bootup for 
ethernet driver initialization. The function called each time before network 
action is ppc_4xx_eth_init().

> One fix might be to free( dev->priv ) at the end of ppc_4xx_eth_halt().
>  I tried this today and noted no ill effects.

So did it work for you?

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[Dave Littell]

Stefan Roese wrote:
> Dave,
> 
> On Saturday 15 March 2008, Dave Littell wrote:
>> I've seen an issue with my 1.3.1-based port of U-Boot where a relatively
>> large number of pings issued from the command line will ultimately fail
>> with the message:
>>
>> Cannot allocate private hw data for eth_device...
>>
>> I looked around this message and it seems that ppc_4xx_eth_initialize()
>> malloc()?s a EMAC_4XX_HW_PST structure and assigns it to dev->priv.
>> However, nothing ever frees this buffer so standing on a ping will
>> ultimately pull down all the available memory.
> 
> Hmmm. IIRC ppc_4xx_eth_initialize() is only called once upon bootup for 
> ethernet driver initialization. The function called each time before network 
> action is ppc_4xx_eth_init().
> 

It looks like ppc_4xx_eth_initialize() is called from eth_init() and
ppc_4xx_eth_init() is only called if there's an emac0_dev.

Hm?

>> One fix might be to free( dev->priv ) at the end of ppc_4xx_eth_halt().
>>  I tried this today and noted no ill effects.
> 
> So did it work for you?
> 

It seems to be working well.


Thanks,
Dave

[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[Stefan Roese]

On Saturday 15 March 2008, Dave Littell wrote:
> >> I looked around this message and it seems that ppc_4xx_eth_initialize()
> >> malloc()?s a EMAC_4XX_HW_PST structure and assigns it to dev->priv.
> >> However, nothing ever frees this buffer so standing on a ping will
> >> ultimately pull down all the available memory.
> >
> > Hmmm. IIRC ppc_4xx_eth_initialize() is only called once upon bootup for
> > ethernet driver initialization. The function called each time before
> > network action is ppc_4xx_eth_init().
>
> It looks like ppc_4xx_eth_initialize() is called from eth_init() and
> ppc_4xx_eth_init() is only called if there's an emac0_dev.
>
> Hm?

I see. You don't have CONFIG_NET_MULTI defined. Which 4xx platform are we 
talking about btw?

> >> One fix might be to free( dev->priv ) at the end of ppc_4xx_eth_halt().
> >>  I tried this today and noted no ill effects.
> >
> > So did it work for you?
>
> It seems to be working well.

OK. But your patch will most likely break all platforms with CONFIG_NET_MULTI 
defined. Please give the attached patch a try and let me know if it works 
too. I'll push it online if it fixes the problem.

diff --git a/cpu/ppc4xx/4xx_enet.c b/cpu/ppc4xx/4xx_enet.c
index d990250..479cb40 100644
--- a/cpu/ppc4xx/4xx_enet.c
+++ b/cpu/ppc4xx/4xx_enet.c
@@ -1998,9 +1998,12 @@ int ppc_4xx_eth_initialize (bd_t * bis)
 }

 #if !defined(CONFIG_NET_MULTI)
-void eth_halt (void) {
+void eth_halt (void)
+{
        if (emac0_dev) {
                ppc_4xx_eth_halt(emac0_dev);
+               if (emac0_dev->priv)
+                       free(emac0_dev->priv);
                free(emac0_dev);
                emac0_dev = NULL;
        }


Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] Possible memory leak in cpu/ppc4xx/4xx_enet.c

[Dave Littell]

Stefan Roese wrote:
> On Saturday 15 March 2008, Dave Littell wrote:
>>>> I looked around this message and it seems that ppc_4xx_eth_initialize()
>>>> malloc()?s a EMAC_4XX_HW_PST structure and assigns it to dev->priv.
>>>> However, nothing ever frees this buffer so standing on a ping will
>>>> ultimately pull down all the available memory.
>>> Hmmm. IIRC ppc_4xx_eth_initialize() is only called once upon bootup for
>>> ethernet driver initialization. The function called each time before
>>> network action is ppc_4xx_eth_init().
>> It looks like ppc_4xx_eth_initialize() is called from eth_init() and
>> ppc_4xx_eth_init() is only called if there's an emac0_dev.
>>
>> Hm?
> 
> I see. You don't have CONFIG_NET_MULTI defined. Which 4xx platform are we 
> talking about btw?
> 

This is a platform similar to the AMCC sequoia, but CONFIG_NET_MULTI is
not defined.

>>>> One fix might be to free( dev->priv ) at the end of ppc_4xx_eth_halt().
>>>>  I tried this today and noted no ill effects.
>>> So did it work for you?
>> It seems to be working well.
> 
> OK. But your patch will most likely break all platforms with CONFIG_NET_MULTI 
> defined. Please give the attached patch a try and let me know if it works 
> too. I'll push it online if it fixes the problem.
> 

It does indeed seem to work - thanks!

> diff --git a/cpu/ppc4xx/4xx_enet.c b/cpu/ppc4xx/4xx_enet.c
> index d990250..479cb40 100644
> --- a/cpu/ppc4xx/4xx_enet.c
> +++ b/cpu/ppc4xx/4xx_enet.c
> @@ -1998,9 +1998,12 @@ int ppc_4xx_eth_initialize (bd_t * bis)
>  }
> 
>  #if !defined(CONFIG_NET_MULTI)
> -void eth_halt (void) {
> +void eth_halt (void)
> +{
>         if (emac0_dev) {
>                 ppc_4xx_eth_halt(emac0_dev);
> +               if (emac0_dev->priv)
> +                       free(emac0_dev->priv);
>                 free(emac0_dev);
>                 emac0_dev = NULL;
>         }
> 
> 

Thanks,
Dave