Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

Hello!

On 10/16/2019 09:33 AM, Vignesh Raghavendra wrote:

>> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling
>> status register") added checking for the status register error bits into
>> chip_good() to only return 1 if these bits are zero. Unfortunately, this
>> means that polling using chip_good() always reaches a time-out condition
>> when erase or program failure bits are set. I think the status register
>> error checking should be fully delegated to cfi_check_err_status() that
>> should return whether any error bits were set or not...
>>
> 
> Please reword last sentence to drop "I think". Something like:
> 
> Lets fully delegate the function of determining error condition to
> cfi_check_err_status() and make chip_good() only look for Device
> Ready/Busy condition.

   OK. :-)

>> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>>
>> ---
>>  drivers/mtd/chips/cfi_cmdset_0002.c |   55 +++++++++++++++++++-----------------
>>  1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c
>> ===================================================================
>> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c
>> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi
>>  		(extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG;
>>  }
>>  
>> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip,
>> -				 unsigned long adr)
>> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip,
>> +				unsigned long adr)
>>  {
>>  	struct cfi_private *cfi = map->fldrv_priv;
>>  	map_word status;
>>  
>>  	if (!cfi_use_status_reg(cfi))
>> -		return;
>> +		return 0;
>>  
>>  	cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>>  			 cfi->device_type, NULL);
>> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct
>>  
>>  	/* The error bits are invalid while the chip's busy */
>>  	if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB)))
>> -		return;
>> +		return 0;
>>  
>>  	if (map_word_bitsset(map, status, CMD(0x3a))) {
>>  		unsigned long chipstatus = MERGESTATUS(status);
>> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct
>>  		if (chipstatus & CFI_SR_SLSB)
>>  			pr_err("%s sector write protected, status %lx\n",
>>  			       map->name, chipstatus);
>> +		return 1;

   So are you OK with extending the set of the error signalling bits I
did here, or I should really have accounted only for ESB and PSB bits
being error signals?

>>  	}
>> +	return 0;
>>  }
>>  
>>  /* #define DEBUG_CFI_FEATURES */
[...]
>> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc
> 
> Nit: for some reason, your diff has function names truncated abruptly
> which makes its slightly harder to locate the context. I use git
> format-patch that produces better readable contexts.

   I use quilt for development, not a big fan of git in this role. :-)

[...]
>> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait
>>  		 */
>>  		if (time_after(jiffies, timeo) &&
>>  		    !chip_good(map, chip, adr, datum)) {
>> +			pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n",
>> +				__func__, adr);
> 
> Since we are returning an error condition, this should be pr_err() (I
> know that rest of the file does not follow this convention, but lets
> make sure new code does)

   OK, I was looking at the other timeout code and failed to notice
that this printk() was converted to pr_err() by Ikegami-san... 

> Rest looks fine to me. Thanks for the patch!

   TY for the review.

MBR, Sergei

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/