| Message ID | 1382137374-21251-13-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Oct 18, 2013 at 08:02:39PM -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 95e2ce3..1ceccb6 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; > > 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; Do we really need to change it to atomic_t? IMHO, the write is also a atomic operation. thanks Huang Shijie
On Sun, Nov 03, 2013 at 06:03:39PM -0500, Huang Shijie wrote: > On Fri, Oct 18, 2013 at 08:02:39PM -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 95e2ce3..1ceccb6 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; > > > > 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; > Do we really need to change it to atomic_t? > > IMHO, the write is also a atomic operation. > I see. Well, if reading and setting an int type is atomic (at least on ARM), then of course we don't need the atomic_t. On the other side, it really shouldn't hurt too much and it emphasize the fact that it's accesed from the IRQ handler and the other callbacks (such as waitfunc). So, I'm still inclined to keeping it atomic. Does it sound stupid?
On Mon, Nov 4, 2013 at 5:51 AM, Ezequiel Garcia <ezequiel.garcia@free-electrons.com> wrote: > On Sun, Nov 03, 2013 at 06:03:39PM -0500, Huang Shijie wrote: >> On Fri, Oct 18, 2013 at 08:02:39PM -0300, Ezequiel Garcia wrote: >> > --- a/drivers/mtd/nand/pxa3xx_nand.c >> > +++ b/drivers/mtd/nand/pxa3xx_nand.c >> > @@ -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; >> Do we really need to change it to atomic_t? >> >> IMHO, the write is also a atomic operation. Atomicity is not an opinion :) > I see. Well, if reading and setting an int type is atomic > (at least on ARM), then of course we don't need the atomic_t. > > On the other side, it really shouldn't hurt too much and > it emphasize the fact that it's accesed from the IRQ handler > and the other callbacks (such as waitfunc). > > So, I'm still inclined to keeping it atomic. > > Does it sound stupid? No, it is not stupid. If nothing else, it is self-documenting, telling the world that this variable is accessed in racy situations, forcing developers to pay closer attention to it. And atomicity means more than simply "indivisible" (i.e., a single load instruction) in this context; I believe it helps prevent other non-thread-safe optimizations from being made by the compiler. That being said, the jury is still out on the need for the atomic flag + 2 completions in the first place (I brought this up a while ago but failed to follow up), but I'll hold my comments to the v3 patch. Brian
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 95e2ce3..1ceccb6 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; 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; 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); 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; @@ -750,6 +759,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); + atomic_set(&info->is_ready, 0); pxa3xx_nand_start(info); ret = wait_for_completion_timeout(&info->cmd_complete, @@ -862,21 +873,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)) { + ret = wait_for_completion_timeout(&info->dev_ready, + CHIP_DELAY_TIMEOUT); + 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, @@ -1027,7 +1044,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)) return 0; return -ENODEV;
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(-)