From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 18 Dec 2014 14:41:49 +0100 Subject: [U-Boot] [PATCH] fs: fat: read: fix fat16 ls/read issue In-Reply-To: References: <1418299263-1148-1-git-send-email-p.marczak@samsung.com> <548B09FD.2080005@samsung.com> <549146E3.7090600@samsung.com> <5492ABCF.402@samsung.com> <5492D73D.7010604@samsung.com> Message-ID: <5492D99D.5010109@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, On 12/18/2014 02:36 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 18 December 2014 at 06:31, Przemyslaw Marczak wrote: >> Hello, >> >> >> On 12/18/2014 02:14 PM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 18 December 2014 at 03:26, Przemyslaw Marczak >>> wrote: >>>> >>>> Hello Simon, >>>> >>>> >>>> On 12/18/2014 04:39 AM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Przemyslaw, >>>>> >>>>> On 17 December 2014 at 02:03, Przemyslaw Marczak >>>>> wrote: >>>>>> >>>>>> >>>>>> Hello, >>>>>> >>>>>> >>>>>> On 12/16/2014 11:26 PM, Simon Glass wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Przemyslaw, >>>>>>> >>>>>>> On 12 December 2014 at 08:30, Przemyslaw Marczak >>>>>>> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hello, >>>>>>>> >>>>>>>> >>>>>>>> On 12/12/2014 01:32 AM, Simon Glass wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Przemyslaw, >>>>>>>>> >>>>>>>>> On 11 December 2014 at 05:01, Przemyslaw Marczak >>>>>>>>> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 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. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> 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] >>>>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> 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 >>>>>>> >>>>>> >>>>>> 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 >>>>> >>>> >>>> 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. >>>> >>>> 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 >>> >> >> 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 > 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, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com