[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Graeme Russ]

Hi All,

I finally got a few spare hours to do some U-Boot hacking and to my dismay
found the build for my board (eNET) had broken. It builds OK, but crashed
during Ethernet initialisation. I tried winding back to a last known good
commit without much luck.

I realised I had upgraded to Ubuntu 10.10 lately, and thought maybe a
tool-chain change was the culprit (was running gcc-4.4.1 and binutils 2.19
from source, now running Ubuntu gcc 4.4.5 and Binutils 2.20)

But, I think that commit ecee9324d73555e744593f3e0d387bec4c566f55 may be
'not quite right'. My board uses the rtl8139 driver, and the following
patch gets my board booting again:

diff --git a/drivers/net/rtl8139.c b/drivers/net/rtl8139.c
index db8a727..3646148 100644
--- a/drivers/net/rtl8139.c
+++ b/drivers/net/rtl8139.c
@@ -229,6 +229,7 @@ int rtl8139_initialize(bd_t *bis)
                dev->halt = rtl_disable;
                dev->send = rtl_transmit;
                dev->recv = rtl_poll;
+               dev->write_hwaddr = NULL;
 #ifdef CONFIG_MCAST_TFTP
                dev->mcast = rtl_bcast_addr;
 #endif

Just before this code we see that dev is malloc'd:

dev = (struct eth_device *)malloc(sizeof *dev);

So there is no guarantee that dev is NULL'd.

Has anyone else run into similar problems?

I saw discussion a little while ago regarding implementing a version of
malloc that returns cleared memory - did this gain any traction?

Regards,

Graeme

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Kumar Gala]

On Nov 6, 2010, at 5:13 PM, Graeme Russ wrote:

> Hi All,
> 
> I finally got a few spare hours to do some U-Boot hacking and to my dismay
> found the build for my board (eNET) had broken. It builds OK, but crashed
> during Ethernet initialisation. I tried winding back to a last known good
> commit without much luck.
> 
> I realised I had upgraded to Ubuntu 10.10 lately, and thought maybe a
> tool-chain change was the culprit (was running gcc-4.4.1 and binutils 2.19
> from source, now running Ubuntu gcc 4.4.5 and Binutils 2.20)
> 
> But, I think that commit ecee9324d73555e744593f3e0d387bec4c566f55 may be
> 'not quite right'. My board uses the rtl8139 driver, and the following
> patch gets my board booting again:
> 
> diff --git a/drivers/net/rtl8139.c b/drivers/net/rtl8139.c
> index db8a727..3646148 100644
> --- a/drivers/net/rtl8139.c
> +++ b/drivers/net/rtl8139.c
> @@ -229,6 +229,7 @@ int rtl8139_initialize(bd_t *bis)
>                dev->halt = rtl_disable;
>                dev->send = rtl_transmit;
>                dev->recv = rtl_poll;
> +               dev->write_hwaddr = NULL;
> #ifdef CONFIG_MCAST_TFTP
>                dev->mcast = rtl_bcast_addr;
> #endif
> 
> Just before this code we see that dev is malloc'd:
> 
> dev = (struct eth_device *)malloc(sizeof *dev);
> 
> So there is no guarantee that dev is NULL'd.
> 
> Has anyone else run into similar problems?
> 
> I saw discussion a little while ago regarding implementing a version of
> malloc that returns cleared memory - did this gain any traction?

I believe we are seeing similar behavior w/e1000 driver.

- k

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Mike Frysinger]

On Saturday, November 06, 2010 18:13:08 Graeme Russ wrote:
> I finally got a few spare hours to do some U-Boot hacking and to my dismay
> found the build for my board (eNET) had broken. It builds OK, but crashed
> during Ethernet initialisation. I tried winding back to a last known good
> commit without much luck.
> 
> I realised I had upgraded to Ubuntu 10.10 lately, and thought maybe a
> tool-chain change was the culprit (was running gcc-4.4.1 and binutils 2.19
> from source, now running Ubuntu gcc 4.4.5 and Binutils 2.20)
> 
> But, I think that commit ecee9324d73555e744593f3e0d387bec4c566f55 may be
> 'not quite right'. My board uses the rtl8139 driver, and the following
> patch gets my board booting again:
>
> Just before this code we see that dev is malloc'd:
> 
> dev = (struct eth_device *)malloc(sizeof *dev);
> 
> So there is no guarantee that dev is NULL'd.

rtl8139 is broken.  it should be clearing its memory.

someone posted a whole bunch of patches to memset() net drivers ...

> I saw discussion a little while ago regarding implementing a version of
> malloc that returns cleared memory - did this gain any traction?

i dont think anyone posted a patch.  it would make sense though to generalize 
the zalloc() code since some places are already doing it.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101109/42fb1c9c/attachment.pgp 

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Graeme Russ]

On 10/11/10 14:36, Mike Frysinger wrote:
> On Saturday, November 06, 2010 18:13:08 Graeme Russ wrote:

>> I saw discussion a little while ago regarding implementing a version of
>> malloc that returns cleared memory - did this gain any traction?
> 
> i dont think anyone posted a patch.  it would make sense though to generalize 
> the zalloc() code since some places are already doing it.

Actually, I think we should be using calloc() more than we are

Regards,

Graeme

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Mike Frysinger]

On Friday, November 12, 2010 05:55:55 Graeme Russ wrote:
> On 10/11/10 14:36, Mike Frysinger wrote:
> > On Saturday, November 06, 2010 18:13:08 Graeme Russ wrote:
> >> I saw discussion a little while ago regarding implementing a version of
> >> malloc that returns cleared memory - did this gain any traction?
> > 
> > i dont think anyone posted a patch.  it would make sense though to
> > generalize the zalloc() code since some places are already doing it.
> 
> Actually, I think we should be using calloc() more than we are

if we want to define zalloc() to calloc(), then sure.  but forcing people to 
use calloc() with a size=1 is silly.  the API is not nearly as obvious to 
passing observers as zalloc().
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101112/25905c2e/attachment.pgp 

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Graeme Russ]

On 13/11/10 12:11, Mike Frysinger wrote:
> On Friday, November 12, 2010 05:55:55 Graeme Russ wrote:
>> On 10/11/10 14:36, Mike Frysinger wrote:
>>> On Saturday, November 06, 2010 18:13:08 Graeme Russ wrote:
>>>> I saw discussion a little while ago regarding implementing a version of
>>>> malloc that returns cleared memory - did this gain any traction?
>>>
>>> i dont think anyone posted a patch.  it would make sense though to
>>> generalize the zalloc() code since some places are already doing it.
>>
>> Actually, I think we should be using calloc() more than we are
> 
> if we want to define zalloc() to calloc(), then sure.  but forcing people to 
> use calloc() with a size=1 is silly.  the API is not nearly as obvious to 
> passing observers as zalloc().

Hmm, U-Boot has a zalloc in lib/gunzip.c which does not zero memory

void *zalloc(void *x, unsigned items, unsigned size)
{
	void *p;

	size *= items;
	size = (size + ZALLOC_ALIGNMENT - 1) & ~(ZALLOC_ALIGNMENT - 1);

	p = malloc (size);

	return (p);
}

and mtd has kzalloc defined thus:

#define kzalloc(size, flags)	calloc(size, 1)

So there is some confusion surrounding 'zalloc' anyway ;)

Regards,

Graeme

[U-Boot] Commit ecee9... (Program net device MAC addresses after initializing) breakage

[Mike Frysinger]

On Friday, November 12, 2010 20:51:12 Graeme Russ wrote:
> On 13/11/10 12:11, Mike Frysinger wrote:
> > On Friday, November 12, 2010 05:55:55 Graeme Russ wrote:
> >> On 10/11/10 14:36, Mike Frysinger wrote:
> >>> On Saturday, November 06, 2010 18:13:08 Graeme Russ wrote:
> >>>> I saw discussion a little while ago regarding implementing a version
> >>>> of malloc that returns cleared memory - did this gain any traction?
> >>> 
> >>> i dont think anyone posted a patch.  it would make sense though to
> >>> generalize the zalloc() code since some places are already doing it.
> >> 
> >> Actually, I think we should be using calloc() more than we are
> > 
> > if we want to define zalloc() to calloc(), then sure.  but forcing people
> > to use calloc() with a size=1 is silly.  the API is not nearly as
> > obvious to passing observers as zalloc().
> 
> Hmm, U-Boot has a zalloc in lib/gunzip.c which does not zero memory

yes, that's why i said we should generalize things
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101112/e64b75aa/attachment.pgp