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

[Przemyslaw Marczak]

Hello,

On 12/18/2014 02:47 PM, Simon Glass wrote:
> Hi,
>
> 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.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
>> Cc: Mikhail Zolotaryov <lebon at lebon.org.ua>
>> Cc: Tom Rini <trini at ti.com>
>> Cc: Stephen Warren <swarren at nvidia.com>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Suriyan Ramasami <suriyan.r at gmail.com>
>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>> Cc: Wolfgang Denk <wd at denx.de>
>> ---
>>   fs/fat/fat.c | 33 ++++++++++++++++++++++++---------
>>   1 file changed, 24 insertions(+), 9 deletions(-)
>
> Tested-by: Simon Glass <sjg at chomium.org>
>

Thank you.

>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 04a51db..afbf12d 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -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.

>> +       int do_read;
>> +       __u8 *dir_ptr;
>
> Why does it use __u8 instead of u8 or uint8_t for example?
>>
>>          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) {
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

Thank you and best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com