[U-Boot] [BUG] common/console.c

[U-Boot] [BUG] common/console.c

[Heiko Schocher]

Hello Jean-Christophe,

I compilied actual u-boot for the muas3001 board and tried to start
it, but it crashes in common/console.c console_init_r () ...

After reversing your Patch:

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=c1de7a6daf9c657484e1c6d433f01fccd49a7f48#patch4

it works again fine.

It seems to me, that in console_init_r ()

struct list_head *list = device_get_list();

list points after device_get_list() not to a valid list ...

at least for MPC8260 systems!

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/1] device: clone device on register

[Jean-Christophe PLAGNIOL-VILLARD]

to prevent that the pointer will not be use for an other register

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 common/devices.c  |   24 +++++++++++++++++++++++-
 include/devices.h |    1 +
 2 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/common/devices.c b/common/devices.c
index 2977436..8beebe2 100644
--- a/common/devices.c
+++ b/common/devices.c
@@ -130,10 +130,32 @@ device_t* device_get_by_name(char* name)
 	return NULL;
 }
 
+device_t* device_clone(device_t *dev)
+{
+	device_t *_dev;
+
+	if(!dev)
+		return NULL;
+
+	_dev = calloc(1, sizeof(device_t));
+
+	if(!_dev)
+		return NULL;
+
+	memcpy(_dev, dev, sizeof(device_t));
+	strncpy(_dev->name, dev->name, 16);
+
+	return _dev;
+}
 
 int device_register (device_t * dev)
 {
-	list_add(&(dev->list), &(devs.list));
+	device_t *_dev;
+
+	_dev = device_clone(dev);
+	if(!_dev)
+		return -1;
+	list_add(&(_dev->list), &(devs.list));
 	return 0;
 }
 
diff --git a/include/devices.h b/include/devices.h
index 490016b..6b78d58 100644
--- a/include/devices.h
+++ b/include/devices.h
@@ -94,6 +94,7 @@ int	devices_init (void);
 int	device_deregister(char *devname);
 struct list_head* device_get_list(void);
 device_t* device_get_by_name(char* name);
+device_t* device_clone(device_t *dev);
 
 #ifdef CONFIG_LCD
 int	drv_lcd_init (void);
-- 
1.5.6.3

[U-Boot] [PATCH 1/1] device: clone device on register

[Haavard Skinnemoen]

Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> to prevent that the pointer will not be use for an other register
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Er. What? If a device is registered multiple times, that sounds like
something which should be fixed, not papered over like this...

Btw, while I'm all for getting rid of common/lists.c, your cryptic
one-line commit message is a shooting offense to anyone who tries to
understand WTF is going on...

Haavard

[U-Boot] [PATCH 1/1] device: clone device on register

[Haavard Skinnemoen]

Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > to prevent that the pointer will not be use for an other register
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>  
> 
> Er. What? If a device is registered multiple times, that sounds like
> something which should be fixed, not papered over like this...

I take that back. I didn't realize how much of a f***ing trainwreck the
device system is...apparently the callers expect device_register() to
clone the device, but this fact was hidden well within the old list
implementation. Your patches makes this much more apparent, which is
obviously good.

Oh, and your patch fixes the breakage I was seeing on avr32. Thanks.

Haavard

[U-Boot] [PATCH 1/1] device: clone device on register

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:15 Mon 01 Sep     , Haavard Skinnemoen wrote:
> Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > to prevent that the pointer will not be use for an other register
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>  
> > 
> > Er. What? If a device is registered multiple times, that sounds like
> > something which should be fixed, not papered over like this...
> 
> I take that back. I didn't realize how much of a f***ing trainwreck the
> device system is...apparently the callers expect device_register() to
> clone the device, but this fact was hidden well within the old list
> implementation. Your patches makes this much more apparent, which is
> obviously good.
> 
> Oh, and your patch fixes the breakage I was seeing on avr32. Thanks.

I've plan to cleanup this part during next release.

and redesign console management to simplify it and reduce its code.

I'm currently designing a generic bus/device/driver mecanism that will
simplify the implementation without increase the size.

I'll send a announce about it

Best Regards,
J.

[U-Boot] [PATCH 1/1] device: clone device on register

[Haavard Skinnemoen]

Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> I've plan to cleanup this part during next release.
> 
> and redesign console management to simplify it and reduce its code.
> 
> I'm currently designing a generic bus/device/driver mecanism that will
> simplify the implementation without increase the size.

Cool. Something like that is definitely needed.

Haavard

[U-Boot] [PATCH 1/1] device: clone device on register

[Wolfgang Denk]

Dear Jean-Christophe,

In message <20080901174820.432871d2@hskinnemo-gx745.norway.atmel.com>
Haavard Skinnemoen wrote:
>
> > to prevent that the pointer will not be use for an other register
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> 
> Er. What? If a device is registered multiple times, that sounds like
> something which should be fixed, not papered over like this...
> 
> Btw, while I'm all for getting rid of common/lists.c, your cryptic
> one-line commit message is a shooting offense to anyone who tries to
> understand WTF is going on...

I agree with Haavard on both accounts.

Can you please look into this again? Thanks

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
The human race is a race of cowards; and I am not  only  marching  in
that procession but carrying a banner.                   - Mark Twain

[U-Boot] [PATCH 1/1] device: clone device on register

[Wolfgang Denk]

I wrote:

> > > to prevent that the pointer will not be use for an other register
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > 
> > Er. What? If a device is registered multiple times, that sounds like
> > something which should be fixed, not papered over like this...
> > 
> > Btw, while I'm all for getting rid of common/lists.c, your cryptic
> > one-line commit message is a shooting offense to anyone who tries to
> > understand WTF is going on...
> 
> I agree with Haavard on both accounts.
> 
> Can you please look into this again? Thanks

OK, I learned that item (1) was actually a non-issue.

But can we please use a commit message that can be understood?
I cannot parse the current one.

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
The universe, they said, depended for its operation on the balance of
four forces which they identified as charm,  persuasion,  uncertainty
and bloody-mindedness.      -- Terry Pratchett, "The Light Fantastic"