[U-Boot] fatwrite problem

[Benoît Thébaudeau]

Hi Tom, Ruud, Mats,

On Friday, April 12, 2013 5:12:31 PM, Tom Rini wrote:
> On Fri, Apr 12, 2013 at 05:06:35PM +0200, Ruud Commandeur wrote:
> > Hi Mats,
> > 
> > Thanks a lot, this seems to solve my problem. Nothing more actualy than
> > adding a "if(size)" around the code block of set_sector( ). I could have
> > thought of that myself, but was not sure if anything else should be done
> > in this case...
> 
> Since your change sounds slightly different, can you confirm that it
> also solves the problem and if so post it as patch with Signed-off-by
> and so forth?  Thanks!
> 
> > 
> > Regards,
> > 
> > Ruud
> > 
> > -----Oorspronkelijk bericht-----
> > Van: Mats K?rrman [mailto:Mats.Karrman at tritech.se]
> > Verzonden: vrijdag 12 april 2013 16:11
> > Aan: Ruud Commandeur
> > CC: u-boot at lists.denx.de
> > Onderwerp: RE: fatwrite problem
> > 
> > Hi Ruud,
> > 
> > Ruud Commandeur wrote:
> > > Once the size of the set_cluster call equals 0, the mmc command is
> > > incomplete and times out. In the earlier reported problem, a patch is
> > > mentioned, but not available for dowload here. Also in the latest
> > > versions of the git repository I could not find a patch for this
> > > problem. Can anyone tell me if there is a fix for this problem?
> > 
> > I asked Damien Huang (back then) and got the following reply (I think there
> > was
> > some character encoding problem so his mail never was accepted by the
> > list).
> > I have not further analyzed the contents, anyway it wasn't the solution to
> > my problem.
> > BR // Mats
> > 
> > Damien Huang wrote:
> > As requested from Mats, I am resending this email. The patch is given
> > below:
> > 
> > diff -cr ./u-boot-original/u-boot/fs/fat/fat_write.c
> > ./u-boot-test/u-boot/fs/fat/fat_write.c
> > *** ./u-boot-original/u-boot/fs/fat/fat_write.c    2013-02-07
> > 14:47:33.314732999 +1100
> > --- ./u-boot-test/u-boot/fs/fat/fat_write.c    2013-02-28
> > 15:36:24.551861920 +1100
> > ***************
> > *** 562,596 ****
> >   {
> >       int idx = 0;
> >       __u32 startsect;
> > !
> > !     if (clustnum > 0)
> > !         startsect = mydata->data_begin +
> > !                 clustnum * mydata->clust_size;
> > !     else
> > !         startsect = mydata->rootdir_sect;
> > !
> > !     debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
> > !
> > !     if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
> > !         debug("Error writing data\n");
> > !         return -1;
> > !     }
> > !
> > !     if (size % mydata->sect_size) {
> > !         __u8 tmpbuf[mydata->sect_size];
> > !
> > !         idx = size / mydata->sect_size;
> > !         buffer += idx * mydata->sect_size;
> > !         memcpy(tmpbuf, buffer, size % mydata->sect_size);
> > !
> > !         if (disk_write(startsect + idx, 1, tmpbuf) < 0) {
> > !             debug("Error writing data\n");
> > !             return -1;
> > !         }
> > !
> > !         return 0;
> > !     }
> > !
> >       return 0;
> >   }
> >  
> > --- 562,595 ----
> >   {
> >       int idx = 0;
> >       __u32 startsect;
> > !     if(size) //if there are data to be set
> > !     {
> > !         if (clustnum > 0)
> > !             startsect = mydata->data_begin +
> > !                     clustnum * mydata->clust_size;
> > !         else
> > !             startsect = mydata->rootdir_sect;
> > !
> > !         debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
> > !
> > !         if (disk_write(startsect, size / mydata->sect_size, buffer) < 0)
> > {
> > !             debug("Error writing data\n");
> > !             return -1;
> > !         }
> > !
> > !         if (size % mydata->sect_size) {
> > !             __u8 tmpbuf[mydata->sect_size];
> > !
> > !             idx = size / mydata->sect_size;
> > !             buffer += idx * mydata->sect_size;
> > !             memcpy(tmpbuf, buffer, size % mydata->sect_size);
> > !
> > !             if (disk_write(startsect + idx, 1, tmpbuf) < 0) {
> > !                 debug("Error writing data\n");
> > !                 return -1;
> > !             }
> > !         }
> > !     }//end if data to be set
> >       return 0;
> >   }
> >  
> > diff -cr ./u-boot-original/u-boot/include/configs/am335x_evm.h
> > ./u-boot-test/u-boot/include/configs/am335x_evm.h
> > *** ./u-boot-original/u-boot/include/configs/am335x_evm.h    2013-02-07
> > 14:47:35.754702325 +1100
> > --- ./u-boot-test/u-boot/include/configs/am335x_evm.h    2013-03-01
> > 12:25:23.942104474 +1100
> > ***************
> > *** 143,148 ****
> > --- 143,149 ----
> >   #define CONFIG_CMD_MMC
> >   #define CONFIG_DOS_PARTITION
> >   #define CONFIG_FS_FAT
> > + #define CONFIG_FAT_WRITE
> >   #define CONFIG_FS_EXT4
> >   #define CONFIG_CMD_FAT
> >   #define CONFIG_CMD_EXT2
> 
> Benoit, what do you think?  Thanks!

This is fine as a workaround, but it does not fix the root cause of the issue:
set_clusster() should not have been called with size set to 0 in the first
place. This is hiding some cluster mishandling. It may mean that a cluster is
wasted at EoF in some cases, which is unexpected and may trigger abnormal
behaviors on systems reading back those files.

What kind of action triggered this issue for you:
 - writing an empty file?
 - writing a file with a size equal to an integer multiple of the cluster size?
 - something else?

This can be caused only be lines 713, 724 or 739. Looking closer at the code,
only line 713 triggers this issue, so an 'if' could be added here rather than in
set_cluster().

The code for writing an empty file is broken: It should set both the start
cluster and the size to 0 in the corresponding directory entry, but it actually
allocates a single cluster (see find_empty_cluster() called from
do_fat_write()).

So 2 fixes are required here:
 - 'if' either in set_cluster(), or on line 713 to discard empty sizes, or make
   the underlying MMC driver return without doing anything if size is 0, the
   latter being perhaps the most robust solution if it does not cause other
   issues for this driver,
 - empty file creation.

Best regards,
Benoît