[U-Boot] fatwrite problem
Ruud Commandeur
RCommandeur at clb.nl
Tue Apr 16 11:32:47 CEST 2013
Hi Everyone,
As far as I noticed, set_cluster can be called quite often with a size being zero. That can indeed be with a file that has a size being an exact multiple (including 0) of the clustersize, but also for files that are smaller than the size of one cluster. In that case, the 1st call to set_cluster has a size being zero:
> fatwrite mmc 0:1 42000000 test3 200
writing test3
set_cluster; clustnum: 1487, startsect: 6104, size 0
set_cluster; clustnum: 1487, startsect: 6104, size 512
set_cluster; clustnum: -6, startsect: 132, size 2048
512 bytes written
This was solved by adding the "if(size)" in set_cluster (otherwise the above test would fail). However: this does not solve all the problems. If I tried to write a file of 16 bytes, I still got an error. Looking closer at set_cluster, the 1st call to disk_write( ) has a size being "size / mydata->sect_size". This would mean that for any file (or file remainder) smaller than the sector size (typicaly 512 bytes), the fatwrite would fail.
So to my opinion, the actual cause of the problem is in the function "disk_write( )", that can't be called with a size being zero. That could be solved by adding tests before calling set_cluster and/or before calling disk_write from set_cluster, but the safest (and simplest) would be to add this piece of code to disk_write:
if(nr_blocks == 0) {
return 0;
}
With this change I have been able to write various file sizes so far without problems (512 bytes, 2048, 2049, 16, 0).
Another option would be to solve this in the cur_dev->block_write function, being mmc_bwrite( ) for the mmc device. Since his has a do { } while, it will always call mmc_write_blocks() even if blkcnt equals zero. Maybe even the mmc_write_blocks would be the best place to fix this...
I will do some further testing here. Meanwhile, please let me know if this change would make sense to you, and what would be the best place to put it. Probably the lowest level function, if you would ask me.
Also I would like to know if it would not cause any oher problems according to your knowledge.
Regards,
Ruud
-----Oorspronkelijk bericht-----
Van: Benoît Thébaudeau [mailto:benoit.thebaudeau at advansee.com]
Verzonden: vrijdag 12 april 2013 23:18
Aan: Tom Rini
CC: Ruud Commandeur; u-boot at lists.denx.de; Mats K?rrman
Onderwerp: Re: [U-Boot] fatwrite problem
Hi Tom,
On Friday, April 12, 2013 10:42:34 PM, Tom Rini wrote:
> 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.
OK.
> > - 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!
OK, but I can't guarantee when.
Best regards,
Benoît
More information about the U-Boot
mailing list