[U-Boot] fatwrite problem
Tom Rini
trini at ti.com
Fri Apr 12 22:42:34 CEST 2013
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>
More information about the U-Boot
mailing list