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

[Vignesh Raghavendra <vigneshr@xxxxxx>]

Hi,

On 28/10/19 8:15 PM, Sergei Shtylyov wrote:
> 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?
> 

I am fine either way. Because as per HyperFlash datasheet, PSB or ESB
will always be set if either of WASB or SLSB is set. So it does not
really matter if we are checking for just ESB and PSB or otherwise.

Regards
Vignesh

>>>  	}
>>> +	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/