[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Wolfgang Denk]

In message <20041122100122.GA5418@simek> you wrote:
> 
> this is resend of mail from Fri Nov 19 18:49:37. I can't find it in
> archive.
> 
> Index: common/cmd_mem.c
> ===================================================================

I hereby reject this patch.

There is no explanation what it does or which problem it is  supposed
to fix.

There is also no CHANGELOG entry.

Please read the README and resubmit with the required information.

Sorry.


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The following statement is not true.  The previous statement is true.

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Ladislav Michl]

On Mon, Nov 22, 2004 at 12:07:18PM +0100, Wolfgang Denk wrote:
> There is no explanation what it does or which problem it is  supposed
> to fix.

When CFG_MEMTEST_SCRATCH is undefined alternate memory test in do_mem_mtest
(with CFG_ALT_MEMTEST defined) dereferences null pointer. It defines:
vu_long *dummy = NULL;
and later does:
*dummy  = ~val;

> There is also no CHANGELOG entry.

CHANGELOG
* Patch by Ladislav Michl, 22 November 2004
  - Fix NULL pointer dereference in alternate memory test (CFG_ALT_MEMTEST)
    when if no CFG_MEMTEST_SCRATCH area defined

Index: common/cmd_mem.c
===================================================================
RCS file: /cvsroot/u-boot/u-boot/common/cmd_mem.c,v
retrieving revision 1.19
diff -p -u -r1.19 cmd_mem.c
--- common/cmd_mem.c	10 Oct 2004 23:27:33 -0000	1.19
+++ common/cmd_mem.c	22 Nov 2004 11:21:43 -0000
@@ -646,8 +646,9 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int 
 	vu_long	num_words;
 #if defined(CFG_MEMTEST_SCRATCH)
 	vu_long *dummy = (vu_long*)CFG_MEMTEST_SCRATCH;
+#define write_dummy(val)	do { *dummy  = ~val; } while (0)
 #else
-	vu_long *dummy = NULL;
+#define write_dummy(val)	do { } while (0)
 #endif
 	int	j;
 	int iterations = 1;
@@ -723,7 +724,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int 
 		    val = bitpattern[j];
 		    for(; val != 0; val <<= 1) {
 			*addr  = val;
-			*dummy  = ~val; /* clear the test data off of the bus */
+			write_dummy(~val); /* clear the test data off of the bus */
 			readback = *addr;
 			if(readback != val) {
 			     printf ("FAILURE (data line): "
@@ -731,11 +732,11 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int 
 					  val, readback);
 			}
 			*addr  = ~val;
-			*dummy  = val;
+			write_dummy(val);
 			readback = *addr;
 			if(readback != ~val) {
 			    printf ("FAILURE (data line): "
-				"Is %08lx, should be %08lx\n",
+				"is %08lx, should be %08lx\n",
 					readback, ~val);
 			}
 		    }

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Ladislav Michl]

On Thu, Nov 25, 2004 at 12:22:23PM +0100, Ladislav Michl wrote:
> CHANGELOG
> * Patch by Ladislav Michl, 22 November 2004
>   - Fix NULL pointer dereference in alternate memory test (CFG_ALT_MEMTEST)
>     when if no CFG_MEMTEST_SCRATCH area defined

Wolfgang, any comments on this?

Regards,
	ladis

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Wolfgang Denk]

In message <20041129092025.GB5558@simek> you wrote:
> On Thu, Nov 25, 2004 at 12:22:23PM +0100, Ladislav Michl wrote:
> > CHANGELOG
> > * Patch by Ladislav Michl, 22 November 2004
> >   - Fix NULL pointer dereference in alternate memory test (CFG_ALT_MEMTEST)
> >     when if no CFG_MEMTEST_SCRATCH area defined
> 
> Wolfgang, any comments on this?

Not yet; it's in my queue.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Women professionals do tend to over-compensate.
	-- Dr. Elizabeth Dehaver, "Where No Man Has Gone Before",
	   stardate 1312.9.

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[VanBaren, Gerald]

> -----Original Message-----
> From: u-boot-users-admin at lists.sourceforge.net
> [mailto:u-boot-users-admin at lists.sourceforge.net] On Behalf
> Of Ladislav Michl
> Sent: Thursday, November 25, 2004 6:22 AM
> To: u-boot-users at lists.sourceforge.net
> Subject: [U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do
> not dereference NULL ptr
>
> On Mon, Nov 22, 2004 at 12:07:18PM +0100, Wolfgang Denk wrote:
> > There is no explanation what it does or which problem it is
>  supposed
> > to fix.
>
> When CFG_MEMTEST_SCRATCH is undefined alternate memory test
> in do_mem_mtest (with CFG_ALT_MEMTEST defined) dereferences
> null pointer. It defines:
> vu_long *dummy = NULL;
> and later does:
> *dummy  = ~val;
>
> > There is also no CHANGELOG entry.
>
> CHANGELOG
> * Patch by Ladislav Michl, 22 November 2004
>   - Fix NULL pointer dereference in alternate memory test
> (CFG_ALT_MEMTEST)
>     when if no CFG_MEMTEST_SCRATCH area defined
>
> Index: common/cmd_mem.c
> ===================================================================
> RCS file: /cvsroot/u-boot/u-boot/common/cmd_mem.c,v
> retrieving revision 1.19
> diff -p -u -r1.19 cmd_mem.c
> --- common/cmd_mem.c	10 Oct 2004 23:27:33 -0000	1.19
> +++ common/cmd_mem.c	22 Nov 2004 11:21:43 -0000
> @@ -646,8 +646,9 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int
>  	vu_long	num_words;
>  #if defined(CFG_MEMTEST_SCRATCH)
>  	vu_long *dummy = (vu_long*)CFG_MEMTEST_SCRATCH;
> +#define write_dummy(val)	do { *dummy  = ~val; } while (0)
>  #else
> -	vu_long *dummy = NULL;
> +#define write_dummy(val)	do { } while (0)
>  #endif
>  	int	j;
>  	int iterations = 1;
> @@ -723,7 +724,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int
>  		    val = bitpattern[j];
>  		    for(; val != 0; val <<= 1) {
>  			*addr  = val;
> -			*dummy  = ~val; /* clear the test data
> off of the bus */
> +			write_dummy(~val); /* clear the test
> data off of the bus */
>  			readback = *addr;
>  			if(readback != val) {
>  			     printf ("FAILURE (data line): "
> @@ -731,11 +732,11 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int
>  					  val, readback);
>  			}
>  			*addr  = ~val;
> -			*dummy  = val;
> +			write_dummy(val);
>  			readback = *addr;
>  			if(readback != ~val) {
>  			    printf ("FAILURE (data line): "
> -				"Is %08lx, should be %08lx\n",
> +				"is %08lx, should be %08lx\n",
>  					readback, ~val);
>  			}
>  		    }

Ladislav:

You are fixing the problem in the wrong way.  You should point "dummy"
at a location in RAM that is writable rather than "fixing" the code by
suppressing the dummy write.  Being pedantic here, dummy isn't NULL, it
is a pointer to the RAM location 0x00000000 (which just happens to be
the same as NULL ;-).  Strictly speaking, the assignment should have
been written "vu_long *dummy = 0x00000000;".

The problem here is that the code assumes 0x00000000 is writable and
apparently this isn't the case on your board.  It obviously is a good
assumption on the other boards supported by u-boot (at least the ones
that use this POST code :-).  The proper fix is, for your board, change
the assignment "vu_long *dummy = 0x00000000;" to point to writable RAM
on your board.  Suppressing the write to 0x00000000 will make the memory
test ineffective on all boards (bad, really bad).

The purpose of the "*dummy = ~val" statement is to discharge the data
bus lines.  The problem this is addressing is when the previous line
"*addr = val" is executed, the data bus has on it the value "val".
Since there is inherent capacitance on the bus lines, that value will
persist (float) on the bus for a finite time, longer than it takes for a
processor to do the "readback = *addr".  The result is that, if the
memory doesn't respond and the bus drivers remain tristated, you will
think the memory responded properly because you read the residual charge
on the bus.

The write of "~val" to dummy is a throw-away operation whose purpose is
to put something other than "val" on the bus so that a read of the
target location won't give a false positive if the memory isn't working
properly.  Where "dummy" points is immaterial as long as "~val" gets out
on the data bus.  Normally you would point it in your RAM space, but you
actually could point it anywhere that causes a valid write cycle on the
data bus.

Regards,
gvb

******************************************
The information contained in, or attached to, this e-mail, may contain confidential information and is intended solely for the use of the individual or entity to whom they are addressed and may be subject to legal privilege.  If you have received this e-mail in error you should notify the sender immediately by reply e-mail, delete the message from your system and notify your system manager.  Please do not copy it for any purpose, or disclose its contents to any other person.  The views or opinions presented in this e-mail are solely those of the author and do not necessarily represent those of the company.  The recipient should check this e-mail and any attachments for the presence of viruses.  The company accepts no liability for any damage caused, directly or indirectly, by any virus transmitted in this email.
******************************************

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Ladislav Michl]

On Mon, Nov 29, 2004 at 07:15:48AM -0700, VanBaren, Gerald (AGRE) wrote:
> You are fixing the problem in the wrong way.
[nice explanation deleted]

Gerald, many thanks for such a long and nice explanation. Unfortunately
I didn't receive mails in this thread in chronologic order, so I saw
only last post. Patch is attached to my previous mail.

Best regards,
	ladis

[U-Boot-Users] Re: [PATCH] !CFG_MEMTEST_SCRATCH - do not dereference NULL ptr

[Wolfgang Denk]

In message <20041129153532.GA14854@simek> you wrote:
> On Mon, Nov 29, 2004 at 07:15:48AM -0700, VanBaren, Gerald (AGRE) wrote:
> > You are fixing the problem in the wrong way.
> [nice explanation deleted]
> 
> Gerald, many thanks for such a long and nice explanation. Unfortunately
> I didn't receive mails in this thread in chronologic order, so I saw
> only last post. Patch is attached to my previous mail.

I reject this patch due to the reasons explained by Gerald VanBaren.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Es ist nicht genug zu wissen, man mu? auch anwenden; es ist nicht ge-
nug zu wollen, man mu? auch tun.   -- Goethe, Maximen und Reflexionen