FAT: Simplify get_contents

PATCH Superseded Benoît Thébaudeau 3 participants 10 messages Jul 19, 2012 – Sep 5, 2012 VFAT/FAT/msdos filesystem

lore

Share & tools

lore

Benoît Thébaudeau

author

2012-07-19 22:02 UTC #0 lore raw

PATCH 6/7 FAT: Simplify get_contents

One call to get_cluster can be factorized with another, so avoid duplicating code.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
 .../fs/fat/fat.c                                   |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

fs/fat/fat.c +1 ~~-13~~

index 4c80820..8ac8b85 100644
@@ -368,21 +368,9 @@ get_contents (fsdata *mydata, dir_entry *dentptr, __u8 *buffer,
 			actsize += bytesperclust;
 		}
 
-		/* actsize >= file size */
-		actsize -= bytesperclust;
-
-		/* get remaining clusters */
-		if (get_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
-			printf("Error reading cluster\n");
-			return -1;
-		}
-
 		/* get remaining bytes */
-		gotsize += (int)actsize;
-		filesize -= actsize;
-		buffer += actsize;
 		actsize = filesize;
-		if (get_cluster(mydata, endclust, buffer, (int)actsize) != 0) {
+		if (get_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
 			printf("Error reading cluster\n");
 			return -1;
 		}

Benoît Thébaudeau

author

2012-07-20 13:21 UTC #1 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

One call to get_cluster can be factorized with another, so avoid duplicating code.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.

 .../fs/fat/fat.c                                   |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

fs/fat/fat.c +1 ~~-13~~

index 101eb3a..19f6a8c 100644
@@ -367,21 +367,9 @@ get_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer,
 			actsize += bytesperclust;
 		}
 
-		/* actsize >= file size */
-		actsize -= bytesperclust;
-
-		/* get remaining clusters */
-		if (get_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
-			printf("Error reading cluster\n");
-			return -1;
-		}
-
 		/* get remaining bytes */
-		gotsize += (int)actsize;
-		filesize -= actsize;
-		buffer += actsize;
 		actsize = filesize;
-		if (get_cluster(mydata, endclust, buffer, (int)actsize) != 0) {
+		if (get_cluster(mydata, curclust, buffer, (int)actsize) != 0) {
 			printf("Error reading cluster\n");
 			return -1;
 		}

Wolfgang Denk

Applied

2012-09-02 15:25 UTC #2 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

Dear Beno?t Th?baudeau,

Quoted 5 lines · In message <[email protected]> you…

One call to get_cluster can be factorized with another, so avoid duplicatin> g code.
Signed-off-by Beno?t Th?baudeau <[email protected]>
Cc Wolfgang Denk <[email protected]>

Quoted 7 lines

---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
 .../fs/fat/fat.c                                   |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Signature · 7 lines

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

There were meetings. There were always meetings. And they were  dull,
which is part of the reason they were meetings. Dull likes company.
                                    - Terry Pratchett, _Making_Money_

OK, this change is NOT equivalent code. My platforms now hang thusly (with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64

This is all fine in full U-Boot.

Signature · 1 line

Tom

Benoît Thébaudeau

author

2012-09-04 22:07 UTC #4 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

Hi Tom,

Quoted 44 lines · Tom Rini

Quoted 17 lines · Wolfgang Denk
Dear Beno??t Th??baudeau,
In message <[email protected]> you
Quoted 12 lines · wrote:
One call to get_cluster can be factorized with another, so avoid duplicatin> g
code.
Signed-off-by: Beno??t Th??baudeau <[email protected]>
Cc: Wolfgang Denk <[email protected]>
---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
Quoted 5 lines
Quoted 3 lines
 .../fs/fat/fat.c                                   |   14
 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
Applied, thanks.
OK, this change is NOT equivalent code. My platforms now hang thusly (with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64

Quoted 1 line

This is all fine in full U-Boot.

OK. I'm looking into it.

Can you give more details, like the type of storage (usb, mmc, etc.)? Do you have a command line and a disk image that could be used to duplicate the issue?

Best regards,
Beno?t

Tom Rini

2012-09-04 22:10 UTC #5 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

Quoted 47 lines · Beno?t Th?baudeau

Hi Tom,
Quoted 44 lines · Tom Rini
Quoted 17 lines · Wolfgang Denk
Dear Beno??t Th??baudeau,
In message <[email protected]> you
Quoted 12 lines · wrote:
One call to get_cluster can be factorized with another, so avoid duplicatin> g
code.
Signed-off-by: Beno??t Th??baudeau <[email protected]>
Cc: Wolfgang Denk <[email protected]>
---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
Quoted 5 lines
Quoted 3 lines
 .../fs/fat/fat.c                                   |   14
 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
Applied, thanks.
OK, this change is NOT equivalent code. My platforms now hang thusly (with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64

Quoted 6 lines

Quoted 1 line
This is all fine in full U-Boot.
OK. I'm looking into it.
Can you give more details, like the type of storage (usb, mmc, etc.)? Do you have a command line and a disk image that could be used to duplicate the issue?

It's an SD card. If you have any "OMAP" platform (beagleboard, beaglebone, pandaboard) or am35x/am37x or similar platforms SPL should hang like that. 72MB partition (or so) on either a 2 or 4GB card. Getting all the way up into U-Boot clears the problem away until power cycle. That last part makes me worried...

Signature · 1 line

Tom

Tom Rini

2012-09-04 22:41 UTC #6 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

Quoted 52 lines · Tom Rini

Quoted 51 lines · Beno?t Th?baudeau
Hi Tom,
Quoted 44 lines · Tom Rini
Quoted 17 lines · Wolfgang Denk
Dear Beno??t Th??baudeau,
In message <[email protected]> you
Quoted 12 lines · wrote:
One call to get_cluster can be factorized with another, so avoid duplicatin> g
code.
Signed-off-by: Beno??t Th??baudeau <[email protected]>
Cc: Wolfgang Denk <[email protected]>
---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
Quoted 5 lines
Quoted 3 lines
 .../fs/fat/fat.c                                   |   14
 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
Applied, thanks.
OK, this change is NOT equivalent code. My platforms now hang thusly (with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64
Quoted 1 line
This is all fine in full U-Boot.
OK. I'm looking into it.

Quoted 8 lines

Quoted 2 lines
Can you give more details, like the type of storage (usb, mmc, etc.)? Do you have a command line and a disk image that could be used to duplicate the issue?
It's an SD card. If you have any "OMAP" platform (beagleboard, beaglebone, pandaboard) or am35x/am37x or similar platforms SPL should hang like that. 72MB partition (or so) on either a 2 or 4GB card. Getting all the way up into U-Boot clears the problem away until power cycle. That last part makes me worried...

OK, this is somehow a 'me' problem it seems. I don't see it on a beagleboard and rebuilding things gives me a different failure now, so something is up. Digging more now, sorry for the noise.

Signature · 1 line

Tom

Benoît Thébaudeau

author

2012-09-04 22:53 UTC #7 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

In reply to Tom Rini

Quoted 56 lines · Tom Rini

Quoted 46 lines · Beno?t Th?baudeau
Quoted 45 lines · Tom Rini
Quoted 17 lines · Wolfgang Denk
Dear Beno??t Th??baudeau,
In message <[email protected]> you
Quoted 12 lines · wrote:
One call to get_cluster can be factorized with another, so avoid duplicatin> g
code.
Signed-off-by: Beno??t Th??baudeau <[email protected]>
Cc: Wolfgang Denk <[email protected]>
---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
Quoted 5 lines
Quoted 3 lines
 .../fs/fat/fat.c                                   |   14
 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
Applied, thanks.
OK, this change is NOT equivalent code. My platforms now hang thusly
(with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64
Quoted 8 lines
Quoted 1 line
This is all fine in full U-Boot.
OK. I'm looking into it.
Can you give more details, like the type of storage (usb, mmc, etc.)? Do you
have a command line and a disk image that could be used to duplicate the issue?

Quoted 4 lines

It's an SD card. If you have any "OMAP" platform (beagleboard, beaglebone, pandaboard) or am35x/am37x or similar platforms SPL should
hang like that.

Unfortunately, I don't have these platforms.

Quoted 4 lines

72MB partition (or so) on either a 2 or 4GB card.
Getting all the way up into U-Boot clears the problem away until power
cycle. That last part makes me worried...

What do you mean by "Getting all the way up into U-Boot"? Is it that your SPL can be aborted and give a command prompt?

The only difference that this specific patch (7/8) introduces in your use case (i.e. a request to read the first 64 bytes of a file of 210896 bytes) is that it removes a spurious call to get_cluster() with a size of 0, which should be transparent.

Can you confirm that your tests bisect to this patch and not to another one in this series?

Can you try to apply locally 5/8 and 8/8 to see if it makes a difference, just in case there would be some dependencies?

Is it OK to send e-mails with attachments on the ML? If so, I can post the full fat.c that I currently use so that you can test with it. I have used it for a couple of years without any issue.

Also, why do you read only 64 bytes from this file? According to your debug trace, this read was successful, and then no error is shown. So, if it hangs, it might be that the read data is corrupted, or that the image is not fully read because of this 64-byte limit.

Best regards,
Beno?t

Tom Rini

2012-09-05 00:03 UTC #8 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

Quoted 83 lines · Beno?t Th?baudeau

Quoted 56 lines · Tom Rini
Quoted 46 lines · Beno?t Th?baudeau
Quoted 45 lines · Tom Rini
Quoted 17 lines · Wolfgang Denk
Dear Beno??t Th??baudeau,
In message <[email protected]> you
Quoted 12 lines · wrote:
One call to get_cluster can be factorized with another, so avoid duplicatin> g
code.
Signed-off-by: Beno??t Th??baudeau <[email protected]>
Cc: Wolfgang Denk <[email protected]>
---
Changes for v2:
- Patch renumbering because of the new v2 1/8.
- Possible code style changes due to the new v2 1/8.
Quoted 5 lines
Quoted 3 lines
 .../fs/fat/fat.c                                   |   14
 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)
Applied, thanks.
OK, this change is NOT equivalent code. My platforms now hang thusly
(with DEBUG set):
reading u-boot.img
VFAT Support enabled
FAT16, fat_sect: 4, fatlength: 144
Rootdir begins at cluster: 0, sector: 292, offset: 24800 Data begins at: 316
Sector size: 512, cluster size: 4
FAT read sect=292, clust_size=4, DIRENTSPERBLOCK=16
Rootvfatname: |u-boot.ais|
RootMismatch: |u-boot.ais|u-boot.ais|
RootMismatch: |u-boot.ais||
RootMismatch: |mlo||
Rootvfatname: |u-boot.img|
RootName: u-boot.img, start: 0xc2, size: 0x337d0
Filesize: 210896 bytes
64 bytes
gc - clustnum: 194, startsect: 1092
Size: 210896, got: 64
Quoted 8 lines
Quoted 1 line
This is all fine in full U-Boot.
OK. I'm looking into it.
Can you give more details, like the type of storage (usb, mmc, etc.)? Do you
have a command line and a disk image that could be used to duplicate the issue?
Quoted 4 lines
It's an SD card. If you have any "OMAP" platform (beagleboard, beaglebone, pandaboard) or am35x/am37x or similar platforms SPL should
hang like that.
Unfortunately, I don't have these platforms.
Quoted 4 lines
72MB partition (or so) on either a 2 or 4GB card.
Getting all the way up into U-Boot clears the problem away until power
cycle. That last part makes me worried...
What do you mean by "Getting all the way up into U-Boot"? Is it that your SPL can be aborted and give a command prompt?
The only difference that this specific patch (7/8) introduces in your use case (i.e. a request to read the first 64 bytes of a file of 210896 bytes) is that it removes a spurious call to get_cluster() with a size of 0, which should be transparent.
Can you confirm that your tests bisect to this patch and not to another one in this series?
Can you try to apply locally 5/8 and 8/8 to see if it makes a difference, just in case there would be some dependencies?

Quoted 8 lines

Is it OK to send e-mails with attachments on the ML? If so, I can post the full fat.c that I currently use so that you can test with it. I have used it for a couple of years without any issue.
Also, why do you read only 64 bytes from this file? According to your debug trace, this read was successful, and then no error is shown. So, if it hangs, it might be that the read data is corrupted, or that the image is not fully read because of this 64-byte limit.

I've bisected this twice and I've come to a conclusion. It is this patch that makes my problem show up, but it's not this patch at fault. What I'm going to go with is that the "am33xx: Remove redundant timer config" patch that I forgot in my last round of pull requests needs to come in as with that set of patches applied locally to top of tree the platform is fine again.

Signature · 1 line

Tom

Wolfgang Denk

2012-09-04 22:08 UTC #9 lore raw

PATCH v2 7/8 FAT: Simplify get_contents

In reply to Tom Rini

Dear Tom,

Quoted 2 lines · In message <20120904205034.GC23991@bill-the-cat> you wrote:

OK, this change is NOT equivalent code. My platforms now hang thusly

argh... :-(

Quoted 1 line

This is all fine in full U-Boot.

What shall we do? Revert? Fix?

Best regards,

Wolfgang Denk

Signature · 4 lines

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
Why is an average signature file longer than an average Perl script??