[PATCH v2] mtd: nftl: reorganize operations in condition check

[PATCH v2] mtd: nftl: reorganize operations in condition check

[Andy Shevchenko]

We need to compare ret variable for negative value. The current code
assigns the boolean to the ret and prints it wrongly in the warning
message.

Reported-by: Andrey Karpov <karpov@viva64.com>
Cc: Giel van Schijndel <me@mortis.eu>
Cc: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/mtd/nftlmount.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nftlmount.c b/drivers/mtd/nftlmount.c
index 51b9d6a..a5dfbfb 100644
--- a/drivers/mtd/nftlmount.c
+++ b/drivers/mtd/nftlmount.c
@@ -89,9 +89,10 @@ static int find_boot_record(struct NFTLrecord *nftl)
 		}
 
 		/* To be safer with BIOS, also use erase mark as discriminant */
-		if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
+		ret = nftl_read_oob(mtd, block * nftl->EraseSize +
 					 SECTORSIZE + 8, 8, &retlen,
-					 (char *)&h1) < 0)) {
+					 (char *)&h1);
+		if (ret < 0) {
 			printk(KERN_WARNING "ANAND header found at 0x%x in mtd%d, but OOB data read failed (err %d)\n",
 			       block * nftl->EraseSize, nftl->mbd.mtd->index, ret);
 			continue;
@@ -109,8 +110,9 @@ static int find_boot_record(struct NFTLrecord *nftl)
 		}
 
 		/* Finally reread to check ECC */
-		if ((ret = mtd->read(mtd, block * nftl->EraseSize, SECTORSIZE,
-				     &retlen, buf) < 0)) {
+		ret = mtd->read(mtd, block * nftl->EraseSize, SECTORSIZE,
+				&retlen, buf);
+		if (ret < 0) {
 			printk(KERN_NOTICE "ANAND header found at 0x%x in mtd%d, but ECC read failed (err %d)\n",
 			       block * nftl->EraseSize, nftl->mbd.mtd->index, ret);
 			continue;
@@ -228,9 +230,11 @@ device is already correct.
 The new DiskOnChip driver already scanned the bad block table.  Just query it.
 			if ((i & (SECTORSIZE - 1)) == 0) {
 				/* read one sector for every SECTORSIZE of blocks */
-				if ((ret = mtd->read(nftl->mbd.mtd, block * nftl->EraseSize +
-						     i + SECTORSIZE, SECTORSIZE, &retlen,
-						     buf)) < 0) {
+				ret = mtd->read(nftl->mbd.mtd,
+						block * nftl->EraseSize + i +
+						SECTORSIZE, SECTORSIZE,
+						&retlen, buf);
+				if (ret < 0) {
 					printk(KERN_NOTICE "Read of bad sector table failed (err %d)\n",
 					       ret);
 					kfree(nftl->ReplUnitTable);
-- 
1.8.3.101.g727a46b

Re: [PATCH v2] mtd: nftl: reorganize operations in condition check

[Brian Norris]

On Wed, Jan 07, 2015 at 10:37:20PM +0200, Andy Shevchenko wrote:
> We need to compare ret variable for negative value. The current code
> assigns the boolean to the ret and prints it wrongly in the warning
> message.
> 
> Reported-by: Andrey Karpov <karpov@viva64.com>
> Cc: Giel van Schijndel <me@mortis.eu>
> Cc: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>

To be clear, this touches some commented out code (yuck). I think you
noted this previously.

For my reference, are you actually testing this driver?

Anyway, pushed to l2-mtd.git. Thanks.

Brian

Re: [PATCH v2] mtd: nftl: reorganize operations in condition check

[Andy Shevchenko]

On Sat, Jan 10, 2015 at 1:29 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Jan 07, 2015 at 10:37:20PM +0200, Andy Shevchenko wrote:
>> We need to compare ret variable for negative value. The current code
>> assigns the boolean to the ret and prints it wrongly in the warning
>> message.
>>
>> Reported-by: Andrey Karpov <karpov@viva64.com>
>> Cc: Giel van Schijndel <me@mortis.eu>
>> Cc: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
>> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> To be clear, this touches some commented out code (yuck). I think you
> noted this previously.

It had been proposed by Giel.

>
> For my reference, are you actually testing this driver?
>

Not a real testing. Only compilation on x86_32.

> Anyway, pushed to l2-mtd.git. Thanks.

Thanks!

>
> Brian



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] mtd: nftl: reorganize operations in condition check

[Giel van Schijndel]

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

On Sat, Jan 10, 2015 at 14:56:32 +0200, Andy Shevchenko wrote:
> On Sat, Jan 10, 2015 at 1:29 AM, Brian Norris <computersforpeace@gmail.com> wrote:
>> On Wed, Jan 07, 2015 at 10:37:20PM +0200, Andy Shevchenko wrote:
>>> We need to compare ret variable for negative value. The current code
>>> assigns the boolean to the ret and prints it wrongly in the warning
>>> message.
>>>
>>> Reported-by: Andrey Karpov <karpov@viva64.com>
>>> Cc: Giel van Schijndel <me@mortis.eu>
>>> Cc: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
>>> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>
>> To be clear, this touches some commented out code (yuck). I think you
>> noted this previously.
> 
> It had been proposed by Giel.

Yes, and I suspect (and hope) Brian's referring to the existence of
commented-out code as "yuck", not the removal of bugs from it.

>> For my reference, are you actually testing this driver?
> 
> Not a real testing. Only compilation on x86_32.

Combined with review I think that's enough given the nature of this
change.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] mtd: nftl: reorganize operations in condition check

[Brian Norris]

On Sat, Jan 10, 2015 at 09:11:13PM +0100, Giel van Schijndel wrote:
> On Sat, Jan 10, 2015 at 14:56:32 +0200, Andy Shevchenko wrote:
> > On Sat, Jan 10, 2015 at 1:29 AM, Brian Norris <computersforpeace@gmail.com> wrote:
> >> On Wed, Jan 07, 2015 at 10:37:20PM +0200, Andy Shevchenko wrote:
> >>> We need to compare ret variable for negative value. The current code
> >>> assigns the boolean to the ret and prints it wrongly in the warning
> >>> message.
> >>>
> >>> Reported-by: Andrey Karpov <karpov@viva64.com>
> >>> Cc: Giel van Schijndel <me@mortis.eu>
> >>> Cc: Dimitri Gorokhovik <dimitri.gorokhovik@free.fr>
> >>> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >>
> >> To be clear, this touches some commented out code (yuck). I think you
> >> noted this previously.
> > 
> > It had been proposed by Giel.
> 
> Yes, and I suspect (and hope) Brian's referring to the existence of
> commented-out code as "yuck", not the removal of bugs from it.

Yes, I was just commenting about the existence of all that dead code.
The code won't even compile now, as it's using some old interfaces.

> >> For my reference, are you actually testing this driver?
> > 
> > Not a real testing. Only compilation on x86_32.
> 
> Combined with review I think that's enough given the nature of this
> change.

Yeah, I wasn't objecting to fixing this trivial stuff. I just don't know
who uses some of the aging drivers we have in MTD.

Brian