[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready

[ezequiel.garcia@free-electrons.com (Ezequiel Garcia)]

On Tue, Nov 05, 2013 at 11:51:36AM -0800, Brian Norris wrote:
> On Tue, Nov 05, 2013 at 09:55:19AM -0300, Ezequiel Garcia wrote:
> > Apparently, the expected behavior of the waitfunc() NAND chip call
> > is to wait for the device to be READY (this is a standard chip line).
> > However, the current implementation does almost nothing, which opens
> > a possibility to issue a command to a non-ready device.
> > 
> > Fix this by adding a new completion to wait for the ready event to arrive.
> > 
> > Because the "is ready" flag is cleared from the controller status
> > register, it's needed to store that state in the driver, and because the
> > field is accesed from an interruption, the field needs to be of an
> > atomic type.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/pxa3xx_nand.c | 45 +++++++++++++++++++++++++++++-------------
> >  1 file changed, 31 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index 2fb0f38..e198c94 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -35,6 +35,7 @@
> >  
> >  #include <linux/platform_data/mtd-nand-pxa3xx.h>
> >  
> > +#define NAND_DEV_READY_TIMEOUT  50
> >  #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
> >  #define NAND_STOP_DELAY		(2 * HZ/50)
> >  #define PAGE_CHUNK_SIZE		(2048)
> > @@ -166,7 +167,7 @@ struct pxa3xx_nand_info {
> >  	struct clk		*clk;
> >  	void __iomem		*mmio_base;
> >  	unsigned long		mmio_phys;
> > -	struct completion	cmd_complete;
> > +	struct completion	cmd_complete, dev_ready;
> 
> I still kinda think this could be consolidated into one completion,
> under which cmdfunc() performs all the necessary waiting (for both the
> "command complete" and "device ready" signals), but I think this is not
> a big advantage right now, considering your code is not too complex
> right now.
> 

Quite frankly, I'd love such solution (given this adds a lot of
complexity) but I can't see how. Maybe restarting the command done completion
at the end of cmdfunc() so waitfunc() would re-use it?

> >  
> >  	unsigned int 		buf_start;
> >  	unsigned int		buf_count;
> > @@ -196,7 +197,13 @@ struct pxa3xx_nand_info {
> >  	int			use_ecc;	/* use HW ECC ? */
> >  	int			use_dma;	/* use DMA ? */
> >  	int			use_spare;	/* use spare ? */
> > -	int			is_ready;
> > +
> > +	/*
> > +	 * The is_ready flag is accesed from several places,
> > +	 * including an interruption hander. We need an atomic
> > +	 * type to avoid races.
> > +	 */
> > +	atomic_t		is_ready;
> 
> I believe your handling of this 'is_ready' bit is a little unwise, as
> you are actually creating extra concurrency that is unneeded. I'll
> summarize what you're doing with this 'is_ready' field:
> 
>   cmdfunc() -> sets info->is_ready=0 for appropriate commands
>             -> kicks off the hardware
> 
> The following two sequences may then occur concurrently:
> 
>   (1) pxa3xx_nand_irq -> ready interrupt occurs
>                       -> set info->is_ready=1
> 		      -> signal 'dev_ready' completion
> 
>   (2) waitfunc -> check info->is_ready, if it is 0...
>                   |_ ... wait for the dev_ready completion
> 
> Instead of setting info->is_ready=1 under (1), you could set it in (2),
> after the completion (or timeout). This avoids the concurrency, since
> cmdfunc() and waitfunc() are sequential. This also avoids a benign race
> in which you may not even call wait_for_completion() in (2) in the
> following scenario:
> 
>   * Suppose waitfunc is delayed a long time
>   * The IRQ handler...
>     - receives the 'ready' interrupt
>     - clears info->is_ready
>     - calls complete(&info->dev_ready)
>   * waitfunc() finally executes
>     - because info->is_ready==1, it skips the wait_for_completion...(),
>       leaving your completion unbalanced
> 

Right.

> This influences a comment I have below regarding your re-initialization
> of the completion struct.
> 
> >  
> >  	unsigned int		fifo_size;	/* max. data size in the FIFO */
> >  	unsigned int		data_size;	/* data to be read from FIFO */
> > @@ -478,7 +485,7 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
> >  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  {
> >  	struct pxa3xx_nand_info *info = devid;
> > -	unsigned int status, is_completed = 0;
> > +	unsigned int status, is_completed = 0, is_ready = 0;
> >  	unsigned int ready, cmd_done;
> >  
> >  	if (info->cs == 0) {
> > @@ -514,8 +521,9 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  		is_completed = 1;
> >  	}
> >  	if (status & ready) {
> > -		info->is_ready = 1;
> > +		atomic_set(&info->is_ready, 1);
> 
> According to my suggestions, I don't think you need to set
> info->is_ready=1 here.
> 

Well, I'm not sure there's other place where this can be done, as it's the
only place where the status register is read. After this, the register
is cleared and the information is lost. (On the other side, see below)

I guess that if we could simply read the status at any time, this would be
much simpler (iow, we would read the status register in waitfunc).

> >  		info->state = STATE_READY;
> > +		is_ready = 1;
> >  	}
> >  
> >  	if (status & NDSR_WRCMDREQ) {
> > @@ -544,6 +552,8 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
> >  	nand_writel(info, NDSR, status);
> >  	if (is_completed)
> >  		complete(&info->cmd_complete);
> > +	if (is_ready)
> > +		complete(&info->dev_ready);
> >  NORMAL_IRQ_EXIT:
> >  	return IRQ_HANDLED;
> >  }
> > @@ -574,7 +584,6 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
> >  	info->oob_size		= 0;
> >  	info->use_ecc		= 0;
> >  	info->use_spare		= 1;
> > -	info->is_ready		= 0;
> >  	info->retcode		= ERR_NONE;
> >  	if (info->cs != 0)
> >  		info->ndcb0 = NDCB0_CSEL;
> > @@ -747,6 +756,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
> >  	exec_cmd = prepare_command_pool(info, command, column, page_addr);
> >  	if (exec_cmd) {
> >  		init_completion(&info->cmd_complete);
> > +		init_completion(&info->dev_ready);
> 
> Do you really need to init the completions each time you run a command?
> AIUI, the only reason you would need to do this is if you aren't
> matching up your calls to complete() and wait_for_completion*()
> properly, so that you simply dodge the issue and reset the completion
> count each time. This might be a result of the complexity of your
> 2-completion signalling design.
> 

Exactly, I need to reset any previous statue.

> > +		atomic_set(&info->is_ready, 0);
> >  		pxa3xx_nand_start(info);
> >  
> >  		ret = wait_for_completion_timeout(&info->cmd_complete,
> > @@ -859,21 +870,27 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
> >  {
> >  	struct pxa3xx_nand_host *host = mtd->priv;
> >  	struct pxa3xx_nand_info *info = host->info_data;
> > +	int ret;
> > +
> > +	/* Need to wait? */
> > +	if (!atomic_read(&info->is_ready)) {
> 
> This read of info->is_ready will no longer need to be atomic, because
> you never modify it from the IRQ context--only from the cmdfunc() and
> waitfunc().
> 

I see.

> > +		ret = wait_for_completion_timeout(&info->dev_ready,
> > +				CHIP_DELAY_TIMEOUT);
> 
> I think you can just do a (non-atomic) info->is_ready=1 here.
> 

The problem is: not every cmdfunc() is followed by a waitfunc(), right?
And even if it would be, cmdfunc() can be accesed from other places
(such as this same driver) so how do you guarantee cmdfunc() and
waitfunc() are called in pair?

Or maybe you're right and this is not an issue? I'm doing some tests
now and we'll report back later.

> > +		if (!ret) {
> > +			dev_err(&info->pdev->dev, "Ready time out!!!\n");
> > +			return NAND_STATUS_FAIL;
> > +		}
> > +	}
> >  
> >  	/* pxa3xx_nand_send_command has waited for command complete */
> >  	if (this->state == FL_WRITING || this->state == FL_ERASING) {
> >  		if (info->retcode == ERR_NONE)
> >  			return 0;
> > -		else {
> > -			/*
> > -			 * any error make it return 0x01 which will tell
> > -			 * the caller the erase and write fail
> > -			 */
> > -			return 0x01;
> > -		}
> > +		else
> > +			return NAND_STATUS_FAIL;
> >  	}
> >  
> > -	return 0;
> > +	return NAND_STATUS_READY;
> >  }
> >  
> >  static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
> > @@ -1026,7 +1043,7 @@ static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
> >  		return ret;
> >  
> >  	chip->cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
> > -	if (info->is_ready)
> > +	if (atomic_read(&info->is_ready))
> 
> Does this need to wait on the dev_ready completion? I'm not sure I can
> guarantee there is no race on info->is_ready here, but then that means
> I'm not sure the code is correct even with the atomic read (which I
> don't think will be necessary, according to my suggestion above).
> 

Hm... yes, this probably should use instead of accessing is_ready.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com