From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 18 Dec 2014 15:40:07 +0100 Subject: [U-Boot] [PATCH] fs: fat: read: fix fat16 ls/read issue In-Reply-To: References: <1418299263-1148-1-git-send-email-p.marczak@samsung.com> <5492E587.3040900@samsung.com> Message-ID: <5492E747.5030205@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, On 12/18/2014 03:34 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 18 December 2014 at 07:32, Przemyslaw Marczak wrote: >> Hello, >> >> On 12/18/2014 02:47 PM, Simon Glass wrote: >>> >>> Hi, >>> >>> On 11 December 2014 at 05:01, Przemyslaw Marczak >>> 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 >>>> Cc: Mikhail Zolotaryov >>>> Cc: Tom Rini >>>> Cc: Stephen Warren >>>> Cc: Simon Glass >>>> Cc: Suriyan Ramasami >>>> Cc: Lukasz Majewski >>>> Cc: Wolfgang Denk >>>> --- >>>> fs/fat/fat.c | 33 ++++++++++++++++++++++++--------- >>>> 1 file changed, 24 insertions(+), 9 deletions(-) >>> >>> >>> Tested-by: Simon Glass >>> >>>> >>>> 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