fs: fat: read: fix fat16 ls/read issue

PATCH New Przemyslaw Marczak 3 participants 26 messages Dec 11, 2014 – Jan 7, 2015 VFAT/FAT/msdos filesystem

lore

Share & tools

lore

Przemyslaw Marczak

author

2014-12-11 12:01 UTC #0 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.

This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.

If some file long name entries are stored in two sectors, the we have two cases:

Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!

Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!

This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next

The commit doesn't affects the fat32 imlementation, which works good as previous.

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc Mikhail Zolotaryov <[email protected]>

Cc Tom Rini <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Simon Glass <[email protected]>

Cc Suriyan Ramasami <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
 fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

fs/fat/fat.c +24 -9

index 04a51db..afbf12d 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	int ret = -1;
 	int firsttime;
 	__u32 root_cluster = 0;
+	__u32 read_blk;
 	int rootdir_size = 0;
-	int j;
+	int j, k;
+	int do_read;
+	__u8 *dir_ptr;
 
 	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
 		debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	}
 
 	j = 0;
+	k = 0;
 	while (1) {
 		int i;
 
-		if (j == 0) {
+		if (mydata->fatsize == 32 || !k) {
+			dir_ptr = do_fat_read_at_block;
+			k = 1;
+		} else {
+			dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+			memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+		}
+
+		do_read = 1;
+
+		if (mydata->fatsize == 32 && j)
+			do_read = 0;
+
+		if (do_read) {
 			debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
 				cursect, mydata->clust_size, DIRENTSPERBLOCK);
 
-			if (disk_read(cursect,
-					(mydata->fatsize == 32) ?
-					(mydata->clust_size) :
-					PREFETCH_BLOCKS,
-					do_fat_read_at_block) < 0) {
+			read_blk = (mydata->fatsize == 32) ?
+				    mydata->clust_size : PREFETCH_BLOCKS;
+			if (disk_read(cursect, read_blk, dir_ptr) < 0) {
 				debug("Error: reading rootdir block\n");
 				goto exit;
 			}
 
-			dentptr = (dir_entry *) do_fat_read_at_block;
+			dentptr = (dir_entry *)dir_ptr;
 		}
 
 		for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 					get_vfatname(mydata,
 						     root_cluster,
-						     do_fat_read_at_block,
+						     dir_ptr,
 						     dentptr, l_name);
 
 					if (dols == LS_ROOT) {

Signature · 1 line

1.9.1

Simon Glass

2014-12-12 00:32 UTC #1 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi Przemyslaw,

Quoted 22 lines · Przemyslaw Marczak

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!

Quoted 7 lines

This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.

This is very interesting\! Is this the same failure that I saw on this thread?

http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html

(search for fatload)

"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.

Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "

Also can you write a test for this in test/fs/fs-test.sh?

Regards,
Simon

[snip]

Przemyslaw Marczak

author

2014-12-12 15:30 UTC #2 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 49 lines · Simon Glass

Hi Przemyslaw,
Quoted 22 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "

Quoted 7 lines

Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]

Probably this is an another case which is caused by the sector buffer bug. Does this patch fixed your issue?

I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the UMS, next it computes CRC32 of those files on the host and next using U-Boot fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.

So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the "test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.

Unfortunately the sandbox config seems to be broken.

The bug was not so obvious, any read/write on fat partition can change fat directory entries or add the new ones and then all data can be read right.

I will send the scripts for such simple test.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Przemyslaw Marczak

author

2014-12-12 15:52 UTC #3 lore raw

PATCH 1/1 fat: scripts for prepare and test read fat files

---------------------------------------------------------
mktest_files: script for generating random size long name files
---------------------------------------------------------
Usage:
./1_mktest_files.sh count

count - number of files to be generated

The output directory is: "./test_files"

---------------------------------------------------------
copy_files: copy the test_files/* into test partition mount point
---------------------------------------------------------
Usage:
./2_copy_files.sh mount_point

-----------------------------------------------------------
fat_test.sh: test fat read by write commands to the device console
----------------------------------------------------------
This script send commands to U-Boot console.
First specify few script variables, e.g:
- TTY="/dev/ttyS0"
- MMCDEV=0
- PARTITION=2
- LOAD_ADDR="0x40000000"

usage:
1. Target:
run: ums 0 mmc 0
2. Run script 1 and 2 to make and copy the test files onto the test partition by UMS
3. This script:
- set test device $PARTITION and other variables in the script, which is required for sending proper commands
- set $TTY in the script
run: ./3_fat_test.sh
4. Compare the crc results on the target and device consoles (sorry for the mess on the console)

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 1_mktest_files.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2_copy_files.sh   | 20 ++++++++++++++
 3_fat_test.sh     | 38 ++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)
 create mode 100755 1_mktest_files.sh
 create mode 100755 2_copy_files.sh
 create mode 100755 3_fat_test.sh

1_mktest_files.sh new file +82 -0

new file mode 100755
index 0000000..e9f4e26
@@ -0,0 +1,82 @@
+#!/bin/bash
+#
+# Copyright (C) 2014 Samsung Electronics
+# Przemyslaw Marczak <[email protected]>
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+F_CNT=${1}
+OUT_DIR="./test_files"
+# File max: 100k
+f_max_size=255
+
+# Minimal len of long name -> len+"xxxk.bin"
+LONG_NAME_LEN=20
+
+if [ ${#1} -eq 0 ]
+	then
+	echo "Bad arg!"
+	echo "usage:"
+	echo "./mktest_files.sh count"
+	exit
+else
+	if [ -d $OUT_DIR ]
+		then
+		echo "Directory: \"$OUT_DIR\" exists - cleanup"
+		rm $OUT_DIR/*
+	else
+		echo "Test files directory: \"$OUT_DIR\""
+		mkdir $OUT_DIR
+	fi
+	echo "Generating $1 files:"
+fi
+
+######
+# 1. # Generate random size for the files
+######
+
+i=0
+for val in `rand -M $f_max_size -N $F_CNT -u`
+	do
+	f_size_list[$i]=$val
+#	echo "Size[$i]: ${f_size_list[$i]}"
+	i=$(($i+1))
+done
+
+######
+# 2. #  Prepare the long name
+######
+CHAR="_"
+
+for I in `seq 1 1 $LONG_NAME_LEN`; do
+	LONG_NAME="$LONG_NAME""$CHAR"
+done
+
+######
+# 3. # Generate files with random data
+######
+i=0
+# Set fat16 file extension (dot + 3 characters)
+EXT=".bin"
+for len_k in ${f_size_list[@]}
+	do
+
+	prefix="$len_k""k"
+
+	prefix_len=${#prefix}
+	long_name_len=${#LONG_NAME}
+	ext_len=${#EXT}
+	name=`echo "$prefix""$LONG_NAME" | head -c $long_name_len`
+	name=$name$EXT
+#	echo Name: $name len: ${#name}
+
+	echo "$i Prefix: $prefix name len: ${#name} chars:"
+	full_name="$OUT_DIR/$name"
+
+	echo "$full_name"
+
+	dd if=/dev/urandom of=${full_name} bs=1k count=$len_k 2>/dev/null
+
+	i=$(($i+1))
+done

2_copy_files.sh new file +20 -0

new file mode 100755
index 0000000..6bfefc6
@@ -0,0 +1,20 @@
+#!/bin/bash
+#
+# Copyright (C) 2014 Samsung Electronics
+# Przemyslaw Marczak <[email protected]>
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+#MOUNT_POINT - for the fat16 partition,
+#              e.g. target partition using ums command
+
+MOUNT_POINT=$1
+DIR="test_files"
+
+for I in `ls -1 -S $DIR/`; do
+	echo Copying "$DIR/$I" to $DEV
+	cp "$DIR/$I" $MOUNT_POINT
+done
+
+sync
\ No newline at end of file

3_fat_test.sh new file +38 -0

new file mode 100755
index 0000000..07222a0
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# Copyright (C) 2014 Samsung Electronics
+# Przemyslaw Marczak <[email protected]>
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+TTY="/dev/ttyS0"
+MMCDEV=0
+PARTITION=2
+LOAD_ADDR="0x40000000"
+
+DIR="test_files"
+
+SPACE="_____________________________ "
+CMD_1_2="setenv t \"fatload mmc $MMCDEV:$PARTITION $LOAD_ADDR '\$F';"
+CMD_2_2="echo $SPACE File: '\$F'; crc $LOAD_ADDR '\$S'\""
+CMD="$CMD_1_2""$CMD_2_2"
+
+# Set command for loading DIR
+echo "$CMD" > $TTY
+
+for I in `ls -1 -S $DIR`;
+	do
+	FILE="$DIR"/"$I"
+	# Hex size:
+	SIZE=`ls -l $FILE | awk '{printf("%#x\n", $5)}'`
+	CRC=`crc32 $FILE`
+
+	echo
+	echo "File: $I"
+	echo "  ==> $CRC"
+
+	CMD="setenv F \'$I\'; setenv S $SIZE; run t;"
+	echo "echo" > $TTY
+	echo $CMD > $TTY
+done

Signature · 1 line

1.9.1

Przemyslaw Marczak

author

2014-12-12 15:54 UTC #4 lore raw

PATCH 1/1 fat: scripts for prepare and test read fat files

Hello,

Quoted 40 lines · Przemyslaw Marczak

---------------------------------------------------------
mktest_files: script for generating random size long name files
---------------------------------------------------------
Usage:
./1_mktest_files.sh count
count - number of files to be generated
The output directory is: "./test_files"
---------------------------------------------------------
copy_files: copy the test_files/* into test partition mount point
---------------------------------------------------------
Usage:
./2_copy_files.sh mount_point
-----------------------------------------------------------
fat_test.sh: test fat read by write commands to the device console
----------------------------------------------------------
This script send commands to U-Boot console.
First specify few script variables, e.g:
- TTY="/dev/ttyS0"
- MMCDEV=0
- PARTITION=2
- LOAD_ADDR="0x40000000"
usage:
1. Target:
run: ums 0 mmc 0
2. Run script 1 and 2 to make and copy the test files onto the test partition by UMS
3. This script:
- set test device $PARTITION and other variables in the script, which is required for sending proper commands
- set $TTY in the script
run: ./3_fat_test.sh
4. Compare the crc results on the target and device consoles (sorry for the mess on the console)
Signed-off-by Przemyslaw Marczak <[email protected]>

Quoted 9 lines

---
  1_mktest_files.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  2_copy_files.sh   | 20 ++++++++++++++
  3_fat_test.sh     | 38 ++++++++++++++++++++++++++
  3 files changed, 140 insertions(+)
create mode 100755 1_mktest_files.sh
create mode 100755 2_copy_files.sh
create mode 100755 3_fat_test.sh

This is just for some quick test.
I will add something more pretty to the sandbox.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-16 20:41 UTC #5 lore raw

PATCH 1/1 fat: scripts for prepare and test read fat files

Hi Przemyslaw,

Quoted 56 lines · Przemyslaw Marczak

Hello,
Quoted 41 lines · Przemyslaw Marczak
---------------------------------------------------------
mktest_files: script for generating random size long name files
---------------------------------------------------------
Usage:
./1_mktest_files.sh count
count - number of files to be generated
The output directory is: "./test_files"
---------------------------------------------------------
copy_files: copy the test_files/* into test partition mount point
---------------------------------------------------------
Usage:
./2_copy_files.sh mount_point
-----------------------------------------------------------
fat_test.sh: test fat read by write commands to the device console
----------------------------------------------------------
This script send commands to U-Boot console.
First specify few script variables, e.g:
- TTY="/dev/ttyS0"
- MMCDEV=0
- PARTITION=2
- LOAD_ADDR="0x40000000"
usage:
1. Target:
run: ums 0 mmc 0
2. Run script 1 and 2 to make and copy the test files onto the test partition by UMS
3. This script:
- set test device $PARTITION and other variables in the script, which is required for sending proper commands
- set $TTY in the script
run: ./3_fat_test.sh
4. Compare the crc results on the target and device consoles (sorry for the mess on the console)
Signed-off-by Przemyslaw Marczak <[email protected]>
Quoted 9 lines
---
  1_mktest_files.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  2_copy_files.sh   | 20 ++++++++++++++
  3_fat_test.sh     | 38 ++++++++++++++++++++++++++
  3 files changed, 140 insertions(+)
create mode 100755 1_mktest_files.sh
create mode 100755 2_copy_files.sh
create mode 100755 3_fat_test.sh

Quoted 2 lines

This is just for some quick test.
I will add something more pretty to the sandbox.

Perhaps this should be written in Python? We now have quite a few tests and it's getting to the point where we might want to have a way to run them all, check results, etc. That would be easier if we could import them through some standard interface. For now, perhaps we should avoid shell scripts except for really trivial things.

You can bring in the patman libraries (we could break these out into another dir but it doesn't seem important):

import os
import sys

# Bring in the patman libraries
our_path = os.path.dirname(os.path.realpath(__file__)) sys.path.append(os.path.join(our_path, 'tools/patman'))

import command

The, for example:
print command.Output('ls', '-l')

Regards,
Simon

Przemyslaw Marczak

author

2014-12-17 08:53 UTC #6 lore raw

PATCH 1/1 fat: scripts for prepare and test read fat files

Hello Simon,

Quoted 78 lines · Simon Glass

Hi Przemyslaw,
Quoted 56 lines · Przemyslaw Marczak
Hello,
Quoted 41 lines · Przemyslaw Marczak
---------------------------------------------------------
mktest_files: script for generating random size long name files
---------------------------------------------------------
Usage:
./1_mktest_files.sh count
count - number of files to be generated
The output directory is: "./test_files"
---------------------------------------------------------
copy_files: copy the test_files/* into test partition mount point
---------------------------------------------------------
Usage:
./2_copy_files.sh mount_point
-----------------------------------------------------------
fat_test.sh: test fat read by write commands to the device console
----------------------------------------------------------
This script send commands to U-Boot console.
First specify few script variables, e.g:
- TTY="/dev/ttyS0"
- MMCDEV=0
- PARTITION=2
- LOAD_ADDR="0x40000000"
usage:
1. Target:
run: ums 0 mmc 0
2. Run script 1 and 2 to make and copy the test files onto the test partition by UMS
3. This script:
- set test device $PARTITION and other variables in the script, which is required for sending proper commands
- set $TTY in the script
run: ./3_fat_test.sh
4. Compare the crc results on the target and device consoles (sorry for the mess on the console)
Signed-off-by Przemyslaw Marczak <[email protected]>
Quoted 9 lines
---
   1_mktest_files.sh | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
   2_copy_files.sh   | 20 ++++++++++++++
   3_fat_test.sh     | 38 ++++++++++++++++++++++++++
   3 files changed, 140 insertions(+)
create mode 100755 1_mktest_files.sh
create mode 100755 2_copy_files.sh
create mode 100755 3_fat_test.sh
Quoted 2 lines
This is just for some quick test.
I will add something more pretty to the sandbox.
Perhaps this should be written in Python? We now have quite a few tests and it's getting to the point where we might want to have a way to run them all, check results, etc. That would be easier if we could import them through some standard interface. For now, perhaps we should avoid shell scripts except for really trivial things.
You can bring in the patman libraries (we could break these out into another dir but it doesn't seem important):
import os
import sys
# Bring in the patman libraries
our_path = os.path.dirname(os.path.realpath(__file__)) sys.path.append(os.path.join(our_path, 'tools/patman'))

Quoted 8 lines

import command
The, for example:
print command.Output('ls', '-l')
Regards,
Simon

The patch fixes the issue with the "hidden" files, so for looking the issue on some other cases I made some simple script. I understand that this is not good for a U-Boot tests, and I agree that better is to write something automated in the Python for the sandbox.
I think that the fix should be merged.
I can write the test for the sandbox in a free time, but it will take a moment, because now I would like to focus on the pmic framework. The pmic was on hold for too long.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-16 22:26 UTC #7 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

In reply to Przemyslaw Marczak

Hi Przemyslaw,

Quoted 82 lines · Przemyslaw Marczak

Hello,
Quoted 55 lines · Simon Glass
Hi Przemyslaw,
Quoted 23 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug. Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the UMS, next it computes CRC32 of those files on the host and next using U-Boot fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the "test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.

Quoted 6 lines

Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.

I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.

If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.

Regards,
Simon

Przemyslaw Marczak

author

2014-12-17 08:53 UTC #8 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 95 lines · Simon Glass

Hi Przemyslaw,
Quoted 82 lines · Przemyslaw Marczak
Hello,
Quoted 55 lines · Simon Glass
Hi Przemyslaw,
Quoted 23 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug. Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the UMS, next it computes CRC32 of those files on the host and next using U-Boot fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the "test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.
Quoted 6 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.

Quoted 8 lines

If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Regards,
Simon

It sounds good. I can do that, as I wrote in my previous message, now I'm focused on the pmic, and this will be a side task for a break time. I will look into the present tests and I think, that I will find an example there.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Przemyslaw Marczak

author

2014-12-17 09:03 UTC #9 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

In reply to Simon Glass

Hello,

Quoted 95 lines · Simon Glass

Hi Przemyslaw,
Quoted 82 lines · Przemyslaw Marczak
Hello,
Quoted 55 lines · Simon Glass
Hi Przemyslaw,
Quoted 23 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug. Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the UMS, next it computes CRC32 of those files on the host and next using U-Boot fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the "test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.
Quoted 6 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.

Quoted 8 lines

If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Regards,
Simon

Before, I wrote, that sandbox seems to be broken, sorry for this - it was just my dirty repo - sandbox compiles and works well.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 03:39 UTC #10 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi Przemyslaw,

Quoted 118 lines · Przemyslaw Marczak

Hello,
Quoted 114 lines · Simon Glass
Hi Przemyslaw,
Quoted 91 lines · Przemyslaw Marczak
Hello,
Quoted 59 lines · Simon Glass
Hi Przemyslaw,
Quoted 24 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.
Quoted 8 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.

Quoted 6 lines

Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was just my dirty repo - sandbox compiles and works well.

Somewhat bewildering, but it does not in fact fix my problem.

Here is a compressed version of the fat filesystem if you want to take a look:

https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 10:26 UTC #11 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello Simon,

Quoted 128 lines · Simon Glass

Hi Przemyslaw,
Quoted 118 lines · Przemyslaw Marczak
Hello,
Quoted 114 lines · Simon Glass
Hi Przemyslaw,
Quoted 91 lines · Przemyslaw Marczak
Hello,
Quoted 59 lines · Simon Glass
Hi Przemyslaw,
Quoted 24 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this: echo "some cmd" > /dev/ttyS0.
Quoted 8 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Quoted 6 lines
Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was just my dirty repo - sandbox compiles and works well.

Quoted 9 lines

Somewhat bewildering, but it does not in fact fix my problem.
Here is a compressed version of the fat filesystem if you want to take a look:
https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing
Regards,
Simon

I had put this image on my Trats2 device on partition mmc 0:6 using ums and dd linux command. Next I put latest mainline u-boot, commit:
e3bf81b1e841ecabe7c8b3d48621256db8b8623e : "Merge branch 'master' of
git://git.denx.de/u-boot-mpc85xx"

So this is the version with the fat bug. But I can see and load the
file: "bcm2835-rpi-b-rev2.dtb".

Trats2 # fatls mmc 0:6

     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb

18 file(s), 1 dir(s)

The only missing file is "boot.scr", it's ignored by "ls" command but can be loaded...

Trats2 # fatload mmc 0:6 0x40000000 boot.scr
reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3

I suppose that the partition image which you shared for me is little different, than this mentioned in the topic "[PATCH U-Boot] ARM: rpi_b:
detect board revision"

Probably the sequence of writing files to this partition was different, and the different file is ignored.

After putting the debug macro on the top of fs/fat/fat.c:

Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16

     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb

END LOOP: j=0 clust_size=16
RootDentname == NULL - 0

18 file(s), 1 dir(s)

And next test on commit 9b416a9f4ca7cf5ac4d5f7143d67edde7f7d7326 with my fat patch.

Trats2 # fatls mmc 0:6

     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb

19 file(s), 1 dir(s)

Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 # fatload mmc 0:6 0x40000000 boot.scr reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3

So the only difference on this image is, that with my patch, the file "boot.scr" ignored by ls command is now visible - but in both cases it can be loaded into memory and the crc is correct.

After enabling the debug:

     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
FAT read sect=81, clust_size=16, DIRENTSPERBLOCK=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb

END LOOP: j=0 clust_size=16
FAT read sect=83, clust_size=16, DIRENTSPERBLOCK=16 RootDentname == NULL - 0

19 file(s), 1 dir(s)

So as I checked the file:
256 boot.scr
is next behind to the:
1331 config.txt~

And this can be checked with hex dump:
hd -s 0xa400 -n 512 bad-fat.dd

Your fat image is good example of what my patch fixes.

As you can see on the simple debug info, without the fix,the sector 80 and 81 is stored in the buffer (there are always 2 sectors in the buffer). If you see the hex dump of the second sector:

hd -s 0xa200 -n 512 bad-fat.dd

You will see that at the end of this sector, there is a long name entry for file "boot.scr".

In the next loop (without the fix):
END LOOP: j=1 clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
extlinux/
the sector 82 and 83 is loaded in to the buffer, so the long name entry of "boot.scr" file from the end of sector 81 is now in the heaven, and the file will be ignored by the ls command.

The sector 82 can be checked by:
hd -s 0xa400 -n 512 bad-fat.dd

It begins with the short name entry of file "boot.scr".

After applying my fix, there are always three sectors in the buffer, because the last one is moved into the buffer begin and two next are loaded into the buffer next to the last one.
And the buffer pointer is always on the second buffered sector beside first loop.

So I think this patch fixes the issue well.

Could you describe your issue more precisely?

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 13:14 UTC #12 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi Przemyslaw,

Quoted 366 lines · Przemyslaw Marczak

Hello Simon,
Quoted 143 lines · Simon Glass
Hi Przemyslaw,
Quoted 128 lines · Przemyslaw Marczak
Hello,
Quoted 123 lines · Simon Glass
Hi Przemyslaw,
Quoted 98 lines · Przemyslaw Marczak
Hello,
Quoted 64 lines · Simon Glass
Hi Przemyslaw,
Quoted 25 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this:
echo
"some cmd" > /dev/ttyS0.
Quoted 8 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Quoted 6 lines
Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was just my dirty repo - sandbox compiles and works well.
Somewhat bewildering, but it does not in fact fix my problem.
Quoted 9 lines
Here is a compressed version of the fat filesystem if you want to take a look:
https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing
Regards,
Simon
I had put this image on my Trats2 device on partition mmc 0:6 using ums and dd linux command. Next I put latest mainline u-boot, commit:
e3bf81b1e841ecabe7c8b3d48621256db8b8623e : "Merge branch 'master' of
git://git.denx.de/u-boot-mpc85xx"
So this is the version with the fat bug. But I can see and load the file:
"bcm2835-rpi-b-rev2.dtb".
Trats2 # fatls mmc 0:6
    17840   bootcode.bin
      120   cmdline.txt
     1331   config.txt
     6115   fixup.dat
     2324   fixup_cd.dat
     9166   fixup_x.dat
  3232856   kernel.img
  2615064   start.elf
   533080   start_cd.elf
  3572200   start_x.elf
      137   issue.txt
    18974   license.oracle
   295524   u-boot.bin
     1331   config.txt~
            extlinux/
  3368648   zimage
     3963   bcm2835-rpi-b.dtb
     3963   bcm2835.dtb
     3963   bcm2835-rpi-b-rev2.dtb
18 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 #
The only missing file is "boot.scr", it's ignored by "ls" command but can be loaded...
Trats2 # fatload mmc 0:6 0x40000000 boot.scr
reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
I suppose that the partition image which you shared for me is little different, than this mentioned in the topic "[PATCH U-Boot] ARM: rpi_b:
detect board revision"
Probably the sequence of writing files to this partition was different, and the different file is ignored.
After putting the debug macro on the top of fs/fat/fat.c:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
    17840   bootcode.bin
      120   cmdline.txt
     1331   config.txt
     6115   fixup.dat
     2324   fixup_cd.dat
     9166   fixup_x.dat
  3232856   kernel.img
  2615064   start.elf
END LOOP: j=0   clust_size=16
   533080   start_cd.elf
  3572200   start_x.elf
      137   issue.txt
    18974   license.oracle
   295524   u-boot.bin
     1331   config.txt~
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
            extlinux/
  3368648   zimage
     3963   bcm2835-rpi-b.dtb
     3963   bcm2835.dtb
     3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
RootDentname == NULL - 0
18 file(s), 1 dir(s)
And next test on commit 9b416a9f4ca7cf5ac4d5f7143d67edde7f7d7326 with my fat patch.
Trats2 # fatls mmc 0:6
    17840   bootcode.bin
      120   cmdline.txt
     1331   config.txt
     6115   fixup.dat
     2324   fixup_cd.dat
     9166   fixup_x.dat
  3232856   kernel.img
  2615064   start.elf
   533080   start_cd.elf
  3572200   start_x.elf
      137   issue.txt
    18974   license.oracle
   295524   u-boot.bin
     1331   config.txt~
      256   boot.scr
            extlinux/
  3368648   zimage
     3963   bcm2835-rpi-b.dtb
     3963   bcm2835.dtb
     3963   bcm2835-rpi-b-rev2.dtb
19 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 # fatload mmc 0:6 0x40000000 boot.scr reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
So the only difference on this image is, that with my patch, the file "boot.scr" ignored by ls command is now visible - but in both cases it can be loaded into memory and the crc is correct.
After enabling the debug:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
    17840   bootcode.bin
      120   cmdline.txt
     1331   config.txt
     6115   fixup.dat
     2324   fixup_cd.dat
     9166   fixup_x.dat
  3232856   kernel.img
  2615064   start.elf
END LOOP: j=0   clust_size=16
FAT read sect=81, clust_size=16, DIRENTSPERBLOCK=16
   533080   start_cd.elf
  3572200   start_x.elf
      137   issue.txt
    18974   license.oracle
   295524   u-boot.bin
     1331   config.txt~
      256   boot.scr
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
            extlinux/
  3368648   zimage
     3963   bcm2835-rpi-b.dtb
     3963   bcm2835.dtb
     3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
FAT read sect=83, clust_size=16, DIRENTSPERBLOCK=16 RootDentname == NULL - 0
19 file(s), 1 dir(s)
So as I checked the file:
256 boot.scr
is next behind to the:
1331 config.txt~
And this can be checked with hex dump:
hd -s 0xa400 -n 512 bad-fat.dd
Your fat image is good example of what my patch fixes.
As you can see on the simple debug info, without the fix,the sector 80 and 81 is stored in the buffer (there are always 2 sectors in the buffer). If you see the hex dump of the second sector:
hd -s 0xa200 -n 512 bad-fat.dd
You will see that at the end of this sector, there is a long name entry for file "boot.scr".
In the next loop (without the fix):
END LOOP: j=1 clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
extlinux/
the sector 82 and 83 is loaded in to the buffer, so the long name entry of "boot.scr" file from the end of sector 81 is now in the heaven, and the file will be ignored by the ls command.
The sector 82 can be checked by:
hd -s 0xa400 -n 512 bad-fat.dd
It begins with the short name entry of file "boot.scr".
After applying my fix, there are always three sectors in the buffer, because the last one is moved into the buffer begin and two next are loaded into the buffer next to the last one.
And the buffer pointer is always on the second buffered sector beside first loop.

Quoted 3 lines

So I think this patch fixes the issue well.
Could you describe your issue more precisely?

I think you left out the path. The file I tried to load was:

/syslinux/..//bcm2835-rpi-b-rev2.dtb

It works OK without the path on the front.

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 13:31 UTC #13 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 373 lines · Simon Glass

Hi Przemyslaw,
Quoted 366 lines · Przemyslaw Marczak
Hello Simon,
Quoted 143 lines · Simon Glass
Hi Przemyslaw,
Quoted 128 lines · Przemyslaw Marczak
Hello,
Quoted 123 lines · Simon Glass
Hi Przemyslaw,
Quoted 98 lines · Przemyslaw Marczak
Hello,
Quoted 64 lines · Simon Glass
Hi Przemyslaw,
Quoted 25 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool.
It just copy the test files to the tested fat16 partition mounted using the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were found on the partiion, by the U-Boot read command. It's not useful for the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this:
echo
"some cmd" > /dev/ttyS0.
Quoted 8 lines
Unfortunately the sandbox config seems to be broken.
The bug was not so obvious, any read/write on fat partition can change fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Quoted 6 lines
Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was just my dirty repo - sandbox compiles and works well.
Somewhat bewildering, but it does not in fact fix my problem.
Quoted 9 lines
Here is a compressed version of the fat filesystem if you want to take a look:
https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing
Regards,
Simon
I had put this image on my Trats2 device on partition mmc 0:6 using ums and dd linux command. Next I put latest mainline u-boot, commit:
e3bf81b1e841ecabe7c8b3d48621256db8b8623e : "Merge branch 'master' of
git://git.denx.de/u-boot-mpc85xx"
So this is the version with the fat bug. But I can see and load the file:
"bcm2835-rpi-b-rev2.dtb".
Trats2 # fatls mmc 0:6
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
18 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 #
The only missing file is "boot.scr", it's ignored by "ls" command but can be loaded...
Trats2 # fatload mmc 0:6 0x40000000 boot.scr
reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
I suppose that the partition image which you shared for me is little different, than this mentioned in the topic "[PATCH U-Boot] ARM: rpi_b:
detect board revision"
Probably the sequence of writing files to this partition was different, and the different file is ignored.
After putting the debug macro on the top of fs/fat/fat.c:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
RootDentname == NULL - 0
18 file(s), 1 dir(s)
And next test on commit 9b416a9f4ca7cf5ac4d5f7143d67edde7f7d7326 with my fat patch.
Trats2 # fatls mmc 0:6
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
19 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 # fatload mmc 0:6 0x40000000 boot.scr reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
So the only difference on this image is, that with my patch, the file "boot.scr" ignored by ls command is now visible - but in both cases it can be loaded into memory and the crc is correct.
After enabling the debug:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
FAT read sect=81, clust_size=16, DIRENTSPERBLOCK=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
FAT read sect=83, clust_size=16, DIRENTSPERBLOCK=16 RootDentname == NULL - 0
19 file(s), 1 dir(s)
So as I checked the file:
256 boot.scr
is next behind to the:
1331 config.txt~
And this can be checked with hex dump:
hd -s 0xa400 -n 512 bad-fat.dd
Your fat image is good example of what my patch fixes.
As you can see on the simple debug info, without the fix,the sector 80 and 81 is stored in the buffer (there are always 2 sectors in the buffer). If you see the hex dump of the second sector:
hd -s 0xa200 -n 512 bad-fat.dd
You will see that at the end of this sector, there is a long name entry for file "boot.scr".
In the next loop (without the fix):
END LOOP: j=1 clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
extlinux/
the sector 82 and 83 is loaded in to the buffer, so the long name entry of "boot.scr" file from the end of sector 81 is now in the heaven, and the file will be ignored by the ls command.
The sector 82 can be checked by:
hd -s 0xa400 -n 512 bad-fat.dd
It begins with the short name entry of file "boot.scr".
After applying my fix, there are always three sectors in the buffer, because the last one is moved into the buffer begin and two next are loaded into the buffer next to the last one.
And the buffer pointer is always on the second buffered sector beside first loop.
Quoted 3 lines
So I think this patch fixes the issue well.
Could you describe your issue more precisely?

Quoted 9 lines

I think you left out the path. The file I tried to load was:
/syslinux/..//bcm2835-rpi-b-rev2.dtb
It works OK without the path on the front.
Regards,
Simon

Yes I didn't use any path.
But why are you using such path, if there is no such directory? There is only /extlinux directory on the fat image which you shared.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 13:36 UTC #14 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi Przemyslaw,

Quoted 418 lines · Przemyslaw Marczak

Hello,
Quoted 414 lines · Simon Glass
Hi Przemyslaw,
Quoted 398 lines · Przemyslaw Marczak
Hello Simon,
Quoted 166 lines · Simon Glass
Hi Przemyslaw,
Quoted 143 lines · Przemyslaw Marczak
Hello,
Quoted 137 lines · Simon Glass
Hi Przemyslaw,
Quoted 110 lines · Przemyslaw Marczak
Hello,
Quoted 70 lines · Simon Glass
Hi Przemyslaw,
Quoted 26 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this
thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the
file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool. It just copy the test files to the tested fat16 partition mounted using
the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were
found on the partiion, by the U-Boot read command. It's not useful for
the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this:
echo
"some cmd" > /dev/ttyS0.
Unfortunately the sandbox config seems to be broken.
Quoted 7 lines
The bug was not so obvious, any read/write on fat partition can change
fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Quoted 7 lines
Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was
just my dirty repo - sandbox compiles and works well.
Somewhat bewildering, but it does not in fact fix my problem.
Here is a compressed version of the fat filesystem if you want to take a look:
Quoted 5 lines
https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing
Regards,
Simon
I had put this image on my Trats2 device on partition mmc 0:6 using ums and
dd linux command. Next I put latest mainline u-boot, commit:
e3bf81b1e841ecabe7c8b3d48621256db8b8623e : "Merge branch 'master' of
git://git.denx.de/u-boot-mpc85xx"
So this is the version with the fat bug. But I can see and load the file:
"bcm2835-rpi-b-rev2.dtb".
Trats2 # fatls mmc 0:6
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
18 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 #
The only missing file is "boot.scr", it's ignored by "ls" command but can be
loaded...
Trats2 # fatload mmc 0:6 0x40000000 boot.scr
reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
I suppose that the partition image which you shared for me is little different, than this mentioned in the topic "[PATCH U-Boot] ARM: rpi_b:
detect board revision"
Probably the sequence of writing files to this partition was different, and
the different file is ignored.
After putting the debug macro on the top of fs/fat/fat.c:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
RootDentname == NULL - 0
18 file(s), 1 dir(s)
And next test on commit 9b416a9f4ca7cf5ac4d5f7143d67edde7f7d7326 with my fat
patch.
Trats2 # fatls mmc 0:6
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
19 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 # fatload mmc 0:6 0x40000000 boot.scr reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
So the only difference on this image is, that with my patch, the file "boot.scr" ignored by ls command is now visible - but in both cases it can
be loaded into memory and the crc is correct.
After enabling the debug:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
     17840   bootcode.bin
       120   cmdline.txt
      1331   config.txt
      6115   fixup.dat
      2324   fixup_cd.dat
      9166   fixup_x.dat
   3232856   kernel.img
   2615064   start.elf
END LOOP: j=0   clust_size=16
FAT read sect=81, clust_size=16, DIRENTSPERBLOCK=16
    533080   start_cd.elf
   3572200   start_x.elf
       137   issue.txt
     18974   license.oracle
    295524   u-boot.bin
      1331   config.txt~
       256   boot.scr
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
             extlinux/
   3368648   zimage
      3963   bcm2835-rpi-b.dtb
      3963   bcm2835.dtb
      3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
FAT read sect=83, clust_size=16, DIRENTSPERBLOCK=16 RootDentname == NULL - 0
19 file(s), 1 dir(s)
So as I checked the file:
256 boot.scr
is next behind to the:
1331 config.txt~
And this can be checked with hex dump:
hd -s 0xa400 -n 512 bad-fat.dd
Your fat image is good example of what my patch fixes.
As you can see on the simple debug info, without the fix,the sector 80 and
81 is stored in the buffer (there are always 2 sectors in the buffer). If you see the hex dump of the second sector:
hd -s 0xa200 -n 512 bad-fat.dd
You will see that at the end of this sector, there is a long name entry for
file "boot.scr".
In the next loop (without the fix):
END LOOP: j=1 clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
extlinux/
the sector 82 and 83 is loaded in to the buffer, so the long name entry of
"boot.scr" file from the end of sector 81 is now in the heaven, and the file
will be ignored by the ls command.
The sector 82 can be checked by:
hd -s 0xa400 -n 512 bad-fat.dd
It begins with the short name entry of file "boot.scr".
After applying my fix, there are always three sectors in the buffer, because
the last one is moved into the buffer begin and two next are loaded into the
buffer next to the last one.
And the buffer pointer is always on the second buffered sector beside first
loop.
Quoted 3 lines
So I think this patch fixes the issue well.
Could you describe your issue more precisely?
I think you left out the path. The file I tried to load was:
/syslinux/..//bcm2835-rpi-b-rev2.dtb
It works OK without the path on the front.

Quoted 7 lines

Quoted 3 lines
Regards,
Simon
Yes I didn't use any path.
But why are you using such path, if there is no such directory? There is only /extlinux directory on the fat image which you shared.

This is a feature of the extlinux system, a general way of loading a kernel that U-Boot now supports. It feels like a U-Boot bug to me.

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 13:41 UTC #15 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 429 lines · Simon Glass

Hi Przemyslaw,
Quoted 418 lines · Przemyslaw Marczak
Hello,
Quoted 414 lines · Simon Glass
Hi Przemyslaw,
Quoted 398 lines · Przemyslaw Marczak
Hello Simon,
Quoted 166 lines · Simon Glass
Hi Przemyslaw,
Quoted 143 lines · Przemyslaw Marczak
Hello,
Quoted 137 lines · Simon Glass
Hi Przemyslaw,
Quoted 110 lines · Przemyslaw Marczak
Hello,
Quoted 70 lines · Simon Glass
Hi Przemyslaw,
Quoted 26 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
Quoted 7 lines
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
This is very interesting\! Is this the same failure that I saw on this
thread?
http://u-boot.10912.n7.nabble.com/PATCH-U-Boot-ARM-rpi-b-detect-board-revision-td196720.html
(search for fatload)
"I tried this out. It worked OK for me except that it can't find the device tree file bcm2835-rpi-b-rev2.dtb.
Oddly I can fatload it from /bcm2835-rpi-b-rev2.dtb but when I try from /syslinux/..//bcm2835-rpi-b-rev2.dtb it fails and cannot find the
file. Reducing the filename length to 8 chars works. I wonder what year of my life FAT will stop plaguing me? "
Quoted 7 lines
Also can you write a test for this in test/fs/fs-test.sh?
Regards,
Simon
[snip]
Probably this is an another case which is caused by the sector buffer bug.
Does this patch fixed your issue?
I have some simple test for manual use with the ums tool. It just copy the test files to the tested fat16 partition mounted using
the
UMS, next it computes CRC32 of those files on the host and next using U-Boot
fatload/crc32 commands - it tests the read feature. But it's not full automated - I didn't work on getting the log from U-Boot console.
So I could check if the file checksums are proper and if all files were
found on the partiion, by the U-Boot read command. It's not useful for
the
"test/fs/fs-test.sh" because this is not designed for the sandbox. My test writes some commands directly to U-Boot console, like this:
echo
"some cmd" > /dev/ttyS0.
Unfortunately the sandbox config seems to be broken.
Quoted 7 lines
The bug was not so obvious, any read/write on fat partition can change
fat
directory entries or add the new ones and then all data can be read right.
I will send the scripts for such simple test.
I'm not sure if it fixes my problem but it seems likely. I will see if I can make time to test it.
If you want to write input data to U-Boot sandbox we can do that fairly easily. You can import cros_subprocess and use the function there to generate output from your test and inspect the input from U-Boot's command line. Let me know if you'd like an example.
Quoted 7 lines
Quoted 3 lines
Regards,
Simon
Before, I wrote, that sandbox seems to be broken, sorry for this - it was
just my dirty repo - sandbox compiles and works well.
Somewhat bewildering, but it does not in fact fix my problem.
Here is a compressed version of the fat filesystem if you want to take a look:
Quoted 5 lines
https://drive.google.com/file/d/0B7WYZbZ9zd-3NGRMNkFQQTdtV2M/view?usp=sharing
Regards,
Simon
I had put this image on my Trats2 device on partition mmc 0:6 using ums and
dd linux command. Next I put latest mainline u-boot, commit:
e3bf81b1e841ecabe7c8b3d48621256db8b8623e : "Merge branch 'master' of
git://git.denx.de/u-boot-mpc85xx"
So this is the version with the fat bug. But I can see and load the file:
"bcm2835-rpi-b-rev2.dtb".
Trats2 # fatls mmc 0:6
      17840   bootcode.bin
        120   cmdline.txt
       1331   config.txt
       6115   fixup.dat
       2324   fixup_cd.dat
       9166   fixup_x.dat
    3232856   kernel.img
    2615064   start.elf
     533080   start_cd.elf
    3572200   start_x.elf
        137   issue.txt
      18974   license.oracle
     295524   u-boot.bin
       1331   config.txt~
              extlinux/
    3368648   zimage
       3963   bcm2835-rpi-b.dtb
       3963   bcm2835.dtb
       3963   bcm2835-rpi-b-rev2.dtb
18 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 #
The only missing file is "boot.scr", it's ignored by "ls" command but can be
loaded...
Trats2 # fatload mmc 0:6 0x40000000 boot.scr
reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
I suppose that the partition image which you shared for me is little different, than this mentioned in the topic "[PATCH U-Boot] ARM: rpi_b:
detect board revision"
Probably the sequence of writing files to this partition was different, and
the different file is ignored.
After putting the debug macro on the top of fs/fat/fat.c:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
      17840   bootcode.bin
        120   cmdline.txt
       1331   config.txt
       6115   fixup.dat
       2324   fixup_cd.dat
       9166   fixup_x.dat
    3232856   kernel.img
    2615064   start.elf
END LOOP: j=0   clust_size=16
     533080   start_cd.elf
    3572200   start_x.elf
        137   issue.txt
      18974   license.oracle
     295524   u-boot.bin
       1331   config.txt~
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
              extlinux/
    3368648   zimage
       3963   bcm2835-rpi-b.dtb
       3963   bcm2835.dtb
       3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
RootDentname == NULL - 0
18 file(s), 1 dir(s)
And next test on commit 9b416a9f4ca7cf5ac4d5f7143d67edde7f7d7326 with my fat
patch.
Trats2 # fatls mmc 0:6
      17840   bootcode.bin
        120   cmdline.txt
       1331   config.txt
       6115   fixup.dat
       2324   fixup_cd.dat
       9166   fixup_x.dat
    3232856   kernel.img
    2615064   start.elf
     533080   start_cd.elf
    3572200   start_x.elf
        137   issue.txt
      18974   license.oracle
     295524   u-boot.bin
       1331   config.txt~
        256   boot.scr
              extlinux/
    3368648   zimage
       3963   bcm2835-rpi-b.dtb
       3963   bcm2835.dtb
       3963   bcm2835-rpi-b-rev2.dtb
19 file(s), 1 dir(s)
Trats2 # fatload mmc 0:6 0x40000000 bcm2835-rpi-b-rev2.dtb reading bcm2835-rpi-b-rev2.dtb
3963 bytes read in 5 ms (773.4 KiB/s)
Trats2 # crc32 0x40000000 0xf7b
CRC32 for 40000000 ... 40000f7a ==> c36ee8db Trats2 # fatload mmc 0:6 0x40000000 boot.scr reading boot.scr
256 bytes read in 2 ms (125 KiB/s)
Trats2 # crc32 0x40000000 0x100
CRC32 for 40000000 ... 400000ff ==> dc5c79b3
So the only difference on this image is, that with my patch, the file "boot.scr" ignored by ls command is now visible - but in both cases it can
be loaded into memory and the crc is correct.
After enabling the debug:
Trats2 # fatls mmc 0:6
VFAT Support enabled
FAT16, fat_sect: 16, fatlength: 32
Rootdir begins at cluster: 0, sector: 80, offset: a000 Data begins at: 80
Sector size: 512, cluster size: 16
FAT read sect=80, clust_size=16, DIRENTSPERBLOCK=16
      17840   bootcode.bin
        120   cmdline.txt
       1331   config.txt
       6115   fixup.dat
       2324   fixup_cd.dat
       9166   fixup_x.dat
    3232856   kernel.img
    2615064   start.elf
END LOOP: j=0   clust_size=16
FAT read sect=81, clust_size=16, DIRENTSPERBLOCK=16
     533080   start_cd.elf
    3572200   start_x.elf
        137   issue.txt
      18974   license.oracle
     295524   u-boot.bin
       1331   config.txt~
        256   boot.scr
END LOOP: j=1   clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
              extlinux/
    3368648   zimage
       3963   bcm2835-rpi-b.dtb
       3963   bcm2835.dtb
       3963   bcm2835-rpi-b-rev2.dtb
END LOOP: j=0 clust_size=16
FAT read sect=83, clust_size=16, DIRENTSPERBLOCK=16 RootDentname == NULL - 0
19 file(s), 1 dir(s)
So as I checked the file:
256 boot.scr
is next behind to the:
1331 config.txt~
And this can be checked with hex dump:
hd -s 0xa400 -n 512 bad-fat.dd
Your fat image is good example of what my patch fixes.
As you can see on the simple debug info, without the fix,the sector 80 and
81 is stored in the buffer (there are always 2 sectors in the buffer). If you see the hex dump of the second sector:
hd -s 0xa200 -n 512 bad-fat.dd
You will see that at the end of this sector, there is a long name entry for
file "boot.scr".
In the next loop (without the fix):
END LOOP: j=1 clust_size=16
FAT read sect=82, clust_size=16, DIRENTSPERBLOCK=16
extlinux/
the sector 82 and 83 is loaded in to the buffer, so the long name entry of
"boot.scr" file from the end of sector 81 is now in the heaven, and the file
will be ignored by the ls command.
The sector 82 can be checked by:
hd -s 0xa400 -n 512 bad-fat.dd
It begins with the short name entry of file "boot.scr".
After applying my fix, there are always three sectors in the buffer, because
the last one is moved into the buffer begin and two next are loaded into the
buffer next to the last one.
And the buffer pointer is always on the second buffered sector beside first
loop.
Quoted 3 lines
So I think this patch fixes the issue well.
Could you describe your issue more precisely?
I think you left out the path. The file I tried to load was:
   /syslinux/..//bcm2835-rpi-b-rev2.dtb
It works OK without the path on the front.
Quoted 7 lines
Quoted 3 lines
Regards,
Simon
Yes I didn't use any path.
But why are you using such path, if there is no such directory? There is only /extlinux directory on the fat image which you shared.

Quoted 6 lines

This is a feature of the extlinux system, a general way of loading a kernel that U-Boot now supports. It feels like a U-Boot bug to me.
Regards,
Simon

Oh, I see. I didn't used this yet.

But anyway, the fat image which you shared also shows the bug in the fat.

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 13:47 UTC #16 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

In reply to Przemyslaw Marczak

Hi,

Quoted 37 lines · Przemyslaw Marczak

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>

Quoted 3 lines

---
 fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Tested-by Simon Glass <[email protected]>

fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 828

Quoted 13 lines

fs/fat/fat.c +2 -1

index 04a51db..afbf12d 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
        int ret = -1;
        int firsttime;
        __u32 root_cluster = 0;
+       __u32 read_blk;
        int rootdir_size = 0;
-       int j;
+       int j, k;

What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.

fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 830

Quoted 2 lines

fs/fat/fat.c +2 -0

+       int do_read;
+       __u8 *dir_ptr;

Why does it use __u8 instead of u8 or uint8_t for example?

Quoted 56 lines

fs/fat/fat.c +20 -9

        if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
                debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
        }
 
        j = 0;
+       k = 0;
        while (1) {
                int i;
 
-               if (j == 0) {
+               if (mydata->fatsize == 32 || !k) {
+                       dir_ptr = do_fat_read_at_block;
+                       k = 1;
+               } else {
+                       dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+                       memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+               }
+
+               do_read = 1;
+
+               if (mydata->fatsize == 32 && j)
+                       do_read = 0;
+
+               if (do_read) {
                        debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
                                cursect, mydata->clust_size, DIRENTSPERBLOCK);
 
-                       if (disk_read(cursect,
-                                       (mydata->fatsize == 32) ?
-                                       (mydata->clust_size) :
-                                       PREFETCH_BLOCKS,
-                                       do_fat_read_at_block) < 0) {
+                       read_blk = (mydata->fatsize == 32) ?
+                                   mydata->clust_size : PREFETCH_BLOCKS;
+                       if (disk_read(cursect, read_blk, dir_ptr) < 0) {
                                debug("Error: reading rootdir block\n");
                                goto exit;
                        }
 
-                       dentptr = (dir_entry *) do_fat_read_at_block;
+                       dentptr = (dir_entry *)dir_ptr;
                }
 
                for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
                                        get_vfatname(mydata,
                                                     root_cluster,
-                                                    do_fat_read_at_block,
+                                                    dir_ptr,
                                                     dentptr, l_name);
 
                                        if (dols == LS_ROOT) {
--

Quoted 2 lines

1.9.1

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 14:06 UTC #17 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 44 lines · Simon Glass

Hi,
Quoted 37 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
  fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
  1 file changed, 24 insertions(+), 9 deletions(-)

Quoted 2 lines

Tested-by Simon Glass <[email protected]>

Thank you.

Quoted 17 lines

fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 828
Quoted 13 lines
fs/fat/fat.c +2 -1
index 04a51db..afbf12d 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
         int ret = -1;
         int firsttime;
         __u32 root_cluster = 0;
+       __u32 read_blk;
         int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.

k is a counter for a first time read. I will change this code and add some comment.

Quoted 58 lines

fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 830

Quoted 2 lines

fs/fat/fat.c +2 -0

+       int do_read;
+       __u8 *dir_ptr;

Why does it use __u8 instead of u8 or uint8_t for example?

Quoted 54 lines

fs/fat/fat.c +20 -8

         if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
                 debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
         }
 
         j = 0;
+       k = 0;
         while (1) {
                 int i;
 
-               if (j == 0) {
+               if (mydata->fatsize == 32 || !k) {
+                       dir_ptr = do_fat_read_at_block;
+                       k = 1;
+               } else {
+                       dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+                       memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+               }
+
+               do_read = 1;
+
+               if (mydata->fatsize == 32 && j)
+                       do_read = 0;
+
+               if (do_read) {
                         debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
                                 cursect, mydata->clust_size, DIRENTSPERBLOCK);
 
-                       if (disk_read(cursect,
-                                       (mydata->fatsize == 32) ?
-                                       (mydata->clust_size) :
-                                       PREFETCH_BLOCKS,
-                                       do_fat_read_at_block) < 0) {
+                       read_blk = (mydata->fatsize == 32) ?
+                                   mydata->clust_size : PREFETCH_BLOCKS;
+                       if (disk_read(cursect, read_blk, dir_ptr) < 0) {
                                 debug("Error: reading rootdir block\n");
                                 goto exit;
                         }
 
-                       dentptr = (dir_entry *) do_fat_read_at_block;
+                       dentptr = (dir_entry *)dir_ptr;
                 }
 
                 for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
                                         get_vfatname(mydata,
                                                      root_cluster,
-                                                    do_fat_read_at_block,
+                                                    dir_ptr,
                                                      dentptr, l_name);

Quoted 8 lines

Quoted 4 lines

fs/fat/fat.c +0 -1

                                         if (dols == LS_ROOT) {
--

1.9.1

Regards,
Simon

Thank you and best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Przemyslaw Marczak

author

2014-12-18 14:32 UTC #18 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

In reply to Simon Glass

Hello,

Quoted 63 lines · Simon Glass

Hi,
Quoted 37 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
  fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
  1 file changed, 24 insertions(+), 9 deletions(-)
Tested-by Simon Glass <[email protected]>
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 828
Quoted 13 lines
fs/fat/fat.c +2 -1
index 04a51db..afbf12d 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
         int ret = -1;
         int firsttime;
         __u32 root_cluster = 0;
+       __u32 read_blk;
         int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.

Quoted 4 lines

fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 830
Quoted 2 lines
fs/fat/fat.c +2 -0
+       int do_read;
+       __u8 *dir_ptr;
Why does it use __u8 instead of u8 or uint8_t for example?

__u8 is used in a whole fat code, and also as a directory entry buffer, so why not to keep the whole code style?

Quoted 54 lines

fs/fat/fat.c +20 -8

         if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
                 debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
         }
 
         j = 0;
+       k = 0;
         while (1) {
                 int i;
 
-               if (j == 0) {
+               if (mydata->fatsize == 32 || !k) {
+                       dir_ptr = do_fat_read_at_block;
+                       k = 1;
+               } else {
+                       dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+                       memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+               }
+
+               do_read = 1;
+
+               if (mydata->fatsize == 32 && j)
+                       do_read = 0;
+
+               if (do_read) {
                         debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
                                 cursect, mydata->clust_size, DIRENTSPERBLOCK);
 
-                       if (disk_read(cursect,
-                                       (mydata->fatsize == 32) ?
-                                       (mydata->clust_size) :
-                                       PREFETCH_BLOCKS,
-                                       do_fat_read_at_block) < 0) {
+                       read_blk = (mydata->fatsize == 32) ?
+                                   mydata->clust_size : PREFETCH_BLOCKS;
+                       if (disk_read(cursect, read_blk, dir_ptr) < 0) {
                                 debug("Error: reading rootdir block\n");
                                 goto exit;
                         }
 
-                       dentptr = (dir_entry *) do_fat_read_at_block;
+                       dentptr = (dir_entry *)dir_ptr;
                 }
 
                 for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
                                         get_vfatname(mydata,
                                                      root_cluster,
-                                                    do_fat_read_at_block,
+                                                    dir_ptr,
                                                      dentptr, l_name);

Quoted 8 lines

Quoted 4 lines

fs/fat/fat.c +0 -1

                                         if (dols == LS_ROOT) {
--

1.9.1

Regards,
Simon

Thanks,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 14:34 UTC #19 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi Przemyslaw,

Quoted 76 lines · Przemyslaw Marczak

Hello,
Quoted 73 lines · Simon Glass
Hi,
Quoted 38 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
  fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
  1 file changed, 24 insertions(+), 9 deletions(-)
Tested-by Simon Glass <[email protected]>
Quoted 6 lines
fs/fat/fat.c +0 -0
index 04a51db..afbf12d 100644
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, · line 829
Quoted 8 lines
fs/fat/fat.c +2 -1
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos,
 void *buffer,
         int ret = -1;
         int firsttime;
         __u32 root_cluster = 0;
+       __u32 read_blk;
         int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.
Quoted 2 lines
fs/fat/fat.c +2 -0
+       int do_read;
+       __u8 *dir_ptr;

Quoted 5 lines

Quoted 1 line
Why does it use __u8 instead of u8 or uint8_t for example?
__u8 is used in a whole fat code, and also as a directory entry buffer, so why not to keep the whole code style?

OK, sounds good.

Do you have any ideas on the bug I reported?

Quoted 63 lines

fs/fat/fat.c +20 -8

         if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
                 debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t
 pos, void *buffer,
         }
 
         j = 0;
+       k = 0;
         while (1) {
                 int i;
 
-               if (j == 0) {
+               if (mydata->fatsize == 32 || !k) {
+                       dir_ptr = do_fat_read_at_block;
+                       k = 1;
+               } else {
+                       dir_ptr = (do_fat_read_at_block +
 mydata->sect_size);
+                       memcpy(do_fat_read_at_block, dir_ptr,
 mydata->sect_size);
+               }
+
+               do_read = 1;
+
+               if (mydata->fatsize == 32 && j)
+                       do_read = 0;
+
+               if (do_read) {
                         debug("FAT read sect=%d, clust_size=%d,
 DIRENTSPERBLOCK=%zd\n",
                                 cursect, mydata->clust_size,
 DIRENTSPERBLOCK);
 
-                       if (disk_read(cursect,
-                                       (mydata->fatsize == 32) ?
-                                       (mydata->clust_size) :
-                                       PREFETCH_BLOCKS,
-                                       do_fat_read_at_block) < 0) {
+                       read_blk = (mydata->fatsize == 32) ?
+                                   mydata->clust_size : PREFETCH_BLOCKS;
+                       if (disk_read(cursect, read_blk, dir_ptr) < 0) {
                                 debug("Error: reading rootdir block\n");
                                 goto exit;
                         }
 
-                       dentptr = (dir_entry *) do_fat_read_at_block;
+                       dentptr = (dir_entry *)dir_ptr;
                 }
 
                 for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos,
 void *buffer,
 
                                         get_vfatname(mydata,
                                                      root_cluster,
-
 do_fat_read_at_block,
+                                                    dir_ptr,
                                                      dentptr, l_name);

Quoted 3 lines

                                         if (dols == LS_ROOT) {

--
1.9.1

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 14:40 UTC #20 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 82 lines · Simon Glass

Hi Przemyslaw,
Quoted 79 lines · Przemyslaw Marczak
Hello,
Quoted 74 lines · Simon Glass
Hi,
Quoted 38 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
   fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
   1 file changed, 24 insertions(+), 9 deletions(-)
Tested-by Simon Glass <[email protected]>
Quoted 6 lines
fs/fat/fat.c +0 -0
index 04a51db..afbf12d 100644
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, · line 829
Quoted 8 lines
fs/fat/fat.c +2 -1
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos,
 void *buffer,
          int ret = -1;
          int firsttime;
          __u32 root_cluster = 0;
+       __u32 read_blk;
          int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 830
Quoted 2 lines
fs/fat/fat.c +2 -0
+       int do_read;
+       __u8 *dir_ptr;
Why does it use __u8 instead of u8 or uint8_t for example?

Quoted 7 lines

Quoted 2 lines
__u8 is used in a whole fat code, and also as a directory entry buffer, so why not to keep the whole code style?
OK, sounds good.
Do you have any ideas on the bug I reported?

No, but I think that this is not any fat issue.

Quoted 63 lines

fs/fat/fat.c +20 -8

          if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
                  debug("Error: reading boot sector\n");

@@ -910,23 +913,35 @@ int do_fat_read_at(const char *filename, loff_t
 pos, void *buffer,
          }
 
          j = 0;
+       k = 0;
          while (1) {
                  int i;
 
-               if (j == 0) {
+               if (mydata->fatsize == 32 || !k) {
+                       dir_ptr = do_fat_read_at_block;
+                       k = 1;
+               } else {
+                       dir_ptr = (do_fat_read_at_block +
 mydata->sect_size);
+                       memcpy(do_fat_read_at_block, dir_ptr,
 mydata->sect_size);
+               }
+
+               do_read = 1;
+
+               if (mydata->fatsize == 32 && j)
+                       do_read = 0;
+
+               if (do_read) {
                          debug("FAT read sect=%d, clust_size=%d,
 DIRENTSPERBLOCK=%zd\n",
                                  cursect, mydata->clust_size,
 DIRENTSPERBLOCK);
 
-                       if (disk_read(cursect,
-                                       (mydata->fatsize == 32) ?
-                                       (mydata->clust_size) :
-                                       PREFETCH_BLOCKS,
-                                       do_fat_read_at_block) < 0) {
+                       read_blk = (mydata->fatsize == 32) ?
+                                   mydata->clust_size : PREFETCH_BLOCKS;
+                       if (disk_read(cursect, read_blk, dir_ptr) < 0) {
                                  debug("Error: reading rootdir block\n");
                                  goto exit;
                          }
 
-                       dentptr = (dir_entry *) do_fat_read_at_block;
+                       dentptr = (dir_entry *)dir_ptr;
                  }
 
                  for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +966,7 @@ int do_fat_read_at(const char *filename, loff_t pos,
 void *buffer,
 
                                          get_vfatname(mydata,
                                                       root_cluster,
-
 do_fat_read_at_block,
+                                                    dir_ptr,
                                                       dentptr, l_name);

Quoted 7 lines

Quoted 3 lines

                                          if (dols == LS_ROOT) {

--
1.9.1

Regards,
Simon

Thanks,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Simon Glass

2014-12-18 14:56 UTC #21 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hi,

Quoted 101 lines · Przemyslaw Marczak

Hello,
Quoted 97 lines · Simon Glass
Hi Przemyslaw,
Quoted 90 lines · Przemyslaw Marczak
Hello,
Quoted 74 lines · Simon Glass
Hi,
Quoted 39 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
   fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
   1 file changed, 24 insertions(+), 9 deletions(-)
Tested-by Simon Glass <[email protected]>
Quoted 6 lines
fs/fat/fat.c +0 -0
index 04a51db..afbf12d 100644
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t · line 830
Quoted 9 lines
fs/fat/fat.c +2 -1
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t
 pos,
 void *buffer,
          int ret = -1;
          int firsttime;
          __u32 root_cluster = 0;
+       __u32 read_blk;
          int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.
Quoted 6 lines
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t pos, void *buffer, · line 830
Quoted 2 lines
fs/fat/fat.c +2 -0
+       int do_read;
+       __u8 *dir_ptr;
Why does it use __u8 instead of u8 or uint8_t for example?
__u8 is used in a whole fat code, and also as a directory entry buffer, so
why not to keep the whole code style?

Quoted 6 lines

Quoted 4 lines
OK, sounds good.
Do you have any ideas on the bug I reported?
No, but I think that this is not any fat issue.

Can you explain what you mean here? What other software could be causing this?

Regards,
Simon

Przemyslaw Marczak

author

2014-12-18 15:12 UTC #22 lore raw

PATCH 1/1 fs: fat: read: fix fat16 ls/read issue

Hello,

Quoted 109 lines · Simon Glass

Hi,
Quoted 106 lines · Przemyslaw Marczak
Hello,
Quoted 101 lines · Simon Glass
Hi Przemyslaw,
Quoted 83 lines · Przemyslaw Marczak
Hello,
Quoted 79 lines · Simon Glass
Hi,
Quoted 39 lines · Przemyslaw Marczak
The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Quoted 3 lines
---
    fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
    1 file changed, 24 insertions(+), 9 deletions(-)
Tested-by Simon Glass <[email protected]>
Quoted 6 lines
fs/fat/fat.c +0 -0
index 04a51db..afbf12d 100644
fs/fat/fat.c int do_fat_read_at(const char *filename, loff_t · line 830
Quoted 9 lines
fs/fat/fat.c +2 -1
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t
 pos,
 void *buffer,
           int ret = -1;
           int firsttime;
           __u32 root_cluster = 0;
+       __u32 read_blk;
           int rootdir_size = 0;
-       int j;
+       int j, k;
What is k? Can we use a proper variable name? Also for j. That might save needing a comment for them.
Quoted 2 lines
fs/fat/fat.c +2 -0
+       int do_read;
+       __u8 *dir_ptr;
Quoted 7 lines
Quoted 1 line
Why does it use __u8 instead of u8 or uint8_t for example?
__u8 is used in a whole fat code, and also as a directory entry buffer, so
why not to keep the whole code style?
OK, sounds good.
Do you have any ideas on the bug I reported?

Quoted 7 lines

Quoted 1 line
No, but I think that this is not any fat issue.
Can you explain what you mean here? What other software could be causing this?
Regards,
Simon

The fat code is quite unreadable, but it is simple.
Passing the "/syslinux/..//bcm2835-rpi-b-rev2.dtb" as the fatload argument is just treat as a "/syslinux" directory which doesn't exists. So the file can't be read, right?

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Przemyslaw Marczak

author

2014-12-18 15:21 UTC #23 lore raw

PATCH v2 1/1 fs: fat: read: fix fat16 ls/read issue

In reply to Przemyslaw Marczak

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.

This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.

If some file long name entries are stored in two sectors, the we have two cases:

Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!

This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next

The commit doesn't affects the fat32 imlementation, which works good as previous.

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc Mikhail Zolotaryov <[email protected]>

Cc Tom Rini <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Simon Glass <[email protected]>

Cc Suriyan Ramasami <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Wolfgang Denk <[email protected]>

---
Changes v2:
- add more expressive variable names
- add code comment to the patch change

---
 fs/fat/fat.c | 69 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 18 deletions(-)

fs/fat/fat.c +51 ~~-18~~

index 04a51db..bccc3e3 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	int ret = -1;
 	int firsttime;
 	__u32 root_cluster = 0;
+	__u32 read_blk;
 	int rootdir_size = 0;
-	int j;
+	int buffer_blk_cnt;
+	int do_read;
+	__u8 *dir_ptr;
 
 	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
 		debug("Error: reading boot sector\n");

@@ -909,24 +912,54 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 		isdir = 1;
 	}
 
-	j = 0;
+	buffer_blk_cnt = 0;
+	firsttime = 1;
 	while (1) {
 		int i;
 
-		if (j == 0) {
-			debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
-				cursect, mydata->clust_size, DIRENTSPERBLOCK);
+		if (mydata->fatsize == 32 || firsttime) {
+			dir_ptr = do_fat_read_at_block;
+			firsttime = 0;
+		} else {
+			/**
+			 * FAT16 sector buffer modification:
+			 * Each loop, the second buffered block is moved to
+			 * the buffer begin, and two next sectors are read
+			 * next to the previously moved one. So the sector
+			 * buffer keeps always 3 sectors for fat16.
+			 * And the current sector is the buffer second sector
+			 * beside the "firsttime" read, when it is the first one.
+			 *
+			 * PREFETCH_BLOCKS is 2 for FAT16 == loop[0:1]
+			 * n = computed root dir sector
+			 * loop |  cursect-1  | cursect    | cursect+1  |
+			 *   0  |  sector n+0 | sector n+1 | none       |
+			 *   1  |  none       | sector n+0 | sector n+1 |
+			 *   0  |  sector n+1 | sector n+2 | sector n+3 |
+			 *   1  |  sector n+3 | ...
+			*/
+			dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+			memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+		}
+
+		do_read = 1;
+
+		if (mydata->fatsize == 32 && buffer_blk_cnt)
+			do_read = 0;
+
+		if (do_read) {
+			read_blk = (mydata->fatsize == 32) ?
+				    mydata->clust_size : PREFETCH_BLOCKS;
+
+			debug("FAT read(sect=%d, cnt:%d), clust_size=%d, DIRENTSPERBLOCK=%zd\n",
+				cursect, read_blk, mydata->clust_size, DIRENTSPERBLOCK);
 
-			if (disk_read(cursect,
-					(mydata->fatsize == 32) ?
-					(mydata->clust_size) :
-					PREFETCH_BLOCKS,
-					do_fat_read_at_block) < 0) {
+			if (disk_read(cursect, read_blk, dir_ptr) < 0) {
 				debug("Error: reading rootdir block\n");
 				goto exit;
 			}
 
-			dentptr = (dir_entry *) do_fat_read_at_block;
+			dentptr = (dir_entry *)dir_ptr;
 		}
 
 		for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +984,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 					get_vfatname(mydata,
 						     root_cluster,
-						     do_fat_read_at_block,
+						     dir_ptr,
 						     dentptr, l_name);
 
 					if (dols == LS_ROOT) {

@@ -1062,7 +1095,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 			goto rootdir_done;	/* We got a match */
 		}
-		debug("END LOOP: j=%d   clust_size=%d\n", j,
+		debug("END LOOP: buffer_blk_cnt=%d   clust_size=%d\n", buffer_blk_cnt,
 		       mydata->clust_size);
 
 		/*

@@ -1070,10 +1103,10 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 		 * root directory clusters when a cluster has been
 		 * completely processed.
 		 */
-		++j;
+		++buffer_blk_cnt;
 		int rootdir_end = 0;
 		if (mydata->fatsize == 32) {
-			if (j == mydata->clust_size) {
+			if (buffer_blk_cnt == mydata->clust_size) {
 				int nxtsect = 0;
 				int nxt_clust = 0;

@@ -1086,11 +1119,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 				root_cluster = nxt_clust;
 
 				cursect = nxtsect;
-				j = 0;
+				buffer_blk_cnt = 0;
 			}
 		} else {
-			if (j == PREFETCH_BLOCKS)
-				j = 0;
+			if (buffer_blk_cnt == PREFETCH_BLOCKS)
+				buffer_blk_cnt = 0;
 
 			rootdir_end = (++cursect - mydata->rootdir_sect >=
 				       rootdir_size);

Signature · 1 line

1.9.1

Przemyslaw Marczak

author

2014-12-18 16:14 UTC #24 lore raw

PATCH v3 1/1 fs: fat: read: fix fat16 ls/read issue

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.

This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.

If some file long name entries are stored in two sectors, the we have two cases:

Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!

This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next

The commit doesn't affects the fat32 imlementation, which works good as previous.

Signed-off-by Przemyslaw Marczak <[email protected]>

Cc Mikhail Zolotaryov <[email protected]>

Cc Tom Rini <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Simon Glass <[email protected]>

Cc Suriyan Ramasami <[email protected]>

Cc Lukasz Majewski <[email protected]>

Cc Wolfgang Denk <[email protected]>

Tested-by Simon Glass <[email protected]>

---
Changes v2:
- add more expressive variable names
- add code comment to the patch change

Changes v3:
- add Tested-by

---
 fs/fat/fat.c | 69 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 18 deletions(-)

fs/fat/fat.c +51 ~~-18~~

index 04a51db..bccc3e3 100644
@@ -823,8 +823,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 	int ret = -1;
 	int firsttime;
 	__u32 root_cluster = 0;
+	__u32 read_blk;
 	int rootdir_size = 0;
-	int j;
+	int buffer_blk_cnt;
+	int do_read;
+	__u8 *dir_ptr;
 
 	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
 		debug("Error: reading boot sector\n");

@@ -909,24 +912,54 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 		isdir = 1;
 	}
 
-	j = 0;
+	buffer_blk_cnt = 0;
+	firsttime = 1;
 	while (1) {
 		int i;
 
-		if (j == 0) {
-			debug("FAT read sect=%d, clust_size=%d, DIRENTSPERBLOCK=%zd\n",
-				cursect, mydata->clust_size, DIRENTSPERBLOCK);
+		if (mydata->fatsize == 32 || firsttime) {
+			dir_ptr = do_fat_read_at_block;
+			firsttime = 0;
+		} else {
+			/**
+			 * FAT16 sector buffer modification:
+			 * Each loop, the second buffered block is moved to
+			 * the buffer begin, and two next sectors are read
+			 * next to the previously moved one. So the sector
+			 * buffer keeps always 3 sectors for fat16.
+			 * And the current sector is the buffer second sector
+			 * beside the "firsttime" read, when it is the first one.
+			 *
+			 * PREFETCH_BLOCKS is 2 for FAT16 == loop[0:1]
+			 * n = computed root dir sector
+			 * loop |  cursect-1  | cursect    | cursect+1  |
+			 *   0  |  sector n+0 | sector n+1 | none       |
+			 *   1  |  none       | sector n+0 | sector n+1 |
+			 *   0  |  sector n+1 | sector n+2 | sector n+3 |
+			 *   1  |  sector n+3 | ...
+			*/
+			dir_ptr = (do_fat_read_at_block + mydata->sect_size);
+			memcpy(do_fat_read_at_block, dir_ptr, mydata->sect_size);
+		}
+
+		do_read = 1;
+
+		if (mydata->fatsize == 32 && buffer_blk_cnt)
+			do_read = 0;
+
+		if (do_read) {
+			read_blk = (mydata->fatsize == 32) ?
+				    mydata->clust_size : PREFETCH_BLOCKS;
+
+			debug("FAT read(sect=%d, cnt:%d), clust_size=%d, DIRENTSPERBLOCK=%zd\n",
+				cursect, read_blk, mydata->clust_size, DIRENTSPERBLOCK);
 
-			if (disk_read(cursect,
-					(mydata->fatsize == 32) ?
-					(mydata->clust_size) :
-					PREFETCH_BLOCKS,
-					do_fat_read_at_block) < 0) {
+			if (disk_read(cursect, read_blk, dir_ptr) < 0) {
 				debug("Error: reading rootdir block\n");
 				goto exit;
 			}
 
-			dentptr = (dir_entry *) do_fat_read_at_block;
+			dentptr = (dir_entry *)dir_ptr;
 		}
 
 		for (i = 0; i < DIRENTSPERBLOCK; i++) {

@@ -951,7 +984,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 					get_vfatname(mydata,
 						     root_cluster,
-						     do_fat_read_at_block,
+						     dir_ptr,
 						     dentptr, l_name);
 
 					if (dols == LS_ROOT) {

@@ -1062,7 +1095,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 			goto rootdir_done;	/* We got a match */
 		}
-		debug("END LOOP: j=%d   clust_size=%d\n", j,
+		debug("END LOOP: buffer_blk_cnt=%d   clust_size=%d\n", buffer_blk_cnt,
 		       mydata->clust_size);
 
 		/*

@@ -1070,10 +1103,10 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 		 * root directory clusters when a cluster has been
 		 * completely processed.
 		 */
-		++j;
+		++buffer_blk_cnt;
 		int rootdir_end = 0;
 		if (mydata->fatsize == 32) {
-			if (j == mydata->clust_size) {
+			if (buffer_blk_cnt == mydata->clust_size) {
 				int nxtsect = 0;
 				int nxt_clust = 0;

@@ -1086,11 +1119,11 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer,
 				root_cluster = nxt_clust;
 
 				cursect = nxtsect;
-				j = 0;
+				buffer_blk_cnt = 0;
 			}
 		} else {
-			if (j == PREFETCH_BLOCKS)
-				j = 0;
+			if (buffer_blk_cnt == PREFETCH_BLOCKS)
+				buffer_blk_cnt = 0;
 
 			rootdir_end = (++cursect - mydata->rootdir_sect >=
 				       rootdir_size);

Signature · 1 line

1.9.1

Tom Rini

2015-01-07 15:12 UTC #25 lore raw

PATCH v3 1/1 fs: fat: read: fix fat16 ls/read issue

Quoted 29 lines · Przemyslaw Marczak

The present fat implementation ignores FAT16 long name directory entries which aren't placed in a single sector.
This was becouse of the buffer was always filled by the two sectors, and the loop was made also for two sectors.
If some file long name entries are stored in two sectors, the we have two cases:
Case 1:
Both of sectors are in the buffer - all required data for long file name is in the buffer.
- Read OK!
Case 2:
The current directory entry is placed at the end of the second buffered sector. And the next entries are placed in a sector which is not buffered yet. Then two next sectors are buffered and the mentioned entry is ignored.
- Read fail!
This commit fixes this issue by:
- read two sectors after loop on each single is done
- keep the last used sector as a first in the buffer before the read of two next
The commit doesn't affects the fat32 imlementation, which works good as previous.

Quoted 9 lines

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Mikhail Zolotaryov <[email protected]>
Cc Tom Rini <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Simon Glass <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Wolfgang Denk <[email protected]>
Tested-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150107/db7b8b15/attachment.pgp>