| Message ID | 1246022509-16194-1-git-send-email-tom.leiming@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2009-06-26 at 21:21 +0800, tom.leiming@gmail.com wrote: > From: Ming Lei <tom.leiming@gmail.com> > > drivers/mtd/nand/omap2.c: In function 'omap_wait': > drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this function) > drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported only once > drivers/mtd/nand/omap2.c:543: error: for each function it appears in.) > drivers/mtd/nand/omap2.c:557: error: implicit declaration of function 'time_before' OK. > ...... > > drivers/mtd/nand/omap2.c: In function 'omap_wait': > drivers/mtd/nand/omap2.c:545: warning: 'status' may be used uninitialized in this function Hm, why do you set it to zero? In the (fairly unlikely) case where you never go through the loop at all, because you're preempted until the timeout has completed, what value do you want it to return? Don't just make changes by bashing on the keyboard until the compiler shuts up; please apply brain. That way, I think you'll notice that the return value from this function is completely screwed anyway.... Vimal? Shouldn't it be returning NAND_STATUS_FAIL in the failure case, not a value read directly from the hardware register? And shouldn't there be a cpu_relax() (or even schedule) in there too? > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > drivers/mtd/nand/omap2.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index 0cd76f8..2905c79 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -15,6 +15,7 @@ > #include <linux/mtd/nand.h> > #include <linux/mtd/partitions.h> > #include <linux/io.h> > +#include <linux/jiffies.h> > > #include <asm/dma.h> > > @@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip) > struct omap_nand_info *info = container_of(mtd, struct omap_nand_info, > mtd); > unsigned long timeo = jiffies; > - int status, state = this->state; > + int state = this->state; > + int status = 0; > > if (state == FL_ERASING) > timeo += (HZ * 400) / 1000;
On Fri, Jun 26, 2009 at 8:39 PM, David Woodhouse<dwmw2@infradead.org> wrote: > On Fri, 2009-06-26 at 21:21 +0800, tom.leiming@gmail.com wrote: >> From: Ming Lei <tom.leiming@gmail.com> >> >> drivers/mtd/nand/omap2.c: In function 'omap_wait': >> drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this function) >> drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported only once >> drivers/mtd/nand/omap2.c:543: error: for each function it appears in.) >> drivers/mtd/nand/omap2.c:557: error: implicit declaration of function 'time_before' > > OK. > >> ...... >> >> drivers/mtd/nand/omap2.c: In function 'omap_wait': >> drivers/mtd/nand/omap2.c:545: warning: 'status' may be used uninitialized in this function > > Hm, why do you set it to zero? In the (fairly unlikely) case where you > never go through the loop at all, because you're preempted until the > timeout has completed, what value do you want it to return? > > Don't just make changes by bashing on the keyboard until the compiler > shuts up; please apply brain. > > That way, I think you'll notice that the return value from this function > is completely screwed anyway.... Vimal? Shouldn't it be returning > NAND_STATUS_FAIL in the failure case, not a value read directly from the > hardware register? And shouldn't there be a cpu_relax() (or even > schedule) in there too? That's correct David. I think 'cond_resched()' would be fine here. > >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> --- >> drivers/mtd/nand/omap2.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c >> index 0cd76f8..2905c79 100644 >> --- a/drivers/mtd/nand/omap2.c >> +++ b/drivers/mtd/nand/omap2.c >> @@ -15,6 +15,7 @@ >> #include <linux/mtd/nand.h> >> #include <linux/mtd/partitions.h> >> #include <linux/io.h> >> +#include <linux/jiffies.h> >> >> #include <asm/dma.h> >> >> @@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip) >> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info, >> mtd); >> unsigned long timeo = jiffies; >> - int status, state = this->state; >> + int state = this->state; >> + int status = 0; >> >> if (state == FL_ERASING) >> timeo += (HZ * 400) / 1000; > -- > David Woodhouse Open Source Technology Centre > David.Woodhouse@intel.com Intel Corporation > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ --- Vimal
On Fri, 2009-06-26 at 21:15 +0530, Singh, Vimal wrote: > > That's correct David. I think 'cond_resched()' would be fine here. Got a patch? I have you down as the person responsible for this driver, given that jzhang@ti.com is bouncing...
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index 0cd76f8..2905c79 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -15,6 +15,7 @@ #include <linux/mtd/nand.h> #include <linux/mtd/partitions.h> #include <linux/io.h> +#include <linux/jiffies.h> #include <asm/dma.h> @@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip) struct omap_nand_info *info = container_of(mtd, struct omap_nand_info, mtd); unsigned long timeo = jiffies; - int status, state = this->state; + int state = this->state; + int status = 0; if (state == FL_ERASING) timeo += (HZ * 400) / 1000;