[U-Boot] [PATCH 01/10] net: rtl8169: Add initialized eth_device structure

[U-Boot] [PATCH 01/10] net: rtl8169: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

rtl8169 does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/rtl8169.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c
index e45d1a5..a2f1c9e 100644
--- a/drivers/net/rtl8169.c
+++ b/drivers/net/rtl8169.c
@@ -894,6 +894,7 @@ int rtl8169_initialize(bd_t *bis)
 		debug ("rtl8169: REALTEK RTL8169 @0x%x\n", iobase);
 
 		dev = (struct eth_device *)malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 		sprintf (dev->name, "RTL8169#%d", card_number);
 
-- 
1.7.1

[U-Boot] [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

rtl8139 driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/rtl8139.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/rtl8139.c b/drivers/net/rtl8139.c
index db8a727..8d5e0f8 100644
--- a/drivers/net/rtl8139.c
+++ b/drivers/net/rtl8139.c
@@ -220,6 +220,7 @@ int rtl8139_initialize(bd_t *bis)
 		debug ("rtl8139: REALTEK RTL8139 @0x%x\n", iobase);
 
 		dev = (struct eth_device *)malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 		sprintf (dev->name, "RTL8139#%d", card_number);
 
-- 
1.7.1

[U-Boot] [PATCH 03/10] net: dc2114x: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

dc2114x driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/dc2114x.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/dc2114x.c b/drivers/net/dc2114x.c
index 5ae53e8..8dce857 100644
--- a/drivers/net/dc2114x.c
+++ b/drivers/net/dc2114x.c
@@ -279,6 +279,7 @@ int dc21x4x_initialize(bd_t *bis)
 		debug ("dc21x4x: DEC 21142 PCI Device @0x%x\n", iobase);
 
 		dev = (struct eth_device*) malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 #ifdef CONFIG_TULIP_FIX_DAVICOM
 		sprintf(dev->name, "Davicom#%d", card_number);
-- 
1.7.1

[U-Boot] [PATCH 04/10] net: eepro100: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

eepro100 driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/eepro100.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/eepro100.c b/drivers/net/eepro100.c
index 22e14e3..cdb233a 100644
--- a/drivers/net/eepro100.c
+++ b/drivers/net/eepro100.c
@@ -450,6 +450,7 @@ int eepro100_initialize (bd_t * bis)
 		}
 
 		dev = (struct eth_device *) malloc (sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 		sprintf (dev->name, "i82559#%d", card_number);
 		dev->priv = (void *) devno; /* this have to come before bus_to_phys() */
-- 
1.7.1

[U-Boot] [PATCH 05/10] net: fec_mxc: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

This prevents access to the member of eth_device which is not initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/fec_mxc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 2d4ffed..071276e 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -707,6 +707,7 @@ static int fec_probe(bd_t *bd)
 		puts("fec_mxc: not enough malloc memory\n");
 		return -ENOMEM;
 	}
+	memset(edev, 0, sizeof(*edev));
 	edev->priv = fec;
 	edev->init = fec_init;
 	edev->send = fec_send;
-- 
1.7.1

[U-Boot] [PATCH 06/10] net: natsemi: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

natsemi driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/natsemi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/natsemi.c b/drivers/net/natsemi.c
index e09da1d..68b56a6 100644
--- a/drivers/net/natsemi.c
+++ b/drivers/net/natsemi.c
@@ -321,6 +321,7 @@ natsemi_initialize(bd_t * bis)
 		}
 
 		dev = (struct eth_device *) malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 		sprintf(dev->name, "dp83815#%d", card_number);
 		dev->iobase = bus_to_phys(iobase);
-- 
1.7.1

[U-Boot] [PATCH 07/10] net: ns8382x: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

ns8382x driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/ns8382x.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ns8382x.c b/drivers/net/ns8382x.c
index 198f73d..2024912 100644
--- a/drivers/net/ns8382x.c
+++ b/drivers/net/ns8382x.c
@@ -340,6 +340,7 @@ ns8382x_initialize(bd_t * bis)
 		}
 
 		dev = (struct eth_device *) malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 
 		sprintf(dev->name, "dp8382x#%d", card_number);
 		dev->iobase = bus_to_phys(iobase);
-- 
1.7.1

[U-Boot] [PATCH 08/10] net: pcnet: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

pcnet driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/pcnet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/pcnet.c b/drivers/net/pcnet.c
index 99b6942..21618a1 100644
--- a/drivers/net/pcnet.c
+++ b/drivers/net/pcnet.c
@@ -187,6 +187,7 @@ int pcnet_initialize (bd_t * bis)
 		 * Allocate and pre-fill the device structure.
 		 */
 		dev = (struct eth_device *) malloc (sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 		dev->priv = (void *) devbusfn;
 		sprintf (dev->name, "pcnet#%d", dev_nr);
 
-- 
1.7.1

[U-Boot] [PATCH 09/10] net: tsi108_eth: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

tsi108_eth driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/tsi108_eth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/tsi108_eth.c b/drivers/net/tsi108_eth.c
index 079354a..564125e 100644
--- a/drivers/net/tsi108_eth.c
+++ b/drivers/net/tsi108_eth.c
@@ -731,7 +731,7 @@ int tsi108_eth_initialize (bd_t * bis)
 
 	for (index = 0; index < CONFIG_TSI108_ETH_NUM_PORTS; index++) {
 		dev = (struct eth_device *)malloc(sizeof(struct eth_device));
-
+		memset(dev, 0, sizeof(*dev));
 		sprintf (dev->name, "TSI108_eth%d", index);
 
 		dev->iobase = ETH_BASE + (index * ETH_PORT_OFFSET);
-- 
1.7.1

[U-Boot] [PATCH 10/10] net: uli526x: Add initialized eth_device structure

[iwamatsu at nigauri.org]

From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>

uli526x driver does not have write_hwaddr function.
However, eth stuff executes write_hwaddr function
because eth_device structure has not been initialized.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
CC: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/uli526x.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/uli526x.c b/drivers/net/uli526x.c
index 56eee7b..dcc1034 100644
--- a/drivers/net/uli526x.c
+++ b/drivers/net/uli526x.c
@@ -225,6 +225,7 @@ int uli526x_initialize(bd_t *bis)
 		iobase &= ~0xf;
 
 		dev = (struct eth_device *)malloc(sizeof *dev);
+		memset(dev, 0, sizeof(*dev));
 		sprintf(dev->name, "uli526x#%d", card_number);
 		db = (struct uli526x_board_info *)
 			malloc(sizeof(struct uli526x_board_info));
-- 
1.7.1

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Reinhard Meyer]

Dear All,
>   		dev = (struct eth_device *)malloc(sizeof *dev);
> +		memset(dev, 0, sizeof(*dev));
>
>   		sprintf (dev->name, "RTL8139#%d", card_number);

Apparently its quite common NOT to check malloc()'s possible
NULL return value... At least most NET drivers don't seem to...

Maybe another RFC to avoid duplicating code:

malloc_cleared_panic() to allocate and clear memory for a
really required structure and put a proper panic message if that
fails. Assuming that continuing u-boot once a driver cannot even
be initialized is futile, that would save even more code in each
driver.

Reinhard

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Nobuhiro Iwamatsu]

Hi,

2010/10/14 Reinhard Meyer <u-boot@emk-elektronik.de>:
> Dear All,
>> ? ? ? ? ? ? ? dev = (struct eth_device *)malloc(sizeof *dev);
>> + ? ? ? ? ? ? memset(dev, 0, sizeof(*dev));
>>
>> ? ? ? ? ? ? ? sprintf (dev->name, "RTL8139#%d", card_number);
>
> Apparently its quite common NOT to check malloc()'s possible
> NULL return value... At least most NET drivers don't seem to...

Oh, This is a stupid mistake.
Thanks.

>
> Maybe another RFC to avoid duplicating code:
>
> malloc_cleared_panic() to allocate and clear memory for a
> really required structure and put a proper panic message if that
> fails. Assuming that continuing u-boot once a driver cannot even
> be initialized is futile, that would save even more code in each
> driver.

I think that a function such as kzalloc of the linux kernel is convenient.

Best regards,
  Nobuhiro

-- 
Nobuhiro Iwamatsu

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4CB6A3D3.1020506@emk-elektronik.de> you wrote:
>
> >   		dev = (struct eth_device *)malloc(sizeof *dev);
> > +		memset(dev, 0, sizeof(*dev));
> >
> >   		sprintf (dev->name, "RTL8139#%d", card_number);
> 
> Apparently its quite common NOT to check malloc()'s possible
> NULL return value... At least most NET drivers don't seem to...
> 
> Maybe another RFC to avoid duplicating code:
> 
> malloc_cleared_panic() to allocate and clear memory for a
> really required structure and put a proper panic message if that
> fails. Assuming that continuing u-boot once a driver cannot even
> be initialized is futile, that would save even more code in each
> driver.

Don't invent the wheel. If you really want to take that route, then
copy existing solutions from other projects. Some of them use
xmalloc() for this purpose; see for example BusyBox:
http://git.busybox.net/busybox/tree/libbb/xfuncs_printf.c
lines 44...51

But note that panicing is NOT always the best thing to do. This
shouldbe reserved for really unrecoverable cases only.

Even if you cannot allocate a struct that is essential for your
network driver, then all that is not working is this network driver,
so this is NOT a reason to panic U-Boot. If someone cuts the network
cable or pulls the plug the end effect is the same, and you don;t want
U-Boot to panic because of htat, or do you?

Error handling is important, and needs to be done in a sensible way.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the beginning, there was nothing, which exploded.
                                - Terry Pratchett, _Lords and Ladies_

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Wolfgang Denk]

Dear Nobuhiro Iwamatsu,

In message <AANLkTikEa5OLvCEmNSJZtoddBrMk50=Zr=nv0CyL5VDy@mail.gmail.com> you wrote:
>
> >>               dev = (struct eth_device *)malloc(sizeof *> dev);
> >> +             memset(dev, 0, sizeof(*dev));
> >>
> >>               sprintf (dev->name, "RTL8139#%d", card_numbe> r);
> >
> > Apparently its quite common NOT to check malloc()'s possible
> > NULL return value... At least most NET drivers don't seem to...
>
> Oh, This is a stupid mistake.

I just want to point out that this was not your mistake!

The existing code did not check the return code either.

So actually you fixed a bug, while Reinhard noticed another,
unrelated bug.


> I think that a function such as kzalloc of the linux kernel is convenient.

Yes, but it needs careful consideration about how to handle malloc()
errors. It may not always be appropriate to crash the whole system.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I go on working for the same reason a hen goes on laying eggs."
- H. L. Mencken

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Reinhard Meyer]

Nobuhiro Iwamatsu schrieb:
>> Apparently its quite common NOT to check malloc()'s possible
>> NULL return value... At least most NET drivers don't seem to...
> 
> Oh, This is a stupid mistake.

That's NOT your mistake! It has been like that before.

Reinhard

[U-Boot] Bombs away! Was: [PATCH 02/10] net: rtl8139: Add initialized eth_device structure

[Mike Frysinger]

On Thursday, October 14, 2010 04:29:51 Wolfgang Denk wrote:
> Reinhard Meyer wrote:
> > >   		dev = (struct eth_device *)malloc(sizeof *dev);
> > > 
> > > +		memset(dev, 0, sizeof(*dev));
> > > 
> > >   		sprintf (dev->name, "RTL8139#%d", card_number);
> > 
> > Apparently its quite common NOT to check malloc()'s possible
> > NULL return value... At least most NET drivers don't seem to...
> > 
> > Maybe another RFC to avoid duplicating code:
> > 
> > malloc_cleared_panic() to allocate and clear memory for a
> > really required structure and put a proper panic message if that
> > fails. Assuming that continuing u-boot once a driver cannot even
> > be initialized is futile, that would save even more code in each
> > driver.
> 
> Don't invent the wheel. If you really want to take that route, then
> copy existing solutions from other projects. Some of them use
> xmalloc() for this purpose; see for example BusyBox:
> http://git.busybox.net/busybox/tree/libbb/xfuncs_printf.c
> lines 44...51

yes, xmalloc/xzalloc/xrealloc are the commonly named funcs.  and we'll  
generally want zalloc too.
-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/20101014/ec5e897c/attachment.pgp