[U-Boot] [PATCH 0/2] ext2: Cleanup and simplify sector access code

[U-Boot] [PATCH 0/2] ext2: Cleanup and simplify sector access code

[Anton Staaf]

This patch set first cleans up all of the chack patch warnings
and errors in fs/ext2/dev.c and then cleans up the partial sector
access logic in the ext2fs_devread function.

I didn't see a file system or ext2 custodian so I've CC'ed Andy
the custodian for MMC as that seems closest.  Please let me know
if I should bring this to someone elses attention.

Signed-off-by: Anton Staaf <robotboy@chromium.org>
Cc: Andy Fleming <afleming@freescale.com>

[U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations

[Anton Staaf]

Fix all checkpatch violations in the low level Ext2 block
device reading code.  This is done in preparation for cleaning
up the partial sector access code.

Signed-off-by: Anton Staaf <robotboy@chromium.org>
Cc: Andy Fleming <afleming@freescale.com>
---
 fs/ext2/dev.c |   82 ++++++++++++++++++++++++++++++---------------------------
 1 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/fs/ext2/dev.c b/fs/ext2/dev.c
index 3b49650..4365b3b 100644
--- a/fs/ext2/dev.c
+++ b/fs/ext2/dev.c
@@ -31,7 +31,7 @@
 static block_dev_desc_t *ext2fs_block_dev_desc;
 static disk_partition_t part_info;
 
-int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int part)
+int ext2fs_set_blk_dev(block_dev_desc_t *rbdd, int part)
 {
 	ext2fs_block_dev_desc = rbdd;
 
@@ -46,51 +46,55 @@ int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int part)
 			return 0;
 		}
 	}
-	return (part_info.size);
+	return part_info.size;
 }
 
 
-int ext2fs_devread (int sector, int byte_offset, int byte_len, char *buf) {
+int ext2fs_devread(int sector, int byte_offset, int byte_len, char *buf)
+{
 	char sec_buf[SECTOR_SIZE];
 	unsigned block_len;
 
-/*
- *  Check partition boundaries
- */
-	if ((sector < 0)
-	    || ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
+	/*
+	 *  Check partition boundaries
+	 */
+	if ((sector < 0) ||
+	    ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
 		part_info.size)) {
-	/*      errnum = ERR_OUTSIDE_PART; */
-		printf (" ** ext2fs_devread() read outside partition sector %d\n", sector);
-		return (0);
+		/* errnum = ERR_OUTSIDE_PART; */
+		printf(" ** %s read outside partition sector %d\n",
+		       __func__,
+		       sector);
+		return 0;
 	}
 
-/*
- *  Get the read to the beginning of a partition.
- */
+	/*
+	 *  Get the read to the beginning of a partition.
+	 */
 	sector += byte_offset >> SECTOR_BITS;
 	byte_offset &= SECTOR_SIZE - 1;
 
-	debug (" <%d, %d, %d>\n", sector, byte_offset, byte_len);
+	debug(" <%d, %d, %d>\n", sector, byte_offset, byte_len);
 
 	if (ext2fs_block_dev_desc == NULL) {
-		printf ("** Invalid Block Device Descriptor (NULL)\n");
-		return (0);
+		printf(" ** %s Invalid Block Device Descriptor (NULL)\n",
+		       __func__);
+		return 0;
 	}
 
 	if (byte_offset != 0) {
 		/* read first part which isn't aligned with start of sector */
 		if (ext2fs_block_dev_desc->
-		    block_read (ext2fs_block_dev_desc->dev,
-				part_info.start + sector, 1,
-				(unsigned long *) sec_buf) != 1) {
-			printf (" ** ext2fs_devread() read error **\n");
-			return (0);
+		    block_read(ext2fs_block_dev_desc->dev,
+			       part_info.start + sector, 1,
+			       (unsigned long *) sec_buf) != 1) {
+			printf(" ** %s read error **\n", __func__);
+			return 0;
 		}
-		memcpy (buf, sec_buf + byte_offset,
-			min (SECTOR_SIZE - byte_offset, byte_len));
-		buf += min (SECTOR_SIZE - byte_offset, byte_len);
-		byte_len -= min (SECTOR_SIZE - byte_offset, byte_len);
+		memcpy(buf, sec_buf + byte_offset,
+		       min(SECTOR_SIZE - byte_offset, byte_len));
+		buf += min(SECTOR_SIZE - byte_offset, byte_len);
+		byte_len -= min(SECTOR_SIZE - byte_offset, byte_len);
 		sector++;
 	}
 
@@ -111,13 +115,13 @@ int ext2fs_devread (int sector, int byte_offset, int byte_len, char *buf) {
 		return 1;
 	}
 
-	if (ext2fs_block_dev_desc->block_read (ext2fs_block_dev_desc->dev,
-					       part_info.start + sector,
-					       block_len / SECTOR_SIZE,
-					       (unsigned long *) buf) !=
+	if (ext2fs_block_dev_desc->block_read(ext2fs_block_dev_desc->dev,
+					      part_info.start + sector,
+					      block_len / SECTOR_SIZE,
+					      (unsigned long *) buf) !=
 	    block_len / SECTOR_SIZE) {
-		printf (" ** ext2fs_devread() read error - block\n");
-		return (0);
+		printf(" ** %s read error - block\n", __func__);
+		return 0;
 	}
 	block_len = byte_len & ~(SECTOR_SIZE - 1);
 	buf += block_len;
@@ -127,13 +131,13 @@ int ext2fs_devread (int sector, int byte_offset, int byte_len, char *buf) {
 	if (byte_len != 0) {
 		/* read rest of data which are not in whole sector */
 		if (ext2fs_block_dev_desc->
-		    block_read (ext2fs_block_dev_desc->dev,
-				part_info.start + sector, 1,
-				(unsigned long *) sec_buf) != 1) {
-			printf (" ** ext2fs_devread() read error - last part\n");
-			return (0);
+		    block_read(ext2fs_block_dev_desc->dev,
+			       part_info.start + sector, 1,
+			       (unsigned long *) sec_buf) != 1) {
+			printf(" ** %s read error - last part\n", __func__);
+			return 0;
 		}
-		memcpy (buf, sec_buf, byte_len);
+		memcpy(buf, sec_buf, byte_len);
 	}
-	return (1);
+	return 1;
 }
-- 
1.7.3.1

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Anton Staaf]

Previously reading or writing zero full sectors (reading the end of
one sector and the beginning of the next for example) was special
cased and involved stack allocating a second sector buffer.  This
change uses the same code path for this case as well as when there
are a non-zero number of full sectors to access.  The result is
easier to read and reduces the maximum stack used.

Signed-off-by: Anton Staaf <robotboy@chromium.org>
Cc: Andy Fleming <afleming@freescale.com>
---
 fs/ext2/dev.c |   42 +++++++++++++++---------------------------
 1 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/fs/ext2/dev.c b/fs/ext2/dev.c
index 4365b3b..78851d0 100644
--- a/fs/ext2/dev.c
+++ b/fs/ext2/dev.c
@@ -53,7 +53,7 @@ int ext2fs_set_blk_dev(block_dev_desc_t *rbdd, int part)
 int ext2fs_devread(int sector, int byte_offset, int byte_len, char *buf)
 {
 	char sec_buf[SECTOR_SIZE];
-	unsigned block_len;
+	unsigned sectors;
 
 	/*
 	 *  Check partition boundaries
@@ -98,35 +98,23 @@ int ext2fs_devread(int sector, int byte_offset, int byte_len, char *buf)
 		sector++;
 	}
 
-	if (byte_len == 0)
-		return 1;
-
 	/*  read sector aligned part */
-	block_len = byte_len & ~(SECTOR_SIZE - 1);
-
-	if (block_len == 0) {
-		u8 p[SECTOR_SIZE];
-
-		block_len = SECTOR_SIZE;
-		ext2fs_block_dev_desc->block_read(ext2fs_block_dev_desc->dev,
-						  part_info.start + sector,
-						  1, (unsigned long *)p);
-		memcpy(buf, p, byte_len);
-		return 1;
-	}
+	sectors = byte_len / SECTOR_SIZE;
+
+	if (sectors > 0) {
+		if (ext2fs_block_dev_desc->block_read(
+			ext2fs_block_dev_desc->dev,
+			part_info.start + sector,
+			sectors,
+			(unsigned long *) buf) != sectors) {
+			printf(" ** %s read error - block\n", __func__);
+			return 0;
+		}
 
-	if (ext2fs_block_dev_desc->block_read(ext2fs_block_dev_desc->dev,
-					      part_info.start + sector,
-					      block_len / SECTOR_SIZE,
-					      (unsigned long *) buf) !=
-	    block_len / SECTOR_SIZE) {
-		printf(" ** %s read error - block\n", __func__);
-		return 0;
+		buf += sectors * SECTOR_SIZE;
+		byte_len -= sectors * SECTOR_SIZE;
+		sector += sectors;
 	}
-	block_len = byte_len & ~(SECTOR_SIZE - 1);
-	buf += block_len;
-	byte_len -= block_len;
-	sector += block_len / SECTOR_SIZE;
 
 	if (byte_len != 0) {
 		/* read rest of data which are not in whole sector */
-- 
1.7.3.1

[U-Boot] [PATCH 0/2] ext2: Cleanup and simplify sector access code

[Anton Staaf]

I just wanted to check that this patch set hadn't fallen completely off the
radar.  :)  What can I do to help it along?

Thanks,
    Anton

On Mon, Jun 13, 2011 at 2:40 PM, Anton Staaf <robotboy@chromium.org> wrote:

> This patch set first cleans up all of the chack patch warnings
> and errors in fs/ext2/dev.c and then cleans up the partial sector
> access logic in the ext2fs_devread function.
>
> I didn't see a file system or ext2 custodian so I've CC'ed Andy
> the custodian for MMC as that seems closest.  Please let me know
> if I should bring this to someone elses attention.
>
> Signed-off-by: Anton Staaf <robotboy@chromium.org>
> Cc: Andy Fleming <afleming@freescale.com>
>

[U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations

[Detlev Zundel]

Hi Anton,

> Fix all checkpatch violations in the low level Ext2 block
> device reading code.  This is done in preparation for cleaning
> up the partial sector access code.
>
> Signed-off-by: Anton Staaf <robotboy@chromium.org>
> Cc: Andy Fleming <afleming@freescale.com>
> ---
>  fs/ext2/dev.c |   82 ++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 43 insertions(+), 39 deletions(-)
>
> diff --git a/fs/ext2/dev.c b/fs/ext2/dev.c
> index 3b49650..4365b3b 100644
> --- a/fs/ext2/dev.c
> +++ b/fs/ext2/dev.c
> @@ -31,7 +31,7 @@
>  static block_dev_desc_t *ext2fs_block_dev_desc;
>  static disk_partition_t part_info;
>  
> -int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int part)
> +int ext2fs_set_blk_dev(block_dev_desc_t *rbdd, int part)
>  {
>  	ext2fs_block_dev_desc = rbdd;
>  
> @@ -46,51 +46,55 @@ int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int part)
>  			return 0;
>  		}
>  	}
> -	return (part_info.size);
> +	return part_info.size;
>  }
>  
>  
> -int ext2fs_devread (int sector, int byte_offset, int byte_len, char *buf) {
> +int ext2fs_devread(int sector, int byte_offset, int byte_len, char *buf)
> +{
>  	char sec_buf[SECTOR_SIZE];
>  	unsigned block_len;
>  
> -/*
> - *  Check partition boundaries
> - */
> -	if ((sector < 0)
> -	    || ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
> +	/*
> +	 *  Check partition boundaries
> +	 */
> +	if ((sector < 0) ||
> +	    ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
>  		part_info.size)) {
> -	/*      errnum = ERR_OUTSIDE_PART; */
> -		printf (" ** ext2fs_devread() read outside partition sector %d\n", sector);
> -		return (0);
> +		/* errnum = ERR_OUTSIDE_PART; */
> +		printf(" ** %s read outside partition sector %d\n",
> +		       __func__,
> +		       sector);
> +		return 0;
>  	}
>  
> -/*
> - *  Get the read to the beginning of a partition.
> - */
> +	/*
> +	 *  Get the read to the beginning of a partition.
> +	 */
>  	sector += byte_offset >> SECTOR_BITS;
>  	byte_offset &= SECTOR_SIZE - 1;
>  
> -	debug (" <%d, %d, %d>\n", sector, byte_offset, byte_len);
> +	debug(" <%d, %d, %d>\n", sector, byte_offset, byte_len);
>  
>  	if (ext2fs_block_dev_desc == NULL) {
> -		printf ("** Invalid Block Device Descriptor (NULL)\n");
> -		return (0);
> +		printf(" ** %s Invalid Block Device Descriptor (NULL)\n",
> +		       __func__);
> +		return 0;


So in contrast to your commit message you actually change the format of
the output (to the better IMHO).  It would have been better to split the
cleanup and the changes.  Being as it is, you should at least document
the consistency changes for the next round.

Apart from that:

Acked-by: Detlev Zundel <dzu@denx.de>

Cheers
  Detlev

-- 
We support democracy, but that doesn't mean we have to support
governments that get elected as a result of democracy.
                                     -- George W. Bush - March 30, 2006
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Detlev Zundel]

Hi Anton,

> Previously reading or writing zero full sectors (reading the end of
> one sector and the beginning of the next for example) was special
> cased and involved stack allocating a second sector buffer.  This
> change uses the same code path for this case as well as when there
> are a non-zero number of full sectors to access.  The result is
> easier to read and reduces the maximum stack used.

It's non-trivial to prove that your change is equivalent and
unfortunately I do not have enough time to do this.  If your tests work,
than this is certainly a good indication ;) The one thing I'd like to be
sure is that the previous code looks like it used the stack for whole
sectors but copied only parts of this to the provided address pointer.
Your new code looks like it always writes whole sectors to the provided
pointer.  Is this safe even if the caller allocated space without
overhead for whole sectors?

Cheers
  Detlev

-- 
In God we trust.  All others we monitor
                       -- NSA motto
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations

[Anton Staaf]

Ack, you're right, I didn't mean to include the printf changes.  Sorry about
that, I will be more careful with the next patches.  Would it be best to
leave this patch as it is or split it up for the next version (if there is
one)?

Thanks,
    Anton

On Wed, Jun 29, 2011 at 5:13 AM, Detlev Zundel <dzu@denx.de> wrote:

> Hi Anton,
>
> > Fix all checkpatch violations in the low level Ext2 block
> > device reading code.  This is done in preparation for cleaning
> > up the partial sector access code.
> >
> > Signed-off-by: Anton Staaf <robotboy@chromium.org>
> > Cc: Andy Fleming <afleming@freescale.com>
> > ---
> >  fs/ext2/dev.c |   82
> ++++++++++++++++++++++++++++++---------------------------
> >  1 files changed, 43 insertions(+), 39 deletions(-)
> >
> > diff --git a/fs/ext2/dev.c b/fs/ext2/dev.c
> > index 3b49650..4365b3b 100644
> > --- a/fs/ext2/dev.c
> > +++ b/fs/ext2/dev.c
> > @@ -31,7 +31,7 @@
> >  static block_dev_desc_t *ext2fs_block_dev_desc;
> >  static disk_partition_t part_info;
> >
> > -int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int part)
> > +int ext2fs_set_blk_dev(block_dev_desc_t *rbdd, int part)
> >  {
> >       ext2fs_block_dev_desc = rbdd;
> >
> > @@ -46,51 +46,55 @@ int ext2fs_set_blk_dev (block_dev_desc_t * rbdd, int
> part)
> >                       return 0;
> >               }
> >       }
> > -     return (part_info.size);
> > +     return part_info.size;
> >  }
> >
> >
> > -int ext2fs_devread (int sector, int byte_offset, int byte_len, char
> *buf) {
> > +int ext2fs_devread(int sector, int byte_offset, int byte_len, char *buf)
> > +{
> >       char sec_buf[SECTOR_SIZE];
> >       unsigned block_len;
> >
> > -/*
> > - *  Check partition boundaries
> > - */
> > -     if ((sector < 0)
> > -         || ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
> > +     /*
> > +      *  Check partition boundaries
> > +      */
> > +     if ((sector < 0) ||
> > +         ((sector + ((byte_offset + byte_len - 1) >> SECTOR_BITS)) >=
> >               part_info.size)) {
> > -     /*      errnum = ERR_OUTSIDE_PART; */
> > -             printf (" ** ext2fs_devread() read outside partition sector
> %d\n", sector);
> > -             return (0);
> > +             /* errnum = ERR_OUTSIDE_PART; */
> > +             printf(" ** %s read outside partition sector %d\n",
> > +                    __func__,
> > +                    sector);
> > +             return 0;
> >       }
> >
> > -/*
> > - *  Get the read to the beginning of a partition.
> > - */
> > +     /*
> > +      *  Get the read to the beginning of a partition.
> > +      */
> >       sector += byte_offset >> SECTOR_BITS;
> >       byte_offset &= SECTOR_SIZE - 1;
> >
> > -     debug (" <%d, %d, %d>\n", sector, byte_offset, byte_len);
> > +     debug(" <%d, %d, %d>\n", sector, byte_offset, byte_len);
> >
> >       if (ext2fs_block_dev_desc == NULL) {
> > -             printf ("** Invalid Block Device Descriptor (NULL)\n");
> > -             return (0);
> > +             printf(" ** %s Invalid Block Device Descriptor (NULL)\n",
> > +                    __func__);
> > +             return 0;
>
>
> So in contrast to your commit message you actually change the format of
> the output (to the better IMHO).  It would have been better to split the
> cleanup and the changes.  Being as it is, you should at least document
> the consistency changes for the next round.
>
> Apart from that:
>
> Acked-by: Detlev Zundel <dzu@denx.de>
>
> Cheers
>  Detlev
>
> --
> We support democracy, but that doesn't mean we have to support
> governments that get elected as a result of democracy.
>                                     -- George W. Bush - March 30, 2006
> --
> DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
>

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Anton Staaf]

On Wed, Jun 29, 2011 at 6:04 AM, Detlev Zundel <dzu@denx.de> wrote:

> Hi Anton,
>
> > Previously reading or writing zero full sectors (reading the end of
> > one sector and the beginning of the next for example) was special
> > cased and involved stack allocating a second sector buffer.  This
> > change uses the same code path for this case as well as when there
> > are a non-zero number of full sectors to access.  The result is
> > easier to read and reduces the maximum stack used.
>
> It's non-trivial to prove that your change is equivalent and
> unfortunately I do not have enough time to do this.  If your tests work,
> than this is certainly a good indication ;) The one thing I'd like to be
> sure is that the previous code looks like it used the stack for whole
> sectors but copied only parts of this to the provided address pointer.
> Your new code looks like it always writes whole sectors to the provided
> pointer.  Is this safe even if the caller allocated space without
> overhead for whole sectors?


Thanks for the reviews by the way.  My new version of the code still bounces
partial sector reads (both at the beginning and end of the range) through a
stack allocated sector buffer.  The portion that is writing directly to the
users buffer is only used for reading the full sectors.  The middle section
(in the "if (sectors > 0)" block) is reading only as many sectors as are
specified by (byte_len / SECTOR_SIZE).  byte_len, buf and sector at this
point in the function have been updated by the first block that deals with
reading the unaligned start of the data (if it exists).

Also, I have tested this code on a Tegra board using ext2ls and ext2load of
a kernel image.

Thanks,
    Anton

Cheers
>  Detlev
>
> --
> In God we trust.  All others we monitor
>                       -- NSA motto
> --
> DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
>

[U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations

[Detlev Zundel]

Hi Anton,

> Ack, you're right, I didn't mean to include the printf changes.  Sorry about
> that, I will be more careful with the next patches.  Would it be best to
> leave this patch as it is or split it up for the next version (if there is
> one)?

IMHO this is a minor issue, so I'm fine with the patch as is if you
include the change in the commit text.

Thanks
  Detlev

-- 
Q: What do you get when you cross an elephant and a banana?
A: |elephant| * |banana| * sin(theta)
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Detlev Zundel]

Hi Anton,

> On Wed, Jun 29, 2011 at 6:04 AM, Detlev Zundel <dzu@denx.de> wrote:
>
>> Hi Anton,
>>
>> > Previously reading or writing zero full sectors (reading the end of
>> > one sector and the beginning of the next for example) was special
>> > cased and involved stack allocating a second sector buffer.  This
>> > change uses the same code path for this case as well as when there
>> > are a non-zero number of full sectors to access.  The result is
>> > easier to read and reduces the maximum stack used.
>>
>> It's non-trivial to prove that your change is equivalent and
>> unfortunately I do not have enough time to do this.  If your tests work,
>> than this is certainly a good indication ;) The one thing I'd like to be
>> sure is that the previous code looks like it used the stack for whole
>> sectors but copied only parts of this to the provided address pointer.
>> Your new code looks like it always writes whole sectors to the provided
>> pointer.  Is this safe even if the caller allocated space without
>> overhead for whole sectors?
>
>
> Thanks for the reviews by the way.  My new version of the code still bounces
> partial sector reads (both at the beginning and end of the range) through a
> stack allocated sector buffer.  The portion that is writing directly to the
> users buffer is only used for reading the full sectors.  The middle section
> (in the "if (sectors > 0)" block) is reading only as many sectors as are
> specified by (byte_len / SECTOR_SIZE).  byte_len, buf and sector at this
> point in the function have been updated by the first block that deals with
> reading the unaligned start of the data (if it exists).
>
> Also, I have tested this code on a Tegra board using ext2ls and ext2load of
> a kernel image.

Thanks for the explanation.  The new code certainly reads cleaner so

Acked-by: Detlev Zundel <dzu@denx.de>

Thanks
  Detlev

-- 
The management question  ...  is not  _whether_  to build a pilot system
and throw it away.  You _will_ do that.  The only question is whether to
plan  in advance  to build  a throwaway,  or  to promise  to deliver the
throwaway to customers.          - Fred Brooks, "The Mythical Man Month"
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Anton Staaf]

Just checking, will this will make it into the next release?

Thanks,
    Anton

On Thu, Jun 30, 2011 at 7:34 AM, Detlev Zundel <dzu@denx.de> wrote:
> Hi Anton,
>
>> On Wed, Jun 29, 2011 at 6:04 AM, Detlev Zundel <dzu@denx.de> wrote:
>>
>>> Hi Anton,
>>>
>>> > Previously reading or writing zero full sectors (reading the end of
>>> > one sector and the beginning of the next for example) was special
>>> > cased and involved stack allocating a second sector buffer. ?This
>>> > change uses the same code path for this case as well as when there
>>> > are a non-zero number of full sectors to access. ?The result is
>>> > easier to read and reduces the maximum stack used.
>>>
>>> It's non-trivial to prove that your change is equivalent and
>>> unfortunately I do not have enough time to do this. ?If your tests work,
>>> than this is certainly a good indication ;) The one thing I'd like to be
>>> sure is that the previous code looks like it used the stack for whole
>>> sectors but copied only parts of this to the provided address pointer.
>>> Your new code looks like it always writes whole sectors to the provided
>>> pointer. ?Is this safe even if the caller allocated space without
>>> overhead for whole sectors?
>>
>>
>> Thanks for the reviews by the way. ?My new version of the code still bounces
>> partial sector reads (both at the beginning and end of the range) through a
>> stack allocated sector buffer. ?The portion that is writing directly to the
>> users buffer is only used for reading the full sectors. ?The middle section
>> (in the "if (sectors > 0)" block) is reading only as many sectors as are
>> specified by (byte_len / SECTOR_SIZE). ?byte_len, buf and sector at this
>> point in the function have been updated by the first block that deals with
>> reading the unaligned start of the data (if it exists).
>>
>> Also, I have tested this code on a Tegra board using ext2ls and ext2load of
>> a kernel image.
>
> Thanks for the explanation. ?The new code certainly reads cleaner so
>
> Acked-by: Detlev Zundel <dzu@denx.de>
>
> Thanks
> ?Detlev
>
> --
> The management question ?... ?is not ?_whether_ ?to build a pilot system
> and throw it away. ?You _will_ do that. ?The only question is whether to
> plan ?in advance ?to build ?a throwaway, ?or ?to promise ?to deliver the
> throwaway to customers. ? ? ? ? ?- Fred Brooks, "The Mythical Man Month"
> --
> DENX Software Engineering GmbH, ? ? ?MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, ?Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
>

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Wolfgang Denk]

Dear Anton Staaf,

In message <CAF6FioV8Ce9UgW3O3KBnTXg6tPSTZRQ4wjehH=vcL3WU40h56A@mail.gmail.com> you wrote:
> Just checking, will this will make it into the next release?

I'm still waiting for a resubmit of patch 1/2 as requested by Detlev:

06/30 Detlev Zundel      Re: [U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations
             http://article.gmane.org/gmane.comp.boot-loaders.u-boot/102360


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
"You ain't experienced..." "Well, nor are you." "That's true. But the
point is ... the point is ... the point is we've been not experienced
for a lot longer than you. We've got  a  lot  of  experience  of  not
having any experience."           - Terry Pratchett, _Witches Abroad_

[U-Boot] [PATCH 2/2] ext2: Simplify partial sector access logic

[Anton Staaf]

How odd, I have the thread where I sent out the second version of the
patch and even have Detlev's Ack to it (the u-boot list is in the list
of recipients), but it isn't reflected on gmane or the u-boot list
archives.  I'll send it again and keep an eye on the list.

Thanks,
    Anton

On Mon, Jul 25, 2011 at 3:03 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Anton Staaf,
>
> In message <CAF6FioV8Ce9UgW3O3KBnTXg6tPSTZRQ4wjehH=vcL3WU40h56A@mail.gmail.com> you wrote:
>> Just checking, will this will make it into the next release?
>
> I'm still waiting for a resubmit of patch 1/2 as requested by Detlev:
>
> 06/30 Detlev Zundel ? ? ?Re: [U-Boot] [PATCH 1/2] ext2: Fix checkpatch violations
> ? ? ? ? ? ? http://article.gmane.org/gmane.comp.boot-loaders.u-boot/102360
>
>
> 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
> "You ain't experienced..." "Well, nor are you." "That's true. But the
> point is ... the point is ... the point is we've been not experienced
> for a lot longer than you. We've got ?a ?lot ?of ?experience ?of ?not
> having any experience." ? ? ? ? ? - Terry Pratchett, _Witches Abroad_
>