[U-Boot] [PATCH 0/2] ext2: Cleanup and simplify sector access code
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
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; }
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)) >=
- /* 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.*/
- 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
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@denx.de
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
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 */
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
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@denx.de
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
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@denx.de
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
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@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_
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
participants (4)
-
Anton Staaf -
Anton Staaf -
Detlev Zundel -
Wolfgang Denk