[U-Boot] fatwrite problem

[Tom Rini <trini@ti.com>]

On Fri, Apr 12, 2013 at 09:39:19PM +0200, Beno??t Th??baudeau wrote:
> 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,

I prefer updating set_cluster since he sees this in one MMC driver and I
in another (meaning we would have to go whack all of them, or start
poking drivers/mmc/mmc.c).  I can confirm that size != 0 in that test
fixes the problem here.

>  - empty file creation.

I took a stab at this and ended up killing my test FAT partition, which
is not a big deal, but can you take a stab at this please?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130412/4bc199b9/attachment.pgp>