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

Boris Brezillon
2017-03-30T12:52:26Z

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!