[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[Josh Wu]

Tested in at91sam9x5ek and sama5d3x-ek in mmc fat.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
When I debugging this issue, I also find the thread talking about this:
  http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158698
But finally no patch is send for this bug. So I send this and cc all the
people in that thread. Hope it can be merged in 2013.07.

 fs/fat/fat_write.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 5829adf..b7ba8ea 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -561,7 +561,7 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
 	     unsigned long size)
 {
 	int idx = 0;
-	__u32 startsect;
+	__u32 startsect, nr_sectors;
 
 	if (clustnum > 0)
 		startsect = mydata->data_begin +
@@ -571,7 +571,11 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
 
 	debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
 
-	if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
+	nr_sectors = size / mydata->sect_size;
+	if (nr_sectors == 0)
+		nr_sectors = 1;
+
+	if (disk_write(startsect, nr_sectors, buffer) < 0) {
 		debug("Error writing data\n");
 		return -1;
 	}
-- 
1.7.9.5

[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[Wolfgang Denk]

Dear Josh Wu,

In message <1374134870-10154-1-git-send-email-josh.wu@atmel.com> you wrote:
> Tested in at91sam9x5ek and sama5d3x-ek in mmc fat.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> When I debugging this issue, I also find the thread talking about this:
>   http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158698
> But finally no patch is send for this bug. So I send this and cc all the
> people in that thread. Hope it can be merged in 2013.07.

Definitely not.  This needs thurough testing before it can go
mainline.


> -	__u32 startsect;
> +	__u32 startsect, nr_sectors;
>  
>  	if (clustnum > 0)
>  		startsect = mydata->data_begin +
> @@ -571,7 +571,11 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
>  
>  	debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
>  
> -	if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
> +	nr_sectors = size / mydata->sect_size;
> +	if (nr_sectors == 0)
> +		nr_sectors = 1;

Why would this be needed?  The case of a partial write is covered
further down below in the code, in the "if (size % mydata->sect_size)"
part...



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I still miss my ex-wife, but my aim is getting better.

[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[Josh Wu]

Dear Wolfgang Denk,

Thank you for the quick reply.

On 7/18/2013 4:24 PM, Wolfgang Denk wrote:
> Dear Josh Wu,
>
> In message <1374134870-10154-1-git-send-email-josh.wu@atmel.com> you wrote:
>> Tested in at91sam9x5ek and sama5d3x-ek in mmc fat.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> When I debugging this issue, I also find the thread talking about this:
>>    http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158698
>> But finally no patch is send for this bug. So I send this and cc all the
>> people in that thread. Hope it can be merged in 2013.07.
> Definitely not.  This needs thurough testing before it can go
> mainline.

got it.

>
>
>> -	__u32 startsect;
>> +	__u32 startsect, nr_sectors;
>>   
>>   	if (clustnum > 0)
>>   		startsect = mydata->data_begin +
>> @@ -571,7 +571,11 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
>>   
>>   	debug("clustnum: %d, startsect: %d\n", clustnum, startsect);
>>   
>> -	if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
>> +	nr_sectors = size / mydata->sect_size;
>> +	if (nr_sectors == 0)
>> +		nr_sectors = 1;
> Why would this be needed?  The case of a partial write is covered
> further down below in the code, in the "if (size % mydata->sect_size)"
> part...

The call of disk_write() will pass the sector size as 0. That will cause 
the mmc driver stalled.

And after a search, the fix patch is already sent by Ruud Commandeur and 
merge in mainline:
   "mmc write bug fix"
   commit a586c0aa211fb79ecaa06aee3299bfdd81329876
   Author: Ruud Commandeur <RCommandeur@clb.nl>
   Date:   Wed May 22 13:19:43 2013 +0200

So please forget this patch. Since it is fixed in mmc driver layer. I 
will test it in my board.
Sorry for the noise.

>
>
>
> Best regards,
>
> Wolfgang Denk
>

Best Regards,
Josh Wu

[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[Wolfgang Denk]

Dear Josh Wu,

In message <51E90A3D.6000007@atmel.com> you wrote:
> 
> > Why would this be needed?  The case of a partial write is covered
> > further down below in the code, in the "if (size % mydata->sect_size)"
> > part...
> 
> The call of disk_write() will pass the sector size as 0. That will cause 
> the mmc driver stalled.

I think instead of "sector size" you mean the number of sectors.  The
sector size is fixed.

Big question: why should we call disk_write() at all when the count is
zero? Maybe a simple

	if ((size / mydata->sect_size) > 0) {
		if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
			...
		}
	}

would fix this issue?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Der Irrtum wiederholt sich immerfort in der Tat.  Deshalb mu? man das
Wahre unerm?dlich in Worten wiederholen.                     - Goethe

[U-Boot] [PATCH] fs: fat: fix bug when write size is less than a sector size

[Josh Wu]

Hi, Dear Wolfgang Denk

On 7/19/2013 7:20 PM, Wolfgang Denk wrote:
> Dear Josh Wu,
>
> In message <51E90A3D.6000007@atmel.com> you wrote:
>>> Why would this be needed?  The case of a partial write is covered
>>> further down below in the code, in the "if (size % mydata->sect_size)"
>>> part...
>> The call of disk_write() will pass the sector size as 0. That will cause
>> the mmc driver stalled.
> I think instead of "sector size" you mean the number of sectors.  The
> sector size is fixed.

yes, I mean the number of sectors.

>
> Big question: why should we call disk_write() at all when the count is
> zero? Maybe a simple
>
> 	if ((size / mydata->sect_size) > 0) {
> 		if (disk_write(startsect, size / mydata->sect_size, buffer) < 0) {
> 			...
> 		}
> 	}
>
> would fix this issue?

I agree with above changes. It make the logical clearer:
    Step 1. write buffer in sectors. If buffer size is less than one 
sector we don't need to call disk_write() with zero sector.
    Step 2. write remain buffer in one sector.

So I think I will send a v2 patch according to your suggestion. Thanks.

>
>
> Best regards,
>
> Wolfgang Denk
>

Best Regards,
Josh Wu