[U-Boot] fatwrite problem
Benoît Thébaudeau
benoit.thebaudeau at advansee.com
Fri Apr 12 23:17:48 CEST 2013
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