| Message ID | 1374134870-10154-1-git-send-email-josh.wu@atmel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
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
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
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
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
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; }
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(-)