Skip to contentSkip to conversation

mtd: nand: silence some shift wrap warnings

PATCH Applied Dan Carpenter 4 participants 5 messages Aug 9, 2013 – Aug 16, 2013

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mtd: nand: silence some shift wrap warnings

There are static checkers which complain when we declare variables as 64 bit bitfields but only use the lower 32 bits because of shift wrapping. In this case "len" is declared as u64 as opposed to unsigned long or something which might be 32 bits.

drivers/mtd/nand/nand_base.c +4 -4
index 9a48758..3eddd04 100644
@@ -108,13 +108,13 @@ static int check_offs_len(struct mtd_info *mtd,
 	int ret = 0;
 
 	/* Start address must align on block boundary */
-	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
+	if (ofs & ((1ULL << chip->phys_erase_shift) - 1)) {
 		pr_debug("%s: unaligned address\n", __func__);
 		ret = -EINVAL;
 	}
 
 	/* Length must align on block boundary */
-	if (len & ((1 << chip->phys_erase_shift) - 1)) {
+	if (len & ((1ULL << chip->phys_erase_shift) - 1)) {
 		pr_debug("%s: length not block aligned\n", __func__);
 		ret = -EINVAL;
 	}
@@ -394,7 +394,7 @@ static int nand_block_markbad_lowlevel(struct mtd_info *mtd, loff_t ofs)
 		memset(&einfo, 0, sizeof(einfo));
 		einfo.mtd = mtd;
 		einfo.addr = ofs;
-		einfo.len = 1 << chip->phys_erase_shift;
+		einfo.len = 1ULL << chip->phys_erase_shift;
 		nand_erase_nand(mtd, &einfo, 0);
 
 		/* Write bad block marker to OOB */
@@ -2630,7 +2630,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 		}
 
 		/* Increment page address and decrement length */
-		len -= (1 << chip->phys_erase_shift);
+		len -= (1ULL << chip->phys_erase_shift);
 		page += pages_per_block;
 
 		/* Check, if we cross a chip boundary */
#1 lore raw
Quoted 1 line · Dan Carpenter

There are static checkers which complain when we declare variables as

could you tell me which "checker" and how do you test it?

you'd better post the warning message here.

thanks
Huang Shijie

Quoted 45 lines

64 bit bitfields but only use the lower 32 bits because of shift wrapping. In this case "len" is declared as u64 as opposed to unsigned long or something which might be 32 bits.

drivers/mtd/nand/nand_base.c +4 -4
index 9a48758..3eddd04 100644
@@ -108,13 +108,13 @@ static int check_offs_len(struct mtd_info *mtd,
  	int ret = 0;
 
  	/* Start address must align on block boundary */
-	if (ofs&  ((1<<  chip->phys_erase_shift) - 1)) {
+	if (ofs&  ((1ULL<<  chip->phys_erase_shift) - 1)) {
  		pr_debug("%s: unaligned address\n", __func__);
  		ret = -EINVAL;
  	}
 
  	/* Length must align on block boundary */
-	if (len&  ((1<<  chip->phys_erase_shift) - 1)) {
+	if (len&  ((1ULL<<  chip->phys_erase_shift) - 1)) {
  		pr_debug("%s: length not block aligned\n", __func__);
  		ret = -EINVAL;
  	}
@@ -394,7 +394,7 @@ static int nand_block_markbad_lowlevel(struct mtd_info *mtd, loff_t ofs)
  		memset(&einfo, 0, sizeof(einfo));
  		einfo.mtd = mtd;
  		einfo.addr = ofs;
-		einfo.len = 1<<  chip->phys_erase_shift;
+		einfo.len = 1ULL<<  chip->phys_erase_shift;
  		nand_erase_nand(mtd,&einfo, 0);
 
  		/* Write bad block marker to OOB */
@@ -2630,7 +2630,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
  		}
 
  		/* Increment page address and decrement length */
-		len -= (1<<  chip->phys_erase_shift);
+		len -= (1ULL<<  chip->phys_erase_shift);
  		page += pages_per_block;
 
  		/* Check, if we cross a chip boundary */
#2 lore raw
Quoted 7 lines · Huang Shijie
Quoted 1 line · Dan Carpenter

There are static checkers which complain when we declare variables as

could you tell me which "checker" and how do you test it?

you'd better post the warning message here.

It's some stuff I'm working on but haven't released yet. But "len" and "ofs" are declared as u64 and loff_t. If they were declared as unsigned long then maybe the current code would be fine because the code would be correct-ish on 32 bit arches.

So this is definitely the right thing to do.

regards,
dan carpenter

#3 lore raw
Quoted 10 lines · Dan Carpenter
Quoted 7 lines · Huang Shijie
Quoted 1 line · Dan Carpenter

There are static checkers which complain when we declare variables as

could you tell me which "checker" and how do you test it?

you'd better post the warning message here.

It's some stuff I'm working on but haven't released yet. But "len"

Which static checker isn't all that important, so no worries. The patch can stand on its own merits.

Quoted 3 lines

and "ofs" are declared as u64 and loff_t. If they were declared as unsigned long then maybe the current code would be fine because the code would be correct-ish on 32 bit arches.

IIUC, it's still correct, since we surely will never have an eraseblock size near 4GB (i.e., if the shift is ever larger than 31, we have a bug somewhere else).

Quoted 1 line

So this is definitely the right thing to do.

As far as types are concerned, yes, this is still the right thing to do.

Brian

#4 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

There are static checkers which complain when we declare variables as 64 bit bitfields but only use the lower 32 bits because of shift wrapping. In this case "len" is declared as u64 as opposed to unsigned long or something which might be 32 bits.

Pushed to l2-mtd.git, thanks!

Signature · 2 lines
Best Regards,
Artem Bityutskiy