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

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

Hello,

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

No, but I think that this is not any fat issue.

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

Thanks,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com