[U-Boot] [PATCH] fs: fat: read: fix fat16 ls/read issue
Simon Glass
sjg at chromium.org
Thu Dec 18 14:36:09 CET 2014
Hi Przemyslaw,
On 18 December 2014 at 06:31, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> Hello,
>
>
> On 12/18/2014 02:14 PM, Simon Glass wrote:
>>
>> Hi Przemyslaw,
>>
>> On 18 December 2014 at 03:26, Przemyslaw Marczak <p.marczak at samsung.com>
>> wrote:
>>>
>>> Hello Simon,
>>>
>>>
>>> On 12/18/2014 04:39 AM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Przemyslaw,
>>>>
>>>> On 17 December 2014 at 02:03, Przemyslaw Marczak <p.marczak at samsung.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>>
>>>>> On 12/16/2014 11:26 PM, Simon Glass wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Przemyslaw,
>>>>>>
>>>>>> On 12 December 2014 at 08:30, Przemyslaw Marczak
>>>>>> <p.marczak at samsung.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>>
>>>>>>> On 12/12/2014 01:32 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Przemyslaw,
>>>>>>>>
>>>>>>>> On 11 December 2014 at 05:01, Przemyslaw Marczak
>>>>>>>> <p.marczak at samsung.com>
>>>>>>>> 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
More information about the U-Boot
mailing list