Possible memory leak in cpu/ppc4xx/4xx_enet.c

U-Boot-Users Dave Littell 2 participants 5 messages Mar 15, 2008 – Mar 17, 2008

Share & tools

Dave Littell

author

2008-03-15 02:44 UTC #0 lore raw

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

Stefan Roese

2008-03-15 06:07 UTC #1 lore raw

Dave,

Quoted 10 lines · Dave Littell

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().

Quoted 2 lines

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
=====================================================================

Dave Littell

author

2008-03-15 15:12 UTC #2 lore raw

Quoted 9 lines · Stefan Roese

Dave,
Quoted 6 lines · Dave Littell
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...

Quoted 9 lines

Quoted 4 lines
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?

Quoted 5 lines

Quoted 2 lines
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

Stefan Roese

2008-03-17 08:56 UTC #3 lore raw

Quoted 5 lines · Dave Littell

Quoted 5 lines
Quoted 4 lines
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.

Quoted 8 lines

Quoted 3 lines
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?

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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.

cpu/ppc4xx/4xx_enet.c +4 -1

index d990250..479cb40 100644
@@ -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

Dave Littell

author

2008-03-17 23:10 UTC #4 lore raw

Quoted 8 lines · Stefan Roese

Quoted 7 lines · Dave Littell
Quoted 7 lines
Quoted 4 lines
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().

Quoted 8 lines

Quoted 4 lines
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.

Quoted 9 lines

Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
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!

Quoted 10 lines

cpu/ppc4xx/4xx_enet.c +0 -0

index d990250..479cb40 100644
@@ -1998,9 +1998,12 @@ int ppc_4xx_eth_initialize (bd_t * bis)
 }
 
 #if !defined(CONFIG_NET_MULTI)

cpu/ppc4xx/4xx_enet.c int ppc_4xx_eth_initialize (bd_t * bis) · line 2009

Quoted 10 lines

cpu/ppc4xx/4xx_enet.c +4 -1

-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