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

[Simon Glass]

Hi Przemyslaw,

On 18 December 2014 at 07:32, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> 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>
>>
>>>
>>> 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.
>>
>>> +       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?

OK, sounds good.

Do you have any ideas on the bug I reported?

>
>
>>>
>>>          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