[U-Boot] [PATCH] fs: fat: read: fix fat16 ls/read issue

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello,

On 12/18/2014 02:36 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 18 December 2014 at 06:31, Przemyslaw Marczak <p.marczak@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@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@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@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@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
>
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