[U-Boot] [PATCH 01/10] net: rtl8169: Add initialized eth_device structure
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);
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);
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
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
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
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
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
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
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);
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() */
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;
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);
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);
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);
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);
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));
participants (5)
-
iwamatsu＠nigauri.org -
Mike Frysinger -
Nobuhiro Iwamatsu
-
Reinhard Meyer -
Wolfgang Denk