smatch stuff: mtd/cafe_nand: cafe_device_ready() always returns true

smatch stuff: mtd/cafe_nand: cafe_device_ready() always returns true

[Dan Carpenter]

Hi David,

cafe_device_ready() always returns 1.  It looks like something else was
intended but I don't know the code enough to say what should go there.

drivers/mtd/nand/cafe_nand.c +107 cafe_device_ready(3)
	warn: condition is always true

   104  static int cafe_device_ready(struct mtd_info *mtd)
   105  {
   106          struct cafe_priv *cafe = mtd->priv;
   107          int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
                                                             ^^^^^^^^^^^^^
	This bit is always non-zero because we take the result of
	cafe_readl() and do a bitwize or with 0x40000000.  Then the
	double negate means that result is always 1.

   108          uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
   109

regards,
dan carpenter

Re: smatch stuff: mtd/cafe_nand: cafe_device_ready() always returns true

[Dan Carpenter]

On 1/3/11, Dan Carpenter <error27@gmail.com> wrote:
> Hi David,
>
> cafe_device_ready() always returns 1.  It looks like something else was
> intended but I don't know the code enough to say what should go there.
>

Probably we should just change the | to an &.  I'll send a patch for that.

regards,
dan carpenter

> drivers/mtd/nand/cafe_nand.c +107 cafe_device_ready(3)
> 	warn: condition is always true
>
>    104  static int cafe_device_ready(struct mtd_info *mtd)
>    105  {
>    106          struct cafe_priv *cafe = mtd->priv;
>    107          int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
>                                                              ^^^^^^^^^^^^^
> 	This bit is always non-zero because we take the result of
> 	cafe_readl() and do a bitwize or with 0x40000000.  Then the
> 	double negate means that result is always 1.
>
>    108          uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
>    109
>
> regards,
> dan carpenter
>
>

[patch] mtd: cafe_nand: fix an & vs | mistake

[Dan Carpenter]

The intent here was clearly to set result to true if the 0x40000000 flag
was set.  But instead there was a | vs & typo and we always set result
to true.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I do not have the hardware to test this.  The original code is clearly
buggy, but what about if 0x40000000 is the wrong flag?

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index f3594a6..ac0d967 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -102,7 +102,7 @@ static const char *part_probes[] = { "cmdlinepart", "RedBoot", NULL };
 static int cafe_device_ready(struct mtd_info *mtd)
 {
 	struct cafe_priv *cafe = mtd->priv;
-	int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
+	int result = !!(cafe_readl(cafe, NAND_STATUS) & 0x40000000);
 	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
 
 	cafe_writel(cafe, irqs, NAND_IRQ);

[patch] mtd: cafe_nand: fix an & vs | mistake

[Dan Carpenter]

The intent here was clearly to set result to true if the 0x40000000 flag
was set.  But instead there was a | vs & typo and we always set result
to true.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I do not have the hardware to test this.  The original code is clearly
buggy, but what about if 0x40000000 is the wrong flag?

diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
index f3594a6..ac0d967 100644
--- a/drivers/mtd/nand/cafe_nand.c
+++ b/drivers/mtd/nand/cafe_nand.c
@@ -102,7 +102,7 @@ static const char *part_probes[] = { "cmdlinepart", "RedBoot", NULL };
 static int cafe_device_ready(struct mtd_info *mtd)
 {
 	struct cafe_priv *cafe = mtd->priv;
-	int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
+	int result = !!(cafe_readl(cafe, NAND_STATUS) & 0x40000000);
 	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
 
 	cafe_writel(cafe, irqs, NAND_IRQ);

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Sat, 2012-06-09 at 19:08 +0300, Dan Carpenter wrote:
> The intent here was clearly to set result to true if the 0x40000000 flag
> was set.  But instead there was a | vs & typo and we always set result
> to true.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I do not have the hardware to test this.  The original code is clearly
> buggy, but what about if 0x40000000 is the wrong flag?

Looks like it is the right flag, I've checked

wiki.laptop.org/images/5/5c/88ALP01_Datasheet_July_2007.pdf

and your patch looks correct.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Sat, 2012-06-09 at 19:08 +0300, Dan Carpenter wrote:
> The intent here was clearly to set result to true if the 0x40000000 flag
> was set.  But instead there was a | vs & typo and we always set result
> to true.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I do not have the hardware to test this.  The original code is clearly
> buggy, but what about if 0x40000000 is the wrong flag?

Looks like it is the right flag, I've checked

wiki.laptop.org/images/5/5c/88ALP01_Datasheet_July_2007.pdf

and your patch looks correct.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Pushed this patch to l2-mtd.git and added CC to -stable. The patch seems
to be a real bug-fix. Daniel, could you please check it - it will be
shame if it hits -stable and breaks something.

Thanks!

On Sat, 2012-06-09 at 19:08 +0300, Dan Carpenter wrote:
> The intent here was clearly to set result to true if the 0x40000000 flag
> was set.  But instead there was a | vs & typo and we always set result
> to true.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I do not have the hardware to test this.  The original code is clearly
> buggy, but what about if 0x40000000 is the wrong flag?
> 
> diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
> index f3594a6..ac0d967 100644
> --- a/drivers/mtd/nand/cafe_nand.c
> +++ b/drivers/mtd/nand/cafe_nand.c
> @@ -102,7 +102,7 @@ static const char *part_probes[] = { "cmdlinepart", "RedBoot", NULL };
>  static int cafe_device_ready(struct mtd_info *mtd)
>  {
>  	struct cafe_priv *cafe = mtd->priv;
> -	int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
> +	int result = !!(cafe_readl(cafe, NAND_STATUS) & 0x40000000);
>  	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
>  
>  	cafe_writel(cafe, irqs, NAND_IRQ);
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Pushed this patch to l2-mtd.git and added CC to -stable. The patch seems
to be a real bug-fix. Daniel, could you please check it - it will be
shame if it hits -stable and breaks something.

Thanks!

On Sat, 2012-06-09 at 19:08 +0300, Dan Carpenter wrote:
> The intent here was clearly to set result to true if the 0x40000000 flag
> was set.  But instead there was a | vs & typo and we always set result
> to true.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I do not have the hardware to test this.  The original code is clearly
> buggy, but what about if 0x40000000 is the wrong flag?
> 
> diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
> index f3594a6..ac0d967 100644
> --- a/drivers/mtd/nand/cafe_nand.c
> +++ b/drivers/mtd/nand/cafe_nand.c
> @@ -102,7 +102,7 @@ static const char *part_probes[] = { "cmdlinepart", "RedBoot", NULL };
>  static int cafe_device_ready(struct mtd_info *mtd)
>  {
>  	struct cafe_priv *cafe = mtd->priv;
> -	int result = !!(cafe_readl(cafe, NAND_STATUS) | 0x40000000);
> +	int result = !!(cafe_readl(cafe, NAND_STATUS) & 0x40000000);
>  	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
>  
>  	cafe_writel(cafe, irqs, NAND_IRQ);
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Daniel Drake]

On Mon, Jun 18, 2012 at 5:25 AM, Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
> Pushed this patch to l2-mtd.git and added CC to -stable. The patch seems
> to be a real bug-fix. Daniel, could you please check it - it will be
> shame if it hits -stable and breaks something.

Thanks for catching this, sorry for the delay in looking closely at
it. I double-checked the device specifications and indeed, Dan's
interpretation is correct.

I also tested the patch. System still remains functional and basic
nand performance seems to be the same.

Thanks!
Daniel

Re: [patch] mtd: cafe_nand: fix an & vs | mistake

[Daniel Drake]

On Mon, Jun 18, 2012 at 5:25 AM, Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
> Pushed this patch to l2-mtd.git and added CC to -stable. The patch seems
> to be a real bug-fix. Daniel, could you please check it - it will be
> shame if it hits -stable and breaks something.

Thanks for catching this, sorry for the delay in looking closely at
it. I double-checked the device specifications and indeed, Dan's
interpretation is correct.

I also tested the patch. System still remains functional and basic
nand performance seems to be the same.

Thanks!
Daniel