[U-Boot] fatwrite problem
Hi Everyone,
Sinced a few days I noticed some problems writing the uimage to a FAT partition on my SD-card. At first I was afraid of some (physical) SD-card problems, but it appears to be related to he size of the uImage. With some further testing (and adding some debug printing), I could easily reproduce the problem:
//4K file, result OK fatwrite mmc 0:1 42000000 testfile 1000 writing testfile set_cluster; clustnum: 5, startsect: 176, size 2048 set_cluster; clustnum: 6, startsect: 180, size 2048 set_cluster; clustnum: -6, startsect: 132, size 2048 4096 bytes written
//512 bytes file, timeout error, file not written fatwrite mmc 0:1 42000000 testfile 200 writing testfile set_cluster; clustnum: 5, startsect: 176, size 0 MMC0: Bus busy timeout! MMC0: Bus busy timeout! MMC0: Bus busy timeout! MMC0: Bus busy timeout! MMC0: Bus busy timeout! MMC0: Bus busy timeout! set_cluster; clustnum: 5, startsect: 176, size 512 MMC0: Bus busy timeout! MMC0: Bus busy timeout! MMC0: Bus busy timeout! set_cluster; clustnum: -6, startsect: 132, size 2048 MMC0: Bus busy timeout! 512 bytes written
It seems to be related to he problem reported here:
http://www.mail-archive.com/u-boot@lists.denx.de/msg107212.html
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?
Regards,
Ruud
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
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...
Regards,
Ruud
-----Oorspronkelijk bericht----- Van: Mats Kärrman [mailto:Mats.Karrman@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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
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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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!
Hi Tom,
Sorry if I "mislead" you, but my code change is quite the same as was posted by Damien. I would be more than happy to send what you ask for, but I don't think this will add much. And I will have to study a bit then how to do this....
Regards,
Ruud
-----Oorspronkelijk bericht----- Van: Tom Rini [mailto:tom.rini@gmail.com] Namens Tom Rini Verzonden: vrijdag 12 april 2013 17:13 Aan: Ruud Commandeur; Beno??t Th??baudeau CC: Mats K?rrman; u-boot@lists.denx.de Onderwerp: Re: [U-Boot] fatwrite problem
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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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!
On Fri, Apr 12, 2013 at 05:23:35PM +0200, Ruud Commandeur wrote:
Hi Tom,
Sorry if I "mislead" you, but my code change is quite the same as was posted by Damien. I would be more than happy to send what you ask for, but I don't think this will add much. And I will have to study a bit then how to do this....
Right, but since you had an idea on fixing the problem, and can also follow the regular patch submission process (with a signed-off-by line), I can more easily accept that for mainline instead of a privately passed along patch without one. Thanks!
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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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
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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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!
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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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
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@advansee.com] Verzonden: vrijdag 12 april 2013 23:18 Aan: Tom Rini CC: Ruud Commandeur; u-boot@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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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
Hi Tom,
It has become a bit quiet on this thread, but I just made the changes as I suggested, including some other fixes. Here is what I did:
Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c). By doing this in the lowest level function, it also solves a hangup if for instance an "mmc write" command is given with a block count being 0.
Solved a checksum issue in fs/fat/fat.c. The mkcksum has const char arguments with a size specifier, like "const char name[8]". In the function, it is assumed that sizeof(name) will have the value 8, but this is not the case (at least not for the compiler I use and I guess not according to ANSI C). This causes "long filename checksum errors" for each fat file listed or written.
Made some changes to fs/fat/fat_write.c. Fixed testing fat_val for 0xffff/0xfff8 and 0xfffffff/0xffffff8 by adding the corresponding fatsize in the test (as I read in earlier posts) and some changes in debug output.
I have used this for a few weeks now without any mmc and fat related problems sofar.
I do have a patch file for this. Should I post this with a "Signed-off-by (etc, etc)", or would you like to receive and review this patch file yourself first?
Regards,
Ruud
-----Oorspronkelijk bericht----- Van: Ruud Commandeur Verzonden: dinsdag 16 april 2013 11:33 Aan: 'Benoît Thébaudeau'; Tom Rini CC: u-boot@lists.denx.de; Mats K?rrman Onderwerp: RE: [U-Boot] fatwrite problem
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@advansee.com] Verzonden: vrijdag 12 april 2013 23:18 Aan: Tom Rini CC: Ruud Commandeur; u-boot@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@tritech.se] Verzonden: vrijdag 12 april 2013 16:11 Aan: Ruud Commandeur CC: u-boot@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
On Tue, May 14, 2013 at 05:13:44PM +0200, Ruud Commandeur wrote:
Hi Tom,
It has become a bit quiet on this thread, but I just made the changes as I suggested, including some other fixes. Here is what I did:
Added a check for blkcnt > 0 in mmc_write_blocks (drivers/mmc.c). By doing this in the lowest level function, it also solves a hangup if for instance an "mmc write" command is given with a block count being 0.
Solved a checksum issue in fs/fat/fat.c. The mkcksum has const char arguments with a size specifier, like "const char name[8]". In the function, it is assumed that sizeof(name) will have the value 8, but this is not the case (at least not for the compiler I use and I guess not according to ANSI C). This causes "long filename checksum errors" for each fat file listed or written.
Made some changes to fs/fat/fat_write.c. Fixed testing fat_val for 0xffff/0xfff8 and 0xfffffff/0xffffff8 by adding the corresponding fatsize in the test (as I read in earlier posts) and some changes in debug output.
I have used this for a few weeks now without any mmc and fat related problems sofar.
I do have a patch file for this. Should I post this with a "Signed-off-by (etc, etc)", or would you like to receive and review this patch file yourself first?
Please post it with a Signed-off-by line for review on the list, thanks!
set_cluster() was using a temporary buffer without enforcing its alignment for DMA and cache. Moreover, it did not check the alignment of the passed buffer, which can come directly from applicative code or from the user.
This could cause random data corruption, which has been observed on i.MX25 writing to an SD card.
Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to disk_write(), which requires the introduction of a buffer bouncing mechanism for the misaligned buffers passed to set_cluster().
By the way, improve the handling of the corresponding return values from disk_write(): - print them with debug() in case of error, - consider that there is an error is disk_write() returns a smaller block count than the requested one, not only if its return value is negative.
After this change, set_cluster() and get_cluster() are almost symmetrical.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com --- fs/fat/fat_write.c | 48 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index adb6940..d0d9df7 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -555,8 +555,9 @@ static int set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) { - int idx = 0; + __u32 idx = 0; __u32 startsect; + int ret;
if (clustnum > 0) startsect = mydata->data_begin + @@ -566,26 +567,45 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
- if ((size / mydata->sect_size) > 0) { - if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) { - debug("Error writing data\n"); + if ((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1)) { + ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size); + + printf("FAT: Misaligned buffer address (%p)\n", buffer); + + while (size >= mydata->sect_size) { + memcpy(tmpbuf, buffer, mydata->sect_size); + ret = disk_write(startsect++, 1, tmpbuf); + if (ret != 1) { + debug("Error writing data (got %d)\n", ret); + return -1; + } + + buffer += mydata->sect_size; + size -= mydata->sect_size; + } + } else if (size >= mydata->sect_size) { + idx = size / mydata->sect_size; + ret = disk_write(startsect, idx, buffer); + if (ret != idx) { + debug("Error writing data (got %d)\n", ret); return -1; } + + startsect += idx; + idx *= mydata->sect_size; + buffer += idx; + size -= idx; }
- if (size % mydata->sect_size) { - __u8 tmpbuf[mydata->sect_size]; + if (size) { + ALLOC_CACHE_ALIGN_BUFFER(__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"); + memcpy(tmpbuf, buffer, size); + ret = disk_write(startsect, 1, tmpbuf); + if (ret != 1) { + debug("Error writing data (got %d)\n", ret); return -1; } - - return 0; }
return 0;
set_contents() had uselessly split calls to set_cluster(). Merge these calls, which removes some cases of set_cluster() being called with a size of zero.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com --- fs/fat/fat_write.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index d0d9df7..e08cf83 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -728,21 +728,10 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer, endclust = newclust; actsize += bytesperclust; } - /* actsize >= file size */ - actsize -= bytesperclust; - /* set remaining clusters */ - if (set_cluster(mydata, curclust, buffer, (int)actsize) != 0) { - debug("error: writing cluster\n"); - return -1; - }
/* set remaining bytes */ - *gotsize += actsize; - filesize -= actsize; - buffer += actsize; actsize = filesize; - - if (set_cluster(mydata, endclust, buffer, (int)actsize) != 0) { + if (set_cluster(mydata, curclust, buffer, (int)actsize) != 0) { debug("error: writing cluster\n"); return -1; }
On Mon, Sep 28, 2015 at 03:45:29PM +0200, Benoît Thébaudeau wrote:
set_contents() had uselessly split calls to set_cluster(). Merge these calls, which removes some cases of set_cluster() being called with a size of zero.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
Applied to u-boot/master, thanks!
curclust was used instead of newclust in the debug() calls and in one CHECK_CLUST() call, which could skip a failure case.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com --- fs/fat/fat_write.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index e08cf83..2399844 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -721,7 +721,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer, goto getit;
if (CHECK_CLUST(newclust, mydata->fatsize)) { - debug("curclust: 0x%x\n", newclust); + debug("newclust: 0x%x\n", newclust); debug("Invalid FAT entry\n"); return 0; } @@ -754,8 +754,8 @@ getit: filesize -= actsize; buffer += actsize;
- if (CHECK_CLUST(curclust, mydata->fatsize)) { - debug("curclust: 0x%x\n", curclust); + if (CHECK_CLUST(newclust, mydata->fatsize)) { + debug("newclust: 0x%x\n", newclust); debug("Invalid FAT entry\n"); return 0; }
On Mon, Sep 28, 2015 at 03:45:30PM +0200, Benoît Thébaudeau wrote:
curclust was used instead of newclust in the debug() calls and in one CHECK_CLUST() call, which could skip a failure case.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
Applied to u-boot/master, thanks!
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com --- fs/fat/fat_write.c | 72 +++++++++++++++++------------------------------------- 1 file changed, 22 insertions(+), 50 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 2399844..2d032ee 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1028,10 +1028,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size, if (retdent) { /* Update file size and start_cluster in a directory entry */ retdent->size = cpu_to_le32(size); - start_cluster = FAT2CPU16(retdent->start); - if (mydata->fatsize == 32) - start_cluster |= - (FAT2CPU16(retdent->starthi) << 16); + start_cluster = START(retdent);
ret = check_overflow(mydata, start_cluster, size); if (ret) { @@ -1044,29 +1041,6 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size, printf("Error: clearing FAT entries\n"); goto exit; } - - ret = set_contents(mydata, retdent, buffer, size, actwrite); - if (ret < 0) { - printf("Error: writing contents\n"); - goto exit; - } - debug("attempt to write 0x%llx bytes\n", *actwrite); - - /* Flush fat buffer */ - ret = flush_fat_buffer(mydata); - if (ret) { - printf("Error: flush fat buffer\n"); - goto exit; - } - - /* Write directory table to device */ - ret = set_cluster(mydata, dir_curclust, - get_dentfromdir_block, - mydata->clust_size * mydata->sect_size); - if (ret) { - printf("Error: writing directory entry\n"); - goto exit; - } } else { /* Set short name to set alias checksum field in dir_slot */ set_name(empty_dentptr, filename); @@ -1088,31 +1062,29 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size, fill_dentry(mydata, empty_dentptr, filename, start_cluster, size, 0x20);
- ret = set_contents(mydata, empty_dentptr, buffer, size, - actwrite); - if (ret < 0) { - printf("Error: writing contents\n"); - goto exit; - } - debug("attempt to write 0x%llx bytes\n", *actwrite); - - /* Flush fat buffer */ - ret = flush_fat_buffer(mydata); - if (ret) { - printf("Error: flush fat buffer\n"); - goto exit; - } - - /* Write directory table to device */ - ret = set_cluster(mydata, dir_curclust, - get_dentfromdir_block, - mydata->clust_size * mydata->sect_size); - if (ret) { - printf("Error: writing directory entry\n"); - goto exit; - } + retdent = empty_dentptr; }
+ ret = set_contents(mydata, retdent, buffer, size, actwrite); + if (ret < 0) { + printf("Error: writing contents\n"); + goto exit; + } + debug("attempt to write 0x%llx bytes\n", *actwrite); + + /* Flush fat buffer */ + ret = flush_fat_buffer(mydata); + if (ret) { + printf("Error: flush fat buffer\n"); + goto exit; + } + + /* Write directory table to device */ + ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block, + mydata->clust_size * mydata->sect_size); + if (ret) + printf("Error: writing directory entry\n"); + exit: free(mydata->fatbuf); return ret;
On Mon, Sep 28, 2015 at 03:45:31PM +0200, Benoît Thébaudeau wrote:
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
Applied to u-boot/master, thanks!
Overwriting an empty file not created by U-Boot did not work, and it could even corrupt the FAT. Moreover, creating empty files or emptying existing files allocated a cluster, which is not standard.
Fix this by always keeping empty files clusterless as specified by Microsoft (the start cluster must be set to 0 in the directory entry in that case), and by supporting overwriting such files.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com --- fs/fat/fat_write.c | 85 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 21 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 2d032ee..af828d0 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -710,6 +710,14 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer,
debug("%llu bytes\n", filesize);
+ if (!curclust) { + if (filesize) { + debug("error: nonempty clusterless file!\n"); + return -1; + } + return 0; + } + actsize = bytesperclust; endclust = curclust; do { @@ -765,15 +773,24 @@ getit: }
/* - * Fill dir_entry + * Set start cluster in directory entry */ -static void fill_dentry(fsdata *mydata, dir_entry *dentptr, - const char *filename, __u32 start_cluster, __u32 size, __u8 attr) +static void set_start_cluster(const fsdata *mydata, dir_entry *dentptr, + __u32 start_cluster) { if (mydata->fatsize == 32) dentptr->starthi = cpu_to_le16((start_cluster & 0xffff0000) >> 16); dentptr->start = cpu_to_le16(start_cluster & 0xffff); +} + +/* + * Fill dir_entry + */ +static void fill_dentry(fsdata *mydata, dir_entry *dentptr, + const char *filename, __u32 start_cluster, __u32 size, __u8 attr) +{ + set_start_cluster(mydata, dentptr, start_cluster); dentptr->size = cpu_to_le32(size);
dentptr->attr = attr; @@ -1030,32 +1047,58 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size, retdent->size = cpu_to_le32(size); start_cluster = START(retdent);
- ret = check_overflow(mydata, start_cluster, size); - if (ret) { - printf("Error: %llu overflow\n", size); - goto exit; - } + if (start_cluster) { + if (size) { + ret = check_overflow(mydata, start_cluster, + size); + if (ret) { + printf("Error: %llu overflow\n", size); + goto exit; + } + }
- ret = clear_fatent(mydata, start_cluster); - if (ret) { - printf("Error: clearing FAT entries\n"); - goto exit; + ret = clear_fatent(mydata, start_cluster); + if (ret) { + printf("Error: clearing FAT entries\n"); + goto exit; + } + + if (!size) + set_start_cluster(mydata, retdent, 0); + } else if (size) { + ret = start_cluster = find_empty_cluster(mydata); + if (ret < 0) { + printf("Error: finding empty cluster\n"); + goto exit; + } + + ret = check_overflow(mydata, start_cluster, size); + if (ret) { + printf("Error: %llu overflow\n", size); + goto exit; + } + + set_start_cluster(mydata, retdent, start_cluster); } } else { /* Set short name to set alias checksum field in dir_slot */ set_name(empty_dentptr, filename); fill_dir_slot(mydata, &empty_dentptr, filename);
- ret = start_cluster = find_empty_cluster(mydata); - if (ret < 0) { - printf("Error: finding empty cluster\n"); - goto exit; - } + if (size) { + ret = start_cluster = find_empty_cluster(mydata); + if (ret < 0) { + printf("Error: finding empty cluster\n"); + goto exit; + }
- ret = check_overflow(mydata, start_cluster, size); - if (ret) { - printf("Error: %llu overflow\n", size); - goto exit; + ret = check_overflow(mydata, start_cluster, size); + if (ret) { + printf("Error: %llu overflow\n", size); + goto exit; + } + } else { + start_cluster = 0; }
/* Set attribute as archieve for regular file */
On Mon, Sep 28, 2015 at 03:45:32PM +0200, Benoît Thébaudeau wrote:
Overwriting an empty file not created by U-Boot did not work, and it could even corrupt the FAT. Moreover, creating empty files or emptying existing files allocated a cluster, which is not standard.
Fix this by always keeping empty files clusterless as specified by Microsoft (the start cluster must be set to 0 in the directory entry in that case), and by supporting overwriting such files.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
Applied to u-boot/master, thanks!
On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote:
set_cluster() was using a temporary buffer without enforcing its alignment for DMA and cache. Moreover, it did not check the alignment of the passed buffer, which can come directly from applicative code or from the user.
This could cause random data corruption, which has been observed on i.MX25 writing to an SD card.
Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to disk_write(), which requires the introduction of a buffer bouncing mechanism for the misaligned buffers passed to set_cluster().
By the way, improve the handling of the corresponding return values from disk_write():
- print them with debug() in case of error,
- consider that there is an error is disk_write() returns a smaller block count than the requested one, not only if its return value is negative.
After this change, set_cluster() and get_cluster() are almost symmetrical.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
OK. I know Stephen has a series to replace all of the FAT code for the next release once some performance issues are addressed. But I'm inclined to take this series (after some reviews and so forth) for this release at least because this sounds like some bad bugs and more things are starting to rely on fatwrite functionality (for example, env saved as a file in FAT is getting common on community-style boards).
Hi Tom,
On Mon, Sep 28, 2015 at 5:22 PM, Tom Rini trini@konsulko.com wrote:
On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote:
set_cluster() was using a temporary buffer without enforcing its alignment for DMA and cache. Moreover, it did not check the alignment of the passed buffer, which can come directly from applicative code or from the user.
This could cause random data corruption, which has been observed on i.MX25 writing to an SD card.
Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to disk_write(), which requires the introduction of a buffer bouncing mechanism for the misaligned buffers passed to set_cluster().
By the way, improve the handling of the corresponding return values from disk_write():
- print them with debug() in case of error,
- consider that there is an error is disk_write() returns a smaller block count than the requested one, not only if its return value is negative.
After this change, set_cluster() and get_cluster() are almost symmetrical.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
OK. I know Stephen has a series to replace all of the FAT code for the next release once some performance issues are addressed. But I'm inclined to take this series (after some reviews and so forth) for this release at least because this sounds like some bad bugs and more things are starting to rely on fatwrite functionality (for example, env saved as a file in FAT is getting common on community-style boards).
I agree, but there is not much review activity. Do you know anyone else who might be interested in this and who should be Cc'ed?
Best regards, Benoît
On Mon, Sep 28, 2015 at 03:45:28PM +0200, Benoît Thébaudeau wrote:
set_cluster() was using a temporary buffer without enforcing its alignment for DMA and cache. Moreover, it did not check the alignment of the passed buffer, which can come directly from applicative code or from the user.
This could cause random data corruption, which has been observed on i.MX25 writing to an SD card.
Fix this by only passing ARCH_DMA_MINALIGN-aligned buffers to disk_write(), which requires the introduction of a buffer bouncing mechanism for the misaligned buffers passed to set_cluster().
By the way, improve the handling of the corresponding return values from disk_write():
- print them with debug() in case of error,
- consider that there is an error is disk_write() returns a smaller block count than the requested one, not only if its return value is negative.
After this change, set_cluster() and get_cluster() are almost symmetrical.
Signed-off-by: Benoît Thébaudeau benoit@wsystem.com
Applied to u-boot/master, thanks!
participants (7)
-
Benoît Thébaudeau -
Benoît Thébaudeau -
Benoît Thébaudeau -
Mats Kärrman -
Ruud Commandeur -
Tom Rini -
Tom Rini