[PATCH v3 12/28] mtd: nand: pxa3xx: Use a completion to signal device ready
Brian Norris
computersforpeace at gmail.com
Tue Nov 5 14:51:36 EST 2013
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 at 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.
>
> 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
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.
> 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.
> + 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().
> + 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.
> + 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).
> return 0;
>
> return -ENODEV;
Brian
More information about the linux-arm-kernel
mailing list