Re: [PATCH v4 4/9] nand: spi: add basic blocks for infrastructure

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 30 Mar 2017 14:25:41 +0200
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 23/03/2017 17:33, Marek Vasut wrote:
> > On 03/23/2017 04:40 PM, Boris Brezillon wrote:  
> >> On Thu, 23 Mar 2017 12:29:58 +0100
> >> Marek Vasut <marex@denx.de> wrote:
> >>
> >>  
> >>>> +
> >>>> +/*
> >>>> + * spinand_read_status - get status register value
> >>>> + * @chip: SPI NAND device structure
> >>>> + * @status: buffer to store value
> >>>> + * Description:
> >>>> + *   After read, write, or erase, the NAND device is expected to set the
> >>>> + *   busy status.
> >>>> + *   This function is to allow reading the status of the command: read,
> >>>> + *   write, and erase.
> >>>> + */
> >>>> +static int spinand_read_status(struct spinand_device *chip, uint8_t *status)
> >>>> +{
> >>>> +	return spinand_read_reg(chip, REG_STATUS, status);
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * spinand_wait - wait until the command is done
> >>>> + * @chip: SPI NAND device structure
> >>>> + * @s: buffer to store status register value (can be NULL)
> >>>> + */
> >>>> +static int spinand_wait(struct spinand_device *chip, u8 *s)
> >>>> +{
> >>>> +	unsigned long timeo = msecs_to_jiffies(400);
> >>>> +	u8 status;
> >>>> +
> >>>> +	do {
> >>>> +		spinand_read_status(chip, &status);
> >>>> +		if ((status & STATUS_OIP_MASK) == STATUS_READY)
> >>>> +			goto out;
> >>>> +	} while (time_before(jiffies, timeo));
> >>>> +
> >>>> +	/*
> >>>> +	 * Extra read, just in case the STATUS_READY bit has changed
> >>>> +	 * since our last check
> >>>> +	 */  
> >>> Is this likely to happen ? Probably not ... so in case you reach this
> >>> place here, timeout happened.  
> >> If the timeout is big enough, no. But it does not hurt to do one last
> >> check.  
> > 400 mSec is not big enough ? It's huge ... this seems like a duplication
> > to me. btw the ad-hoc 400 mSec delay value should be replaced with a macro.
> >  
> In fact, there is a bug:
> 
> > unsigned long timeo = msecs_to_jiffies(400);
> > [...]
> > do { } while (time_before(jiffies, timeo));  
> 
> The condition is almost true except during the 5 minutes following the boot (before jiffies wrap around).
> So, only one status read is done, which give a high chance of returning a timeout status.
> I guess you should do : 	
> 
> 	unsigned long timeo = jiffies + msecs_to_jiffies(400);
> 
> I also suspect that this is the reason why you have an additional status read.

Oh, nice catch!