| Message ID | 1466277252-13867-7-git-send-email-hauke@hauke-m.de |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 18 Jun 2016 21:14:10 +0200 Hauke Mehrtens <hauke@hauke-m.de> wrote: > From: John Crispin <john@phrozen.org> > > The external Bus Unit (EBU) can control different flash devices, but > these NAND flash commands have to be atomic and should not be > interrupted in between. Lock the EBU from the beginning of the command > till the end by moving the lock to the chip select. > > Signed-off-by: John Crispin <john@phrozen.org> > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > drivers/mtd/nand/xway_nand.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c > index 1511bdb..064ee41 100644 > --- a/drivers/mtd/nand/xway_nand.c > +++ b/drivers/mtd/nand/xway_nand.c > @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) > > static void xway_select_chip(struct mtd_info *mtd, int chip) > { > + static unsigned long csflags; > > switch (chip) { > case -1: > ltq_ebu_w32_mask(NAND_CON_CE, 0, EBU_NAND_CON); > ltq_ebu_w32_mask(NAND_CON_NANDM, 0, EBU_NAND_CON); > + spin_unlock_irqrestore(&ebu_lock, csflags); > break; > case 0: > + spin_lock_irqsave(&ebu_lock, csflags); > ltq_ebu_w32_mask(0, NAND_CON_NANDM, EBU_NAND_CON); > ltq_ebu_w32_mask(0, NAND_CON_CE, EBU_NAND_CON); So now the spinlock is taken (and irq disabled) during the whole NAND operation. Ouch! I hope you don't have real-time constraints in your system. > break; > @@ -114,7 +117,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) > struct nand_chip *chip = mtd_to_nand(mtd); > struct xway_nand_data *data = nand_get_controller_data(chip); > unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W; > - unsigned long flags; > > if (ctrl & NAND_CTRL_CHANGE) { > if (ctrl & NAND_CLE) > @@ -124,11 +126,9 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) > } > > if (cmd != NAND_CMD_NONE) { > - spin_lock_irqsave(&ebu_lock, flags); > writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd)); > while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0) > ; > - spin_unlock_irqrestore(&ebu_lock, flags); > } > } > > @@ -141,14 +141,8 @@ static unsigned char xway_read_byte(struct mtd_info *mtd) > { > struct nand_chip *this = mtd_to_nand(mtd); > unsigned long nandaddr = (unsigned long) this->IO_ADDR_R; > - unsigned long flags; > - int ret; > - > - spin_lock_irqsave(&ebu_lock, flags); > - ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA)); > - spin_unlock_irqrestore(&ebu_lock, flags); > > - return ret; > + return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA)); > } > > > @@ -156,26 +150,20 @@ static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len) > { > struct nand_chip *this = mtd_to_nand(mtd); > unsigned long nandaddr = (unsigned long) this->IO_ADDR_R; > - unsigned long flags; > int i; > > - spin_lock_irqsave(&ebu_lock, flags); > for (i = 0; i < len; i++) > buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA)); > - spin_unlock_irqrestore(&ebu_lock, flags); > } > > static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len) > { > struct nand_chip *this = mtd_to_nand(mtd); > unsigned long nandaddr = (unsigned long) this->IO_ADDR_W; > - unsigned long flags; > int i; > > - spin_lock_irqsave(&ebu_lock, flags); > for (i = 0; i < len; i++) > ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA)); > - spin_unlock_irqrestore(&ebu_lock, flags); > } > > /*
Am 19.06.2016 um 14:41 schrieb Boris Brezillon: > On Sat, 18 Jun 2016 21:14:10 +0200 > Hauke Mehrtens <hauke@hauke-m.de> wrote: > >> From: John Crispin <john@phrozen.org> >> >> The external Bus Unit (EBU) can control different flash devices, but >> these NAND flash commands have to be atomic and should not be >> interrupted in between. Lock the EBU from the beginning of the command >> till the end by moving the lock to the chip select. >> >> Signed-off-by: John Crispin <john@phrozen.org> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> drivers/mtd/nand/xway_nand.c | 20 ++++---------------- >> 1 file changed, 4 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c >> index 1511bdb..064ee41 100644 >> --- a/drivers/mtd/nand/xway_nand.c >> +++ b/drivers/mtd/nand/xway_nand.c >> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) >> >> static void xway_select_chip(struct mtd_info *mtd, int chip) >> { >> + static unsigned long csflags; Why is csflags static? AFAICT this is racy then... Thanks, //richard
On 06/19/2016 02:53 PM, Richard Weinberger wrote: > Am 19.06.2016 um 14:41 schrieb Boris Brezillon: >> On Sat, 18 Jun 2016 21:14:10 +0200 >> Hauke Mehrtens <hauke@hauke-m.de> wrote: >> >>> From: John Crispin <john@phrozen.org> >>> >>> The external Bus Unit (EBU) can control different flash devices, but >>> these NAND flash commands have to be atomic and should not be >>> interrupted in between. Lock the EBU from the beginning of the command >>> till the end by moving the lock to the chip select. >>> >>> Signed-off-by: John Crispin <john@phrozen.org> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>> --- >>> drivers/mtd/nand/xway_nand.c | 20 ++++---------------- >>> 1 file changed, 4 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c >>> index 1511bdb..064ee41 100644 >>> --- a/drivers/mtd/nand/xway_nand.c >>> +++ b/drivers/mtd/nand/xway_nand.c >>> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) >>> >>> static void xway_select_chip(struct mtd_info *mtd, int chip) >>> { >>> + static unsigned long csflags; > > Why is csflags static? AFAICT this is racy then... Because the lock is taken when the chip is selected and it is unlocked when the chip select is deactivated again. This should be part of the private driver struct. Hauke
On Sun, 19 Jun 2016 14:53:40 +0200 Richard Weinberger <richard@nod.at> wrote: > Am 19.06.2016 um 14:41 schrieb Boris Brezillon: > > On Sat, 18 Jun 2016 21:14:10 +0200 > > Hauke Mehrtens <hauke@hauke-m.de> wrote: > > > >> From: John Crispin <john@phrozen.org> > >> > >> The external Bus Unit (EBU) can control different flash devices, but > >> these NAND flash commands have to be atomic and should not be > >> interrupted in between. Lock the EBU from the beginning of the command > >> till the end by moving the lock to the chip select. > >> > >> Signed-off-by: John Crispin <john@phrozen.org> > >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > >> --- > >> drivers/mtd/nand/xway_nand.c | 20 ++++---------------- > >> 1 file changed, 4 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c > >> index 1511bdb..064ee41 100644 > >> --- a/drivers/mtd/nand/xway_nand.c > >> +++ b/drivers/mtd/nand/xway_nand.c > >> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) > >> > >> static void xway_select_chip(struct mtd_info *mtd, int chip) > >> { > >> + static unsigned long csflags; > > Why is csflags static? AFAICT this is racy then... Oh, nice catch! It's definitely wrong. > > Thanks, > //richard
On Sun, 19 Jun 2016 14:56:03 +0200 Hauke Mehrtens <hauke@hauke-m.de> wrote: > On 06/19/2016 02:53 PM, Richard Weinberger wrote: > > Am 19.06.2016 um 14:41 schrieb Boris Brezillon: > >> On Sat, 18 Jun 2016 21:14:10 +0200 > >> Hauke Mehrtens <hauke@hauke-m.de> wrote: > >> > >>> From: John Crispin <john@phrozen.org> > >>> > >>> The external Bus Unit (EBU) can control different flash devices, but > >>> these NAND flash commands have to be atomic and should not be > >>> interrupted in between. Lock the EBU from the beginning of the command > >>> till the end by moving the lock to the chip select. > >>> > >>> Signed-off-by: John Crispin <john@phrozen.org> > >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > >>> --- > >>> drivers/mtd/nand/xway_nand.c | 20 ++++---------------- > >>> 1 file changed, 4 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c > >>> index 1511bdb..064ee41 100644 > >>> --- a/drivers/mtd/nand/xway_nand.c > >>> +++ b/drivers/mtd/nand/xway_nand.c > >>> @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) > >>> > >>> static void xway_select_chip(struct mtd_info *mtd, int chip) > >>> { > >>> + static unsigned long csflags; > > > > Why is csflags static? AFAICT this is racy then... > > Because the lock is taken when the chip is selected and it is unlocked > when the chip select is deactivated again. This should be part of the > private driver struct. static here means that you have a single instance for all callers. This is working fine because you have a lock at the NAND core level, and you only have a single controller, but you definitely don't want to rely on that in the long run. In any case, using a static variable to save irqflags sounds like a bad idea. This should always be saved on the stack to limit the time spent in spin_lock_irqsave()/spin_unlock_irqrestored() critical sections. Which bring us back to my first question: are you sure you want to disable irqs for such a long/unbounded time?
diff --git a/drivers/mtd/nand/xway_nand.c b/drivers/mtd/nand/xway_nand.c index 1511bdb..064ee41 100644 --- a/drivers/mtd/nand/xway_nand.c +++ b/drivers/mtd/nand/xway_nand.c @@ -94,13 +94,16 @@ static void xway_reset_chip(struct nand_chip *chip) static void xway_select_chip(struct mtd_info *mtd, int chip) { + static unsigned long csflags; switch (chip) { case -1: ltq_ebu_w32_mask(NAND_CON_CE, 0, EBU_NAND_CON); ltq_ebu_w32_mask(NAND_CON_NANDM, 0, EBU_NAND_CON); + spin_unlock_irqrestore(&ebu_lock, csflags); break; case 0: + spin_lock_irqsave(&ebu_lock, csflags); ltq_ebu_w32_mask(0, NAND_CON_NANDM, EBU_NAND_CON); ltq_ebu_w32_mask(0, NAND_CON_CE, EBU_NAND_CON); break; @@ -114,7 +117,6 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) struct nand_chip *chip = mtd_to_nand(mtd); struct xway_nand_data *data = nand_get_controller_data(chip); unsigned long nandaddr = (unsigned long) chip->IO_ADDR_W; - unsigned long flags; if (ctrl & NAND_CTRL_CHANGE) { if (ctrl & NAND_CLE) @@ -124,11 +126,9 @@ static void xway_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) } if (cmd != NAND_CMD_NONE) { - spin_lock_irqsave(&ebu_lock, flags); writeb(cmd, (void __iomem *) (nandaddr | data->xway_latchcmd)); while ((ltq_ebu_r32(EBU_NAND_WAIT) & NAND_WAIT_WR_C) == 0) ; - spin_unlock_irqrestore(&ebu_lock, flags); } } @@ -141,14 +141,8 @@ static unsigned char xway_read_byte(struct mtd_info *mtd) { struct nand_chip *this = mtd_to_nand(mtd); unsigned long nandaddr = (unsigned long) this->IO_ADDR_R; - unsigned long flags; - int ret; - - spin_lock_irqsave(&ebu_lock, flags); - ret = ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA)); - spin_unlock_irqrestore(&ebu_lock, flags); - return ret; + return ltq_r8((void __iomem *)(nandaddr + NAND_READ_DATA)); } @@ -156,26 +150,20 @@ static void xway_read_buf(struct mtd_info *mtd, u_char *buf, int len) { struct nand_chip *this = mtd_to_nand(mtd); unsigned long nandaddr = (unsigned long) this->IO_ADDR_R; - unsigned long flags; int i; - spin_lock_irqsave(&ebu_lock, flags); for (i = 0; i < len; i++) buf[i] = ltq_r8((void __iomem *)(nandaddr | NAND_READ_DATA)); - spin_unlock_irqrestore(&ebu_lock, flags); } static void xway_write_buf(struct mtd_info *mtd, const u_char *buf, int len) { struct nand_chip *this = mtd_to_nand(mtd); unsigned long nandaddr = (unsigned long) this->IO_ADDR_W; - unsigned long flags; int i; - spin_lock_irqsave(&ebu_lock, flags); for (i = 0; i < len; i++) ltq_w8(buf[i], (void __iomem *)(nandaddr | NAND_WRITE_DATA)); - spin_unlock_irqrestore(&ebu_lock, flags); } /*