RE: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
> -Original Message- > From: Wood Scott-B07421 > Sent: Friday, October 15, 2010 0:01 AM > To: Zang Roy-R61911 > Cc: Wood Scott-B07421; Anton Vorontsov; linux-...@lists.infradead.org; > dw...@infradead.org; dedeki...@gmail.com; a...@linux-foundation.org; Lan > Chunhe-B25806; Gala Kumar-B11780; linuxppc-...@ozlabs.org; Hu Mingkai-B21284 > Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand > flash partitions > > On Wed, 13 Oct 2010 20:09:02 -0700 > "Zang Roy-R61911" wrote: > > > > > > > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL; > > > > > > > > > > No need for = NULL. > > > > Any harm? Or just personal habit or style? Can you explain about > > why? > > > > > > Besides not wanting superfluous code on general principle, it could > > > hide a bug if in the future the real initialization is missing on some > > > code path. It would become a runtime NULL dereference rather than a > > > compiler warning. > > > > Not exactly. > > Per my understand, if the pointer will definitely be assigned in code > > path, > > it is not necessary to init it when define. for example, > > > > char c; > > char b; > > char *a; > > if (condition) > > a = &c; > > else > > a = &b; > > ... > > > > for other case, if the path will not ensure the pointer assignment, it > > will be inited > > when define to avoid warning. for example, > > > > char c; > > char *a = NULL; > > if (condition) > > a = &c; > > ... > > Yes, but this patch looks like the former case, not the > latter. That is right. > Is GCC giving a warning without the initializer? no. we are on the same point. Thanks. Roy ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Wed, 13 Oct 2010 20:09:02 -0700 "Zang Roy-R61911" wrote: > > > > > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL; > > > > > > > > No need for = NULL. > > > Any harm? Or just personal habit or style? Can you explain about > why? > > > > Besides not wanting superfluous code on general principle, it could > > hide a bug if in the future the real initialization is missing on some > > code path. It would become a runtime NULL dereference rather than a > > compiler warning. > > Not exactly. > Per my understand, if the pointer will definitely be assigned in code > path, > it is not necessary to init it when define. for example, > > char c; > char b; > char *a; > if (condition) > a = &c; > else > a = &b; > ... > > for other case, if the path will not ensure the pointer assignment, it > will be inited > when define to avoid warning. for example, > > char c; > char *a = NULL; > if (condition) > a = &c; > ... Yes, but this patch looks like the former case, not the latter. Is GCC giving a warning without the initializer? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
> -Original Message-
> From: Anton Vorontsov [mailto:cbouatmai...@gmail.com]
> Sent: Monday, September 20, 2010 21:19 PM
> To: Zang Roy-R61911
> Cc: linux-...@lists.infradead.org; dw...@infradead.org; dedeki...@gmail.com;
> a...@linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421; Gala Kumar-
> B11780; linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand
> flash partitions
>
> On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
> [...]
> > +static struct mutex fsl_elbc_nand_mutex;
> > +
> > +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> > {
> > - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> > + struct fsl_lbc_regs __iomem *lbc;
> > struct fsl_elbc_mtd *priv;
> > struct resource res;
> > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
>
> No need for = NULL.
>
> [...]
> > - ctrl->chips[bank] = priv;
> > + mutex_init(&fsl_elbc_nand_mutex);
>
> This may cause all sorts of misbehaviours, e.g.
>
> A: mutex_init(foo)
> A: mutex_lock(foo)
> B: mutex_init(foo) <- destroyed "A"-context mutex.
> A: mutex_unlock(foo) <- oops
>
> Instead of dynamically initializing the mutex, just define it
> with DEFINE_MUTEX() above.
>
> (Btw, #include is needed.)
>
> > +
> > + mutex_lock(&fsl_elbc_nand_mutex);
> [...]
> > -static int __devinit fsl_elbc_ctrl_init(struct fsl_elbc_ctrl *ctrl)
> > +static int fsl_elbc_nand_remove(struct platform_device *dev)
> [...]
> > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
> [...]
> > + if (elbc_fcm_ctrl->chips[i])
> > + fsl_elbc_chip_remove(elbc_fcm_ctrl->chips[i]);
> [...]
> > + fsl_lbc_ctrl_dev->nand = NULL;
> > + kfree(elbc_fcm_ctrl);
>
> Will cause NULL dereference and/or use-after-free for other
> elbc nand instances. To avoid that, reference counting for
> elbc_fcm_ctrl is required.
Make sense.
will update.
Roy
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
> -Original Message-
> From: Wood Scott-B07421
> Sent: Monday, October 04, 2010 23:38 PM
> To: Zang Roy-R61911
> Cc: Anton Vorontsov; linux-...@lists.infradead.org;
dw...@infradead.org;
> dedeki...@gmail.com; a...@linux-foundation.org; Lan Chunhe-B25806;
Wood Scott-
> B07421; Gala Kumar-B11780; linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver
detect nand
> flash partitions
>
> On Sat, 2 Oct 2010 05:36:27 -0700
> "Zang Roy-R61911" wrote:
>
> >
> >
> > > -Original Message-
> > > From: Anton Vorontsov [mailto:cbouatmai...@gmail.com]
> > > Sent: Monday, September 20, 2010 21:19 PM
> > > To: Zang Roy-R61911
> > > Cc: linux-...@lists.infradead.org; dw...@infradead.org;
> dedeki...@gmail.com;
> > > a...@linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421;
Gala
> Kumar-
> > > B11780; linuxppc-...@ozlabs.org
> > > Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver
detect
> nand
> > > flash partitions
> > >
> > > On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
> > > [...]
> > > > +static struct mutex fsl_elbc_nand_mutex;
> > > > +
> > > > +static int __devinit fsl_elbc_nand_probe(struct platform_device
*dev)
> > > > {
> > > > - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> > > > + struct fsl_lbc_regs __iomem *lbc;
> > > > struct fsl_elbc_mtd *priv;
> > > > struct resource res;
> > > > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
> > >
> > > No need for = NULL.
> > Any harm? Or just personal habit or style? Can you explain about
why?
>
> Besides not wanting superfluous code on general principle, it could
> hide a bug if in the future the real initialization is missing on some
> code path. It would become a runtime NULL dereference rather than a
> compiler warning.
Not exactly.
Per my understand, if the pointer will definitely be assigned in code
path,
it is not necessary to init it when define. for example,
char c;
char b;
char *a;
if (condition)
a = &c;
else
a = &b;
...
for other case, if the path will not ensure the pointer assignment, it
will be inited
when define to avoid warning. for example,
char c;
char *a = NULL;
if (condition)
a = &c;
...
Thanks.
Roy
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Sat, 2 Oct 2010 05:36:27 -0700
"Zang Roy-R61911" wrote:
>
>
> > -Original Message-
> > From: Anton Vorontsov [mailto:cbouatmai...@gmail.com]
> > Sent: Monday, September 20, 2010 21:19 PM
> > To: Zang Roy-R61911
> > Cc: linux-...@lists.infradead.org; dw...@infradead.org; dedeki...@gmail.com;
> > a...@linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421; Gala Kumar-
> > B11780; linuxppc-...@ozlabs.org
> > Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect
> > nand
> > flash partitions
> >
> > On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
> > [...]
> > > +static struct mutex fsl_elbc_nand_mutex;
> > > +
> > > +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> > > {
> > > - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> > > + struct fsl_lbc_regs __iomem *lbc;
> > > struct fsl_elbc_mtd *priv;
> > > struct resource res;
> > > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
> >
> > No need for = NULL.
> Any harm? Or just personal habit or style? Can you explain about why?
Besides not wanting superfluous code on general principle, it could
hide a bug if in the future the real initialization is missing on some
code path. It would become a runtime NULL dereference rather than a
compiler warning.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
> -Original Message-
> From: Anton Vorontsov [mailto:cbouatmai...@gmail.com]
> Sent: Monday, September 20, 2010 21:19 PM
> To: Zang Roy-R61911
> Cc: linux-...@lists.infradead.org; dw...@infradead.org; dedeki...@gmail.com;
> a...@linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421; Gala Kumar-
> B11780; linuxppc-...@ozlabs.org
> Subject: Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand
> flash partitions
>
> On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
> [...]
> > +static struct mutex fsl_elbc_nand_mutex;
> > +
> > +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> > {
> > - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> > + struct fsl_lbc_regs __iomem *lbc;
> > struct fsl_elbc_mtd *priv;
> > struct resource res;
> > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
>
> No need for = NULL.
Any harm? Or just personal habit or style? Can you explain about why?
Thanks.
Roy
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
[...]
> +static struct mutex fsl_elbc_nand_mutex;
> +
> +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> {
> - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> + struct fsl_lbc_regs __iomem *lbc;
> struct fsl_elbc_mtd *priv;
> struct resource res;
> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
No need for = NULL.
[...]
> - ctrl->chips[bank] = priv;
> + mutex_init(&fsl_elbc_nand_mutex);
This may cause all sorts of misbehaviours, e.g.
A: mutex_init(foo)
A: mutex_lock(foo)
B: mutex_init(foo) <- destroyed "A"-context mutex.
A: mutex_unlock(foo) <- oops
Instead of dynamically initializing the mutex, just define it
with DEFINE_MUTEX() above.
(Btw, #include is needed.)
> +
> + mutex_lock(&fsl_elbc_nand_mutex);
[...]
> -static int __devinit fsl_elbc_ctrl_init(struct fsl_elbc_ctrl *ctrl)
> +static int fsl_elbc_nand_remove(struct platform_device *dev)
[...]
> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
[...]
> + if (elbc_fcm_ctrl->chips[i])
> + fsl_elbc_chip_remove(elbc_fcm_ctrl->chips[i]);
[...]
> + fsl_lbc_ctrl_dev->nand = NULL;
> + kfree(elbc_fcm_ctrl);
Will cause NULL dereference and/or use-after-free for other
elbc nand instances. To avoid that, reference counting for
elbc_fcm_ctrl is required.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
From: Jack Lan
The former driver had the two functions:
1. detecting nand flash partitions;
2. registering elbc interrupt.
Now, second function is removed to fsl_lbc.c.
Signed-off-by: Lan Chunhe-B25806
Signed-off-by: Roy Zang
---
drivers/mtd/nand/Kconfig |1 +
drivers/mtd/nand/fsl_elbc_nand.c | 477 +++---
2 files changed, 193 insertions(+), 285 deletions(-)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 8b4b67c..4132c46 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -458,6 +458,7 @@ config MTD_NAND_ORION
config MTD_NAND_FSL_ELBC
tristate "NAND support for Freescale eLBC controllers"
depends on PPC_OF
+ select FSL_LBC
help
Various Freescale chips, including the 8313, include a NAND Flash
Controller Module with built-in hardware ECC capabilities.
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index 80de0bf..76ffd24 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -1,9 +1,11 @@
/* Freescale Enhanced Local Bus Controller NAND driver
*
- * Copyright (c) 2006-2007 Freescale Semiconductor
+ * Copyright (c) 2006-2007, 2010 Freescale Semiconductor
*
* Authors: Nick Spence ,
* Scott Wood
+ * Jack Lan
+ * Roy Zang
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -27,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -42,14 +45,12 @@
#define ERR_BYTE 0xFF /* Value returned for read bytes when read failed */
#define FCM_TIMEOUT_MSECS 500 /* Maximum number of mSecs to wait for FCM */
-struct fsl_elbc_ctrl;
-
/* mtd information per set */
struct fsl_elbc_mtd {
struct mtd_info mtd;
struct nand_chip chip;
- struct fsl_elbc_ctrl *ctrl;
+ struct fsl_lbc_ctrl *ctrl;
struct device *dev;
int bank; /* Chip select bank number */
@@ -58,18 +59,12 @@ struct fsl_elbc_mtd {
unsigned int fmr; /* FCM Flash Mode Register value */
};
-/* overview of the fsl elbc controller */
+/* Freescale eLBC FCM controller infomation */
-struct fsl_elbc_ctrl {
+struct fsl_elbc_fcm_ctrl {
struct nand_hw_control controller;
struct fsl_elbc_mtd *chips[MAX_BANKS];
- /* device info */
- struct device *dev;
- struct fsl_lbc_regs __iomem *regs;
- int irq;
- wait_queue_head_t irq_wait;
- unsigned int irq_status; /* status read from LTESR by irq handler */
u8 __iomem *addr;/* Address of assigned FCM buffer*/
unsigned int page; /* Last page written to / read from */
unsigned int read_bytes; /* Number of bytes read during command */
@@ -164,11 +159,12 @@ static void set_addr(struct mtd_info *mtd, int column,
int page_addr, int oob)
{
struct nand_chip *chip = mtd->priv;
struct fsl_elbc_mtd *priv = chip->priv;
- struct fsl_elbc_ctrl *ctrl = priv->ctrl;
+ struct fsl_lbc_ctrl *ctrl = priv->ctrl;
struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
+ struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
int buf_num;
- ctrl->page = page_addr;
+ elbc_fcm_ctrl->page = page_addr;
out_be32(&lbc->fbar,
page_addr >> (chip->phys_erase_shift - chip->page_shift));
@@ -185,16 +181,18 @@ static void set_addr(struct mtd_info *mtd, int column,
int page_addr, int oob)
buf_num = page_addr & 7;
}
- ctrl->addr = priv->vbase + buf_num * 1024;
- ctrl->index = column;
+ elbc_fcm_ctrl->addr = priv->vbase + buf_num * 1024;
+ elbc_fcm_ctrl->index = column;
/* for OOB data point to the second half of the buffer */
if (oob)
- ctrl->index += priv->page_size ? 2048 : 512;
+ elbc_fcm_ctrl->index += priv->page_size ? 2048 : 512;
- dev_vdbg(ctrl->dev, "set_addr: bank=%d, ctrl->addr=0x%p (0x%p), "
+ dev_vdbg(priv->dev, "set_addr: bank=%d, "
+ "elbc_fcm_ctrl->addr=0x%p (0x%p), "
"index %x, pes %d ps %d\n",
-buf_num, ctrl->addr, priv->vbase, ctrl->index,
+buf_num, elbc_fcm_ctrl->addr, priv->vbase,
+elbc_fcm_ctrl->index,
chip->phys_erase_shift, chip->page_shift);
}
@@ -205,18 +203,19 @@ static int fsl_elbc_run_command(struct mtd_info *mtd)
{
struct nand_chip *chip = mtd->priv;
struct fsl_elbc_mtd *priv = chip->priv;
- struct fsl_elbc_ctrl *ctrl = priv->ctrl;
+ struct fsl_lbc_ctrl *ctrl = priv->ctrl;
+ struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = ctrl->nand;
struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
/* Setup the FMR[OP] to execute witho
