Re: [PATCH v3] mtd:nor:timeout:fix do_write_buffer() timeout error

[Brian Norris <computersforpeace@gmail.com>]

+ Linux-MTD

Please do not remove the MTD mailing list from your email. I am much
more likely to ignore it that way.

Also, please don't top post. I'm top-posting right now, because I can't
understand your thread very easily, and it's hard to resurrect for
sending to the mailing list anyway.

Regarding Stijn's comments: if you have good reason to believe that some
flash's CFI parameter will be non-zero, but incorrect, then perhaps you
could enforce a minimum value.

Brian

On Fri, Jul 11, 2014 at 12:18:40AM +0000, Bean Huo wrote:
> hi,Brian Norris
>     are you a maintainer of MTD? please help me and please give me a result,thanks!
> 
> ________________________________
> > Date: Thu, 10 Jul 2014 11:53:50 +0200 
> > Subject: Re: [PATCH v3] mtd:nor:timeout:fix do_write_buffer() timeout error 
> > From: highguy@gmail.com 
> > To: beanhuo@outlook.com 
> > CC: dwmw2@infradead.org; computersforpeace@gmail.com; 
> > paul.gortmaker@windriver.com 
> > 
> > On 10/07/2014 10:53, Bean Huo wrote: 
> > 
> > hi, 
> > thanks for your response! 
> > please look at the file cfi_cmdset_0001.c,It also use timeout value of CFI. 
> > But the difference with my patch is that if timeout value is not defined in the CFI,the default timeout value is 500000us. 
> > Do you mean that the default timeout value extend moure than 2MS? 
> > 
> > if (cfi->cfiq->BufWriteTimeoutTyp && 
> > cfi->cfiq->BufWriteTimeoutMax) 
> > cfi->chips[i].buffer_write_time_max = 
> > 1<<(cfi->cfiq->BufWriteTimeoutTyp + 
> > cfi->cfiq->BufWriteTimeoutMax); 
> > 
> > 
> > Good question. I don't think I can answer that one. Perhaps one of the MTD 
> > maintainers can. 
> > 
> > Regards, 
> > Stijn 
> > 
> > ________________________________ 
> > 
> > 
> > Date: Thu, 10 Jul 2014 08:33:16 +0200 
> > Subject: Re: [PATCH v3] mtd:nor:timeout:fix do_write_buffer() timeout error 
> > From: highguy@gmail.com<mailto:highguy@gmail.com> 
> > To: beanhuo@outlook.com<mailto:beanhuo@outlook.com> 
> > CC: dwmw2@infradead.org<mailto:dwmw2@infradead.org>; computersforpeace@gmail.com<mailto:computersforpeace@gmail.com>; 
> > paul.gortmaker@windriver.com<mailto:paul.gortmaker@windriver.com> 
> > 
> > 
> > On Thu, Jul 10, 2014 at 4:22 AM, Bean Huo 
> > <beanhuo@outlook.com<mailto:beanhuo@outlook.com><mailto:beanhuo@outlook.com><mailto:beanhuo@outlook.com>> wrote: 
> > 
> > hi,Stijn 
> > please see below about my answer: 
> > 
> > 
> > ________________________________ 
> > 
> > 
> > Date: Wed, 9 Jul 2014 10:03:02 +0200 
> > Subject: Re: [PATCH v3] mtd:nor:timeout:fix do_write_buffer() timeout error 
> > From: highguy@gmail.com<mailto:highguy@gmail.com><mailto:highguy@gmail.com><mailto:highguy@gmail.com> 
> > To: beanhuo@outlook.com<mailto:beanhuo@outlook.com><mailto:beanhuo@outlook.com><mailto:beanhuo@outlook.com> 
> > CC: dwmw2@infradead.org<mailto:dwmw2@infradead.org><mailto:dwmw2@infradead.org><mailto:dwmw2@infradead.org>; 
> > 
> > 
> > computersforpeace@gmail.com<mailto:computersforpeace@gmail.com><mailto:computersforpeace@gmail.com><mailto:computersforpeace@gmail.com>; 
> > 
> > 
> > paul.gortmaker@windriver.com<mailto:paul.gortmaker@windriver.com><mailto:paul.gortmaker@windriver.com><mailto:paul.gortmaker@windriver.com> 
> > 
> > Hi Bean, 
> > 
> > If I'm not mistaken the 2ms was also there because manufacturers 
> > 
> > 
> > don't always 
> > 
> > 
> > fill out the CFI data properly. I've seen incorrect values for other 
> > fields, so I'd be 
> > hesitant to trust the data. 
> > 
> > My proposition would be 3-fold: 
> > - first read the data from CFI 
> > 
> > 
> > this has been done in my patch.timeout value will be firstly pre-read 
> > from CFI. 
> > 
> > 
> > 
> > - use the value max(value, 2ms) 
> > 
> > 
> > in my patch,if CFI don't exsit this area value,and do_write_buffer max 
> > timeout will still be 2Ms 
> > 
> > 
> > But now you risk breaking flashes with incorrect timeout value <2ms. 
> > Those flashes would have 
> > worked fine, but by using the incorrect CFI value now, they'll silently 
> > break. 
> > 
> > 
> > 
> > - perhaps an override list (aka shame-the-mfr-list) for those flashes 
> > with known incorrect values 
> > 
> > 
> > I think,your worry is completely unnecessary,if flash with incorrect 
> > values ih this area, 
> > this is related to the quality of the flash,not the linux kernel.But,on 
> > the contrary,if forsome flashes with correct timeout 
> > value(because the size of the buffer program has been increased from 
> > 256 Bytes to 512 Bytes,timeout value is greater than 2MS) 
> > in this area,and we now still use default max timeout 
> > 2Ms,sometimes,this will lead to buffer-write operation failed.I have 
> > found this case 
> > in some flashes,also for this reason,I now submit this patch. 
> > 
> > 
> > If flashes have incorrect values, then this IS a worry for the linux kernel. 
> > People don't stop using these flashes because the CFI values are faulty, they 
> > expect S/W to workaround it. That is what the kernel has done until now. 
> > Have a look at the file pci-quirks.c. A whole file dedicated to fixing up 
> > crappy hardware. It's the kernel's job to make things work, no 
> > questions asked. 
> > (although we get to shame the vendors in public). 
> > 
> > Regards, 
> > Stijn 
> > 
> > 
> > 
> > You could probably implement 1&2 with almost no hassle, 3 would probably be 
> > for the first flash that has incorrect values (and needs over 2ms timeout). 
> > 
> > Regards, 
> > Stijn 
> > 
> > 
> > 
> >