[U-Boot] fatwrite problem

[U-Boot] fatwrite problem

[Ruud Commandeur]

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 at 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

[U-Boot] fatwrite problem

[Mats Kärrman]

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

[U-Boot] fatwrite problem

[Ruud Commandeur]

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 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

[U-Boot] fatwrite problem

[Tom Rini]

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!

-- 
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/9e6efd04/attachment.pgp>

[U-Boot] fatwrite problem

[Ruud Commandeur]

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 at 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 at 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 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!

-- 
Tom

[U-Boot] fatwrite problem

[Tom Rini]

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!

-- 
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/80e0bb89/attachment.pgp>

[U-Boot] fatwrite problem

[Benoît Thébaudeau]

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,
 - empty file creation.

Best regards,
Beno?t

[U-Boot] fatwrite problem

[Tom Rini]

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>

[U-Boot] fatwrite problem

[Benoît Thébaudeau]

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

[U-Boot] fatwrite problem

[Ruud Commandeur]

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

[U-Boot] fatwrite problem

[Ruud Commandeur]

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 at 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 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

[U-Boot] fatwrite problem

[Tom Rini]

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!

-- 
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/20130514/6444f936/attachment.pgp>

[U-Boot] [PATCH 1/5] fs/fat/fat_write: Fix buffer alignments

[Benoît Thébaudeau]

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;
-- 
2.1.4

[U-Boot] [PATCH 2/5] fs/fat/fat_write: Merge calls to set_cluster()

[Benoît Thébaudeau]

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;
 		}
-- 
2.1.4

[U-Boot] [U-Boot, 2/5] fs/fat/fat_write: Merge calls to set_cluster()

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151012/d2f0c786/attachment.sig>

[U-Boot] [PATCH 3/5] fs/fat/fat_write: Fix curclust/newclust mix-up

[Benoît Thébaudeau]

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;
 		}
-- 
2.1.4

[U-Boot] [U-Boot, 3/5] fs/fat/fat_write: Fix curclust/newclust mix-up

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151012/67547bd6/attachment.sig>

[U-Boot] [PATCH 4/5] fs/fat/fat_write: Factor out duplicate code

[Benoît Thébaudeau]

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;
-- 
2.1.4

[U-Boot] [U-Boot, 4/5] fs/fat/fat_write: Factor out duplicate code

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151012/f0661c42/attachment.sig>

[U-Boot] [PATCH 5/5] fs/fat/fat_write: Fix management of empty files

[Benoît Thébaudeau]

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 */
-- 
2.1.4

[U-Boot] [U-Boot, 5/5] fs/fat/fat_write: Fix management of empty files

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151012/fd6b878f/attachment.sig>

[U-Boot] [PATCH 1/5] fs/fat/fat_write: Fix buffer alignments

[Tom Rini]

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).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150928/b03b5a69/attachment.sig>

[U-Boot] [PATCH 1/5] fs/fat/fat_write: Fix buffer alignments

[Benoît Thébaudeau]

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

[U-Boot] [U-Boot,1/5] fs/fat/fat_write: Fix buffer alignments

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151012/a75c427c/attachment.sig>