[PATCH 0/4] debug, printk cleanup

[PATCH 0/4] debug, printk cleanup

[Brian Norris]

These patches come from discussion at:
http://lists.infradead.org/pipermail/linux-mtd/2011-June/036161.html

They are based on the mtd-2.6 git tree, plus a few of the cleanup
patches already in Artem's l2 git tree.

Note:
The 4th patch adds the __func__ prefix to some print messages that
didn't have them previously. Thus, I'd be fine if the 4th patch is
dropped.

Thanks,
Brian

Brian Norris (4):
  mtd: remove printk's for [kv][mz]alloc failures
  mtd: nand: convert printk() to pr_*()
  mtd: nand: replace DEBUG() with dev_dbg()
  mtd: nand: define pr_fmt() to include __func__ in debug output

 drivers/mtd/cmdlinepart.c         |    3 -
 drivers/mtd/inftlcore.c           |    4 +-
 drivers/mtd/nand/nand_base.c      |   90 ++++++++++++++++++------------------
 drivers/mtd/nand/nand_bbt.c       |   66 +++++++++++---------------
 drivers/mtd/nand/rtc_from4.c      |    1 -
 drivers/mtd/nftlcore.c            |    4 +-
 drivers/mtd/onenand/onenand_bbt.c |    4 +-
 drivers/mtd/ssfdc.c               |   10 +---
 8 files changed, 78 insertions(+), 104 deletions(-)

[PATCH 1/4] mtd: remove printk's for [kv][mz]alloc failures

[Brian Norris]

When a memory allocation fails, the kernel will print out a backtrace
automatically. These print statements are unnecessary.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/cmdlinepart.c         |    3 ---
 drivers/mtd/inftlcore.c           |    4 +---
 drivers/mtd/nand/nand_bbt.c       |   13 +++----------
 drivers/mtd/nand/rtc_from4.c      |    1 -
 drivers/mtd/nftlcore.c            |    4 +---
 drivers/mtd/onenand/onenand_bbt.c |    4 +---
 drivers/mtd/ssfdc.c               |   10 ++--------
 7 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index e790f38..be0c121 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -188,10 +188,7 @@ static struct mtd_partition * newpart(char *s,
 			     extra_mem_size;
 		parts = kzalloc(alloc_size, GFP_KERNEL);
 		if (!parts)
-		{
-			printk(KERN_ERR ERRP "out of memory\n");
 			return NULL;
-		}
 		extra_mem = (unsigned char *)(parts + *num_parts);
 	}
 	/* enter this partition (offset will be calculated later if it is zero at this point) */
diff --git a/drivers/mtd/inftlcore.c b/drivers/mtd/inftlcore.c
index d7592e6..c9a31e6 100644
--- a/drivers/mtd/inftlcore.c
+++ b/drivers/mtd/inftlcore.c
@@ -67,10 +67,8 @@ static void inftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 
 	inftl = kzalloc(sizeof(*inftl), GFP_KERNEL);
 
-	if (!inftl) {
-		printk(KERN_WARNING "INFTL: Out of memory for data structures\n");
+	if (!inftl)
 		return;
-	}
 
 	inftl->mbd.mtd = mtd;
 	inftl->mbd.devnum = -1;
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ba40166..8875d6d 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1121,10 +1121,8 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	 * table.
 	 */
 	this->bbt = kzalloc(len, GFP_KERNEL);
-	if (!this->bbt) {
-		printk(KERN_ERR "nand_scan_bbt: Out of memory\n");
+	if (!this->bbt)
 		return -ENOMEM;
-	}
 
 	/*
 	 * If no primary table decriptor is given, scan the device to build a
@@ -1146,7 +1144,6 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	len += (len >> this->page_shift) * mtd->oobsize;
 	buf = vmalloc(len);
 	if (!buf) {
-		printk(KERN_ERR "nand_bbt: Out of memory\n");
 		kfree(this->bbt);
 		this->bbt = NULL;
 		return -ENOMEM;
@@ -1195,10 +1192,8 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
 	len = (1 << this->bbt_erase_shift);
 	len += (len >> this->page_shift) * mtd->oobsize;
 	buf = kmalloc(len, GFP_KERNEL);
-	if (!buf) {
-		printk(KERN_ERR "nand_update_bbt: Out of memory\n");
+	if (!buf)
 		return -ENOMEM;
-	}
 
 	writeops = md != NULL ? 0x03 : 0x01;
 
@@ -1307,10 +1302,8 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
 		return -EINVAL;
 	}
 	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
-	if (!bd) {
-		printk(KERN_ERR "nand_create_default_bbt_descr: Out of memory\n");
+	if (!bd)
 		return -ENOMEM;
-	}
 	bd->options = this->bbt_options;
 	bd->offs = this->badblockpos;
 	bd->len = (this->options & NAND_BUSWIDTH_16) ? 2 : 1;
diff --git a/drivers/mtd/nand/rtc_from4.c b/drivers/mtd/nand/rtc_from4.c
index c9f9127..a07da2a 100644
--- a/drivers/mtd/nand/rtc_from4.c
+++ b/drivers/mtd/nand/rtc_from4.c
@@ -444,7 +444,6 @@ static int rtc_from4_errstat(struct mtd_info *mtd, struct nand_chip *this,
 		len = mtd->writesize;
 		buf = kmalloc(len, GFP_KERNEL);
 		if (!buf) {
-			printk(KERN_ERR "rtc_from4_errstat: Out of memory!\n");
 			er_stat = 1;
 			goto out;
 		}
diff --git a/drivers/mtd/nftlcore.c b/drivers/mtd/nftlcore.c
index b155666..f3b3239 100644
--- a/drivers/mtd/nftlcore.c
+++ b/drivers/mtd/nftlcore.c
@@ -67,10 +67,8 @@ static void nftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 
 	nftl = kzalloc(sizeof(struct NFTLrecord), GFP_KERNEL);
 
-	if (!nftl) {
-		printk(KERN_WARNING "NFTL: out of memory for data structures\n");
+	if (!nftl)
 		return;
-	}
 
 	nftl->mbd.mtd = mtd;
 	nftl->mbd.devnum = -1;
diff --git a/drivers/mtd/onenand/onenand_bbt.c b/drivers/mtd/onenand/onenand_bbt.c
index fc2c16a..09a7d1f 100644
--- a/drivers/mtd/onenand/onenand_bbt.c
+++ b/drivers/mtd/onenand/onenand_bbt.c
@@ -188,10 +188,8 @@ int onenand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	len = this->chipsize >> (this->erase_shift + 2);
 	/* Allocate memory (2bit per block) and clear the memory bad block table */
 	bbm->bbt = kzalloc(len, GFP_KERNEL);
-	if (!bbm->bbt) {
-		printk(KERN_ERR "onenand_scan_bbt: Out of memory\n");
+	if (!bbm->bbt)
 		return -ENOMEM;
-	}
 
 	/* Set the bad block position */
 	bbm->badblockpos = ONENAND_BADBLOCK_POS;
diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
index 5cd1897..00d1405 100644
--- a/drivers/mtd/ssfdc.c
+++ b/drivers/mtd/ssfdc.c
@@ -304,11 +304,8 @@ static void ssfdcr_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 		return;
 
 	ssfdc = kzalloc(sizeof(struct ssfdcr_record), GFP_KERNEL);
-	if (!ssfdc) {
-		printk(KERN_WARNING
-			"SSFDC_RO: out of memory for data structures\n");
+	if (!ssfdc)
 		return;
-	}
 
 	ssfdc->mbd.mtd = mtd;
 	ssfdc->mbd.devnum = -1;
@@ -342,11 +339,8 @@ static void ssfdcr_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	/* Allocate logical block map */
 	ssfdc->logic_block_map = kmalloc(sizeof(ssfdc->logic_block_map[0]) *
 					 ssfdc->map_len, GFP_KERNEL);
-	if (!ssfdc->logic_block_map) {
-		printk(KERN_WARNING
-			"SSFDC_RO: out of memory for data structures\n");
+	if (!ssfdc->logic_block_map)
 		goto out_err;
-	}
 	memset(ssfdc->logic_block_map, 0xff, sizeof(ssfdc->logic_block_map[0]) *
 		ssfdc->map_len);
 
-- 
1.7.0.4

Re: [PATCH 1/4] mtd: remove printk's for [kv][mz]alloc failures

[Artem Bityutskiy]

On Tue, 2011-06-07 at 16:01 -0700, Brian Norris wrote:
> When a memory allocation fails, the kernel will print out a backtrace
> automatically. These print statements are unnecessary.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/cmdlinepart.c         |    3 ---
>  drivers/mtd/inftlcore.c           |    4 +---
>  drivers/mtd/nand/nand_bbt.c       |   13 +++----------
>  drivers/mtd/nand/rtc_from4.c      |    1 -
>  drivers/mtd/nftlcore.c            |    4 +---
>  drivers/mtd/onenand/onenand_bbt.c |    4 +---
>  drivers/mtd/ssfdc.c               |   10 ++--------
>  7 files changed, 8 insertions(+), 31 deletions(-)

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   44 +++++++++++++++++++++---------------------
 drivers/mtd/nand/nand_bbt.c  |   39 ++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 019187b..9a11eea 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2167,7 +2167,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 
 	/* Reject writes, which are not page aligned */
 	if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
-		printk(KERN_NOTICE "%s: Attempt to write not "
+		pr_notice("%s: Attempt to write not "
 				"page aligned data\n", __func__);
 		return -EINVAL;
 	}
@@ -2561,7 +2561,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 		/* Heck if we have a bad block, we do not erase bad blocks! */
 		if (nand_block_checkbad(mtd, ((loff_t) page) <<
 					chip->page_shift, 0, allowbbt)) {
-			printk(KERN_WARNING "%s: attempt to erase a bad block "
+			pr_warning("%s: attempt to erase a bad block "
 					"at page 0x%08x\n", __func__, page);
 			instr->state = MTD_ERASE_FAILED;
 			goto erase_exit;
@@ -2735,7 +2735,7 @@ static void nand_resume(struct mtd_info *mtd)
 	if (chip->state == FL_PM_SUSPENDED)
 		nand_release_device(mtd);
 	else
-		printk(KERN_ERR "%s called for a chip which is not "
+		pr_err("%s called for a chip which is not "
 		       "in suspended state\n", __func__);
 }
 
@@ -2827,13 +2827,13 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
 		return 0;
 
-	printk(KERN_INFO "ONFI flash detected\n");
+	pr_info("ONFI flash detected\n");
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
 		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
-			printk(KERN_INFO "ONFI param page %d valid\n", i);
+			pr_info("ONFI param page %d valid\n", i);
 			break;
 		}
 	}
@@ -2857,7 +2857,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->onfi_version = 0;
 
 	if (!chip->onfi_version) {
-		printk(KERN_INFO "%s: unsupported ONFI version: %d\n",
+		pr_info("%s: unsupported ONFI version: %d\n",
 								__func__, val);
 		return 0;
 	}
@@ -2923,7 +2923,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		id_data[i] = chip->read_byte(mtd);
 
 	if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
-		printk(KERN_INFO "%s: second ID read did not match "
+		pr_info("%s: second ID read did not match "
 		       "%02x,%02x against %02x,%02x\n", __func__,
 		       *maf_id, *dev_id, id_data[0], id_data[1]);
 		return ERR_PTR(-ENODEV);
@@ -3068,10 +3068,10 @@ ident_done:
 	 * chip correct!
 	 */
 	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
-		printk(KERN_INFO "NAND device: Manufacturer ID:"
+		pr_info("NAND device: Manufacturer ID:"
 		       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
 		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
-		printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
+		pr_warning("NAND bus width %d instead %d bit\n",
 		       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
 		       busw ? 16 : 8);
 		return ERR_PTR(-EINVAL);
@@ -3129,7 +3129,7 @@ ident_done:
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
-	printk(KERN_INFO "NAND device: Manufacturer ID:"
+	pr_info("NAND device: Manufacturer ID:"
 		" 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
 		nand_manuf_ids[maf_idx].name,
 		chip->onfi_version ? chip->onfi_params.model : type->name);
@@ -3166,7 +3166,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 
 	if (IS_ERR(type)) {
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
-			printk(KERN_WARNING "No NAND device found.\n");
+			pr_warning("No NAND device found.\n");
 		chip->select_chip(mtd, -1);
 		return PTR_ERR(type);
 	}
@@ -3184,7 +3184,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 			break;
 	}
 	if (i > 1)
-		printk(KERN_INFO "%d NAND chips detected\n", i);
+		pr_info("%d NAND chips detected\n", i);
 
 	/* Store the number of chips and calc total size for mtd */
 	chip->numchips = i;
@@ -3234,7 +3234,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			chip->ecc.layout = &nand_oob_128;
 			break;
 		default:
-			printk(KERN_WARNING "No oob scheme defined for "
+			pr_warning("No oob scheme defined for "
 			       "oobsize %d\n", mtd->oobsize);
 			BUG();
 		}
@@ -3253,7 +3253,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		/* Similar to NAND_ECC_HW, but a separate read_page handle */
 		if (!chip->ecc.calculate || !chip->ecc.correct ||
 		     !chip->ecc.hwctl) {
-			printk(KERN_WARNING "No ECC functions supplied; "
+			pr_warning("No ECC functions supplied; "
 			       "Hardware ECC not possible\n");
 			BUG();
 		}
@@ -3282,7 +3282,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		     chip->ecc.read_page == nand_read_page_hwecc ||
 		     !chip->ecc.write_page ||
 		     chip->ecc.write_page == nand_write_page_hwecc)) {
-			printk(KERN_WARNING "No ECC functions supplied; "
+			pr_warning("No ECC functions supplied; "
 			       "Hardware ECC not possible\n");
 			BUG();
 		}
@@ -3302,7 +3302,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 		if (mtd->writesize >= chip->ecc.size)
 			break;
-		printk(KERN_WARNING "%d byte HW ECC not possible on "
+		pr_warning("%d byte HW ECC not possible on "
 		       "%d byte page size, fallback to SW ECC\n",
 		       chip->ecc.size, mtd->writesize);
 		chip->ecc.mode = NAND_ECC_SOFT;
@@ -3324,7 +3324,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 	case NAND_ECC_SOFT_BCH:
 		if (!mtd_nand_has_bch()) {
-			printk(KERN_WARNING "CONFIG_MTD_ECC_BCH not enabled\n");
+			pr_warning("CONFIG_MTD_ECC_BCH not enabled\n");
 			BUG();
 		}
 		chip->ecc.calculate = nand_bch_calculate_ecc;
@@ -3351,13 +3351,13 @@ int nand_scan_tail(struct mtd_info *mtd)
 					       chip->ecc.bytes,
 					       &chip->ecc.layout);
 		if (!chip->ecc.priv) {
-			printk(KERN_WARNING "BCH ECC initialization failed!\n");
+			pr_warning("BCH ECC initialization failed!\n");
 			BUG();
 		}
 		break;
 
 	case NAND_ECC_NONE:
-		printk(KERN_WARNING "NAND_ECC_NONE selected by board driver. "
+		pr_warning("NAND_ECC_NONE selected by board driver. "
 		       "This is not recommended !!\n");
 		chip->ecc.read_page = nand_read_page_raw;
 		chip->ecc.write_page = nand_write_page_raw;
@@ -3370,7 +3370,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		break;
 
 	default:
-		printk(KERN_WARNING "Invalid NAND_ECC_MODE %d\n",
+		pr_warning("Invalid NAND_ECC_MODE %d\n",
 		       chip->ecc.mode);
 		BUG();
 	}
@@ -3392,7 +3392,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	 */
 	chip->ecc.steps = mtd->writesize / chip->ecc.size;
 	if (chip->ecc.steps * chip->ecc.size != mtd->writesize) {
-		printk(KERN_WARNING "Invalid ecc parameters\n");
+		pr_warning("Invalid ecc parameters\n");
 		BUG();
 	}
 	chip->ecc.total = chip->ecc.steps * chip->ecc.bytes;
@@ -3483,7 +3483,7 @@ int nand_scan(struct mtd_info *mtd, int maxchips)
 
 	/* Many callers got this wrong, so check for it for a while... */
 	if (!mtd->owner && caller_is_module()) {
-		printk(KERN_CRIT "%s called with NULL mtd->owner!\n",
+		pr_crit("%s called with NULL mtd->owner!\n",
 				__func__);
 		BUG();
 	}
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 8875d6d..b8551e9 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -205,10 +205,10 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 		res = mtd->read(mtd, from, len, &retlen, buf);
 		if (res < 0) {
 			if (retlen != len) {
-				printk(KERN_INFO "nand_bbt: Error reading bad block table\n");
+				pr_info("nand_bbt: Error reading bad block table\n");
 				return res;
 			}
-			printk(KERN_WARNING "nand_bbt: ECC error while reading bad block table\n");
+			pr_warn("nand_bbt: ECC error while reading bad block table\n");
 		}
 
 		/* Analyse data */
@@ -219,7 +219,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				if (tmp == msk)
 					continue;
 				if (reserved_block_code && (tmp == reserved_block_code)) {
-					printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%012llx\n",
+					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
 					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
 					mtd->ecc_stats.bbtblocks++;
@@ -229,7 +229,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				 * Leave it for now, if it's matured we can
 				 * move this message to MTD_DEBUG_LEVEL0.
 				 */
-				printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%012llx\n",
+				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
 				if (tmp == 0)
@@ -383,7 +383,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
-		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
+		pr_debug("Bad block table at page %d, version 0x%02X\n",
 		       td->pages[0], td->version[0]);
 	}
 
@@ -392,7 +392,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
-		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
+		pr_debug("Bad block table at page %d, version 0x%02X\n",
 		       md->pages[0], md->version[0]);
 	}
 	return 1;
@@ -466,7 +466,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	loff_t from;
 	size_t readlen;
 
-	printk(KERN_INFO "Scanning device for bad blocks\n");
+	pr_info("Scanning device for bad blocks\n");
 
 	if (bd->options & NAND_BBT_SCANALLPAGES)
 		len = 1 << (this->bbt_erase_shift - this->page_shift);
@@ -495,7 +495,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = 0;
 	} else {
 		if (chip >= this->numchips) {
-			printk(KERN_WARNING "create_bbt(): chipnr (%d) > available chips (%d)\n",
+			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
 			       chip + 1, this->numchips);
 			return -EINVAL;
 		}
@@ -524,7 +524,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 
 		if (ret) {
 			this->bbt[i >> 3] |= 0x03 << (i & 0x6);
-			printk(KERN_WARNING "Bad eraseblock %d at 0x%012llx\n",
+			pr_warn("Bad eraseblock %d at 0x%012llx\n",
 			       i >> 1, (unsigned long long)from);
 			mtd->ecc_stats.badblocks++;
 		}
@@ -607,10 +607,10 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
 	/* Check, if we found a bbt for each requested chip */
 	for (i = 0; i < chips; i++) {
 		if (td->pages[i] == -1)
-			printk(KERN_WARNING "Bad block table not found for chip %d\n", i);
+			pr_warn("Bad block table not found for chip %d\n", i);
 		else
-			printk(KERN_DEBUG "Bad block table found at page %d, version 0x%02X\n", td->pages[i],
-			       td->version[i]);
+			pr_debug("Bad block table found at page %d, version "
+				 "0x%02X\n", td->pages[i], td->version[i]);
 	}
 	return 0;
 }
@@ -723,7 +723,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			if (!md || md->pages[chip] != page)
 				goto write;
 		}
-		printk(KERN_ERR "No space left to write bad block table\n");
+		pr_err("No space left to write bad block table\n");
 		return -ENOSPC;
 	write:
 
@@ -758,12 +758,12 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			res = mtd->read(mtd, to, len, &retlen, buf);
 			if (res < 0) {
 				if (retlen != len) {
-					printk(KERN_INFO "nand_bbt: Error "
+					pr_info("nand_bbt: Error "
 					       "reading block for writing "
 					       "the bad block table\n");
 					return res;
 				}
-				printk(KERN_WARNING "nand_bbt: ECC error "
+				pr_warn("nand_bbt: ECC error "
 				       "while reading block for writing "
 				       "bad block table\n");
 			}
@@ -840,7 +840,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 		if (res < 0)
 			goto outerr;
 
-		printk(KERN_DEBUG "Bad block table written to 0x%012llx, version "
+		pr_debug("Bad block table written to 0x%012llx, version "
 		       "0x%02X\n", (unsigned long long)to, td->version[chip]);
 
 		/* Mark it as used */
@@ -849,8 +849,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	return 0;
 
  outerr:
-	printk(KERN_WARNING
-	       "nand_bbt: Error while writing bad block table %d\n", res);
+	pr_warning("nand_bbt: Error while writing bad block table %d\n", res);
 	return res;
 }
 
@@ -1130,7 +1129,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	 */
 	if (!td) {
 		if ((res = nand_memory_bbt(mtd, bd))) {
-			printk(KERN_ERR "nand_bbt: Can't scan flash and build the RAM-based BBT\n");
+			pr_err("nand_bbt: Can't scan flash and build the RAM-based BBT\n");
 			kfree(this->bbt);
 			this->bbt = NULL;
 		}
@@ -1298,7 +1297,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
 {
 	struct nand_bbt_descr *bd;
 	if (this->badblock_pattern) {
-		printk(KERN_WARNING "BBT descr already allocated; not replacing.\n");
+		pr_warn("BBT descr already allocated; not replacing.\n");
 		return -EINVAL;
 	}
 	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
-- 
1.7.0.4

Re: [PATCH 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

Brian, thanks for the patch, it certainly makes the code nicer and a bit
more readable. But I have few requests.

On Tue, 2011-06-07 at 16:01 -0700, Brian Norris wrote:
> -		printk(KERN_NOTICE "%s: Attempt to write not "
> +		pr_notice("%s: Attempt to write not "
>  				"page aligned data\n", __func__);

OK, so with this change the string becomes shorter and you do not have
to split it any more. You should make it look like this:

               pr_notice("%s: attempt to write not page aligned data\n",
                          __func__);
 
This is more readable and preferable - no string split. Please, do this
for other messages too.

Also, while on it, may be you could unify the messages and make them:

1. start with small letter if there is a function name prerix -
   currently some start with small and some start with capital.
2. if there is a dot at the end - remove it.


> @@ -2561,7 +2561,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  		/* Heck if we have a bad block, we do not erase bad blocks! */
>  		if (nand_block_checkbad(mtd, ((loff_t) page) <<
>  					chip->page_shift, 0, allowbbt)) {
> -			printk(KERN_WARNING "%s: attempt to erase a bad block "
> +			pr_warning("%s: attempt to erase a bad block "
>  					"at page 0x%08x\n", __func__, page);

Since you now have more space, I think it is a bit better to re-wrap the
message, i.e., make it like this:

                       pr_warning("%s: attempt to erase a bad block at page "
                                  "0x%08x\n", __func__, page);

or, if you prefer, even like this:

                       pr_warning("%s: attempt to erase a bad block at page 0x%08x\n",
                                   __func__, page);
 
because the coding style does allow strings longer than 80 characters if
"exceeding 80 columns significantly increases readability".

Ditto for the rest. With this, your clean-up will make the code even
cleaner :-)

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

Also fix some punctuation, indentation, and capitalization that went
along with the affected lines.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   65 ++++++++++++++++++++----------------------
 drivers/mtd/nand/nand_bbt.c  |   39 ++++++++++++-------------
 2 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 019187b..7550cea 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2167,8 +2167,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 
 	/* Reject writes, which are not page aligned */
 	if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
-		printk(KERN_NOTICE "%s: Attempt to write not "
-				"page aligned data\n", __func__);
+		pr_notice("%s: attempt to write non page aligned data\n",
+			   __func__);
 		return -EINVAL;
 	}
 
@@ -2561,8 +2561,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 		/* Heck if we have a bad block, we do not erase bad blocks! */
 		if (nand_block_checkbad(mtd, ((loff_t) page) <<
 					chip->page_shift, 0, allowbbt)) {
-			printk(KERN_WARNING "%s: attempt to erase a bad block "
-					"at page 0x%08x\n", __func__, page);
+			pr_warning("%s: attempt to erase a bad block at page 0x%08x\n",
+				    __func__, page);
 			instr->state = MTD_ERASE_FAILED;
 			goto erase_exit;
 		}
@@ -2735,8 +2735,8 @@ static void nand_resume(struct mtd_info *mtd)
 	if (chip->state == FL_PM_SUSPENDED)
 		nand_release_device(mtd);
 	else
-		printk(KERN_ERR "%s called for a chip which is not "
-		       "in suspended state\n", __func__);
+		pr_err("%s called for a chip which is not in suspended state\n",
+			__func__);
 }
 
 /* Set default functions */
@@ -2827,13 +2827,13 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
 		return 0;
 
-	printk(KERN_INFO "ONFI flash detected\n");
+	pr_info("ONFI flash detected\n");
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
 		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
-			printk(KERN_INFO "ONFI param page %d valid\n", i);
+			pr_info("ONFI param page %d valid\n", i);
 			break;
 		}
 	}
@@ -2857,8 +2857,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->onfi_version = 0;
 
 	if (!chip->onfi_version) {
-		printk(KERN_INFO "%s: unsupported ONFI version: %d\n",
-								__func__, val);
+		pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
 		return 0;
 	}
 
@@ -2923,7 +2922,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		id_data[i] = chip->read_byte(mtd);
 
 	if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
-		printk(KERN_INFO "%s: second ID read did not match "
+		pr_info("%s: second ID read did not match "
 		       "%02x,%02x against %02x,%02x\n", __func__,
 		       *maf_id, *dev_id, id_data[0], id_data[1]);
 		return ERR_PTR(-ENODEV);
@@ -3068,10 +3067,10 @@ ident_done:
 	 * chip correct!
 	 */
 	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
-		printk(KERN_INFO "NAND device: Manufacturer ID:"
+		pr_info("NAND device: Manufacturer ID:"
 		       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
 		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
-		printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
+		pr_warning("NAND bus width %d instead %d bit\n",
 		       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
 		       busw ? 16 : 8);
 		return ERR_PTR(-EINVAL);
@@ -3129,7 +3128,7 @@ ident_done:
 	if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
 		chip->cmdfunc = nand_command_lp;
 
-	printk(KERN_INFO "NAND device: Manufacturer ID:"
+	pr_info("NAND device: Manufacturer ID:"
 		" 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, *dev_id,
 		nand_manuf_ids[maf_idx].name,
 		chip->onfi_version ? chip->onfi_params.model : type->name);
@@ -3166,7 +3165,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 
 	if (IS_ERR(type)) {
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
-			printk(KERN_WARNING "No NAND device found.\n");
+			pr_warning("No NAND device found\n");
 		chip->select_chip(mtd, -1);
 		return PTR_ERR(type);
 	}
@@ -3184,7 +3183,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 			break;
 	}
 	if (i > 1)
-		printk(KERN_INFO "%d NAND chips detected\n", i);
+		pr_info("%d NAND chips detected\n", i);
 
 	/* Store the number of chips and calc total size for mtd */
 	chip->numchips = i;
@@ -3234,8 +3233,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 			chip->ecc.layout = &nand_oob_128;
 			break;
 		default:
-			printk(KERN_WARNING "No oob scheme defined for "
-			       "oobsize %d\n", mtd->oobsize);
+			pr_warning("No oob scheme defined for oobsize %d\n",
+				   mtd->oobsize);
 			BUG();
 		}
 	}
@@ -3253,8 +3252,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 		/* Similar to NAND_ECC_HW, but a separate read_page handle */
 		if (!chip->ecc.calculate || !chip->ecc.correct ||
 		     !chip->ecc.hwctl) {
-			printk(KERN_WARNING "No ECC functions supplied; "
-			       "Hardware ECC not possible\n");
+			pr_warning("No ECC functions supplied; "
+				   "hardware ECC not possible\n");
 			BUG();
 		}
 		if (!chip->ecc.read_page)
@@ -3282,8 +3281,8 @@ int nand_scan_tail(struct mtd_info *mtd)
 		     chip->ecc.read_page == nand_read_page_hwecc ||
 		     !chip->ecc.write_page ||
 		     chip->ecc.write_page == nand_write_page_hwecc)) {
-			printk(KERN_WARNING "No ECC functions supplied; "
-			       "Hardware ECC not possible\n");
+			pr_warning("No ECC functions supplied; "
+				   "hardware ECC not possible\n");
 			BUG();
 		}
 		/* Use standard syndrome read/write page function? */
@@ -3302,9 +3301,9 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 		if (mtd->writesize >= chip->ecc.size)
 			break;
-		printk(KERN_WARNING "%d byte HW ECC not possible on "
-		       "%d byte page size, fallback to SW ECC\n",
-		       chip->ecc.size, mtd->writesize);
+		pr_warning("%d byte HW ECC not possible on "
+			   "%d byte page size, fallback to SW ECC\n",
+			   chip->ecc.size, mtd->writesize);
 		chip->ecc.mode = NAND_ECC_SOFT;
 
 	case NAND_ECC_SOFT:
@@ -3324,7 +3323,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 
 	case NAND_ECC_SOFT_BCH:
 		if (!mtd_nand_has_bch()) {
-			printk(KERN_WARNING "CONFIG_MTD_ECC_BCH not enabled\n");
+			pr_warning("CONFIG_MTD_ECC_BCH not enabled\n");
 			BUG();
 		}
 		chip->ecc.calculate = nand_bch_calculate_ecc;
@@ -3351,14 +3350,14 @@ int nand_scan_tail(struct mtd_info *mtd)
 					       chip->ecc.bytes,
 					       &chip->ecc.layout);
 		if (!chip->ecc.priv) {
-			printk(KERN_WARNING "BCH ECC initialization failed!\n");
+			pr_warning("BCH ECC initialization failed!\n");
 			BUG();
 		}
 		break;
 
 	case NAND_ECC_NONE:
-		printk(KERN_WARNING "NAND_ECC_NONE selected by board driver. "
-		       "This is not recommended !!\n");
+		pr_warning("NAND_ECC_NONE selected by board driver. "
+			   "This is not recommended!\n");
 		chip->ecc.read_page = nand_read_page_raw;
 		chip->ecc.write_page = nand_write_page_raw;
 		chip->ecc.read_oob = nand_read_oob_std;
@@ -3370,8 +3369,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 		break;
 
 	default:
-		printk(KERN_WARNING "Invalid NAND_ECC_MODE %d\n",
-		       chip->ecc.mode);
+		pr_warning("Invalid NAND_ECC_MODE %d\n", chip->ecc.mode);
 		BUG();
 	}
 
@@ -3392,7 +3390,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	 */
 	chip->ecc.steps = mtd->writesize / chip->ecc.size;
 	if (chip->ecc.steps * chip->ecc.size != mtd->writesize) {
-		printk(KERN_WARNING "Invalid ecc parameters\n");
+		pr_warning("Invalid ecc parameters\n");
 		BUG();
 	}
 	chip->ecc.total = chip->ecc.steps * chip->ecc.bytes;
@@ -3483,8 +3481,7 @@ int nand_scan(struct mtd_info *mtd, int maxchips)
 
 	/* Many callers got this wrong, so check for it for a while... */
 	if (!mtd->owner && caller_is_module()) {
-		printk(KERN_CRIT "%s called with NULL mtd->owner!\n",
-				__func__);
+		pr_crit("%s called with NULL mtd->owner!\n", __func__);
 		BUG();
 	}
 
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 8875d6d..d7221df 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -205,10 +205,10 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 		res = mtd->read(mtd, from, len, &retlen, buf);
 		if (res < 0) {
 			if (retlen != len) {
-				printk(KERN_INFO "nand_bbt: Error reading bad block table\n");
+				pr_info("nand_bbt: Error reading bad block table\n");
 				return res;
 			}
-			printk(KERN_WARNING "nand_bbt: ECC error while reading bad block table\n");
+			pr_warn("nand_bbt: ECC error while reading bad block table\n");
 		}
 
 		/* Analyse data */
@@ -219,7 +219,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				if (tmp == msk)
 					continue;
 				if (reserved_block_code && (tmp == reserved_block_code)) {
-					printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%012llx\n",
+					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
 					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
 					mtd->ecc_stats.bbtblocks++;
@@ -229,7 +229,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				 * Leave it for now, if it's matured we can
 				 * move this message to MTD_DEBUG_LEVEL0.
 				 */
-				printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%012llx\n",
+				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
 				if (tmp == 0)
@@ -383,7 +383,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
-		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
+		pr_debug("Bad block table at page %d, version 0x%02X\n",
 		       td->pages[0], td->version[0]);
 	}
 
@@ -392,7 +392,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
-		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
+		pr_debug("Bad block table at page %d, version 0x%02X\n",
 		       md->pages[0], md->version[0]);
 	}
 	return 1;
@@ -466,7 +466,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	loff_t from;
 	size_t readlen;
 
-	printk(KERN_INFO "Scanning device for bad blocks\n");
+	pr_info("Scanning device for bad blocks\n");
 
 	if (bd->options & NAND_BBT_SCANALLPAGES)
 		len = 1 << (this->bbt_erase_shift - this->page_shift);
@@ -495,7 +495,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = 0;
 	} else {
 		if (chip >= this->numchips) {
-			printk(KERN_WARNING "create_bbt(): chipnr (%d) > available chips (%d)\n",
+			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
 			       chip + 1, this->numchips);
 			return -EINVAL;
 		}
@@ -524,7 +524,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 
 		if (ret) {
 			this->bbt[i >> 3] |= 0x03 << (i & 0x6);
-			printk(KERN_WARNING "Bad eraseblock %d at 0x%012llx\n",
+			pr_warn("Bad eraseblock %d at 0x%012llx\n",
 			       i >> 1, (unsigned long long)from);
 			mtd->ecc_stats.badblocks++;
 		}
@@ -607,10 +607,10 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
 	/* Check, if we found a bbt for each requested chip */
 	for (i = 0; i < chips; i++) {
 		if (td->pages[i] == -1)
-			printk(KERN_WARNING "Bad block table not found for chip %d\n", i);
+			pr_warn("Bad block table not found for chip %d\n", i);
 		else
-			printk(KERN_DEBUG "Bad block table found at page %d, version 0x%02X\n", td->pages[i],
-			       td->version[i]);
+			pr_debug("Bad block table found at page %d, version "
+				 "0x%02X\n", td->pages[i], td->version[i]);
 	}
 	return 0;
 }
@@ -723,7 +723,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			if (!md || md->pages[chip] != page)
 				goto write;
 		}
-		printk(KERN_ERR "No space left to write bad block table\n");
+		pr_err("No space left to write bad block table\n");
 		return -ENOSPC;
 	write:
 
@@ -758,12 +758,12 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			res = mtd->read(mtd, to, len, &retlen, buf);
 			if (res < 0) {
 				if (retlen != len) {
-					printk(KERN_INFO "nand_bbt: Error "
+					pr_info("nand_bbt: Error "
 					       "reading block for writing "
 					       "the bad block table\n");
 					return res;
 				}
-				printk(KERN_WARNING "nand_bbt: ECC error "
+				pr_warn("nand_bbt: ECC error "
 				       "while reading block for writing "
 				       "bad block table\n");
 			}
@@ -840,7 +840,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 		if (res < 0)
 			goto outerr;
 
-		printk(KERN_DEBUG "Bad block table written to 0x%012llx, version "
+		pr_debug("Bad block table written to 0x%012llx, version "
 		       "0x%02X\n", (unsigned long long)to, td->version[chip]);
 
 		/* Mark it as used */
@@ -849,8 +849,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	return 0;
 
  outerr:
-	printk(KERN_WARNING
-	       "nand_bbt: Error while writing bad block table %d\n", res);
+	pr_warning("nand_bbt: Error while writing bad block table %d\n", res);
 	return res;
 }
 
@@ -1130,7 +1129,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	 */
 	if (!td) {
 		if ((res = nand_memory_bbt(mtd, bd))) {
-			printk(KERN_ERR "nand_bbt: Can't scan flash and build the RAM-based BBT\n");
+			pr_err("nand_bbt: Can't scan flash and build the RAM-based BBT\n");
 			kfree(this->bbt);
 			this->bbt = NULL;
 		}
@@ -1298,7 +1297,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
 {
 	struct nand_bbt_descr *bd;
 	if (this->badblock_pattern) {
-		printk(KERN_WARNING "BBT descr already allocated; not replacing.\n");
+		pr_warn("BBT descr already allocated; not replacing\n");
 		return -EINVAL;
 	}
 	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
-- 
1.7.0.4

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote:
> Also fix some punctuation, indentation, and capitalization that went
> along with the affected lines.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/nand/nand_base.c |   65 ++++++++++++++++++++----------------------
>  drivers/mtd/nand/nand_bbt.c  |   39 ++++++++++++-------------
>  2 files changed, 50 insertions(+), 54 deletions(-)
> 

Pushed with few tweaks, thanks!

>  	if (id_data[0] != *maf_id || id_data[1] != *dev_id) {
> -		printk(KERN_INFO "%s: second ID read did not match "
> +		pr_info("%s: second ID read did not match "
>  		       "%02x,%02x against %02x,%02x\n", __func__,
>  		       *maf_id, *dev_id, id_data[0], id_data[1]);

This was not properly aligned - I've aligned it:

                pr_info("%s: second ID read did not match "
                        "%02x,%02x against %02x,%02x\n", __func__,
                        *maf_id, *dev_id, id_data[0], id_data[1]);


>  		return ERR_PTR(-ENODEV);
> @@ -3068,10 +3067,10 @@ ident_done:
>  	 * chip correct!
>  	 */
>  	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> -		printk(KERN_INFO "NAND device: Manufacturer ID:"
> +		pr_info("NAND device: Manufacturer ID:"
>  		       " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
>  		       *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
> -		printk(KERN_WARNING "NAND bus width %d instead %d bit\n",
> +		pr_warning("NAND bus width %d instead %d bit\n",
>  		       (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
>  		       busw ? 16 : 8);

And this, I've changed it:

                pr_info("NAND device: Manufacturer ID:"
                        " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
                        *dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
                pr_warning("NAND bus width %d instead %d bit\n",
                           (chip->options & NAND_BUSWIDTH_16) ? 16 : 8, 
                           busw ? 16 : 8);


> -					printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%012llx\n",
> +					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
>  					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
>  					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
>  					mtd->ecc_stats.bbtblocks++;
> @@ -229,7 +229,7 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
>  				 * Leave it for now, if it's matured we can
>  				 * move this message to MTD_DEBUG_LEVEL0.
>  				 */
> -				printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%012llx\n",
> +				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
>  				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);

Here I aligned lines too. They are also quite long, but that would need
to put this piece of code to a separate function, which is a different
story.

>  				/* Factory marked bad or worn out? */
>  				if (tmp == 0)
> @@ -383,7 +383,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
>  		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
>  			      mtd->writesize, td);
>  		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
> -		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
> +		pr_debug("Bad block table at page %d, version 0x%02X\n",
>  		       td->pages[0], td->version[0]);

Was not aligned.

>  	}
>  
> @@ -392,7 +392,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
>  		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
>  			      mtd->writesize, td);
>  		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
> -		printk(KERN_DEBUG "Bad block table at page %d, version 0x%02X\n",
> +		pr_debug("Bad block table at page %d, version 0x%02X\n",
>  		       md->pages[0], md->version[0]);

Was not aligned.

> +			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
>  			       chip + 1, this->numchips);

And this and other, I amended them.


> +					pr_info("nand_bbt: Error "
>  					       "reading block for writing "
>  					       "the bad block table\n");
>  					return res;
>  				}
> -				printk(KERN_WARNING "nand_bbt: ECC error "
> +				pr_warn("nand_bbt: ECC error "
>  				       "while reading block for writing "
>  				       "bad block table\n");

Tweaked these 2.

Plus some other minor tweaks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote:
> -		printk(KERN_DEBUG "Bad block table written to 0x%012llx, version "
> +		pr_debug("Bad block table written to 0x%012llx, version "
>  		       "0x%02X\n", (unsigned long long)to, td->version[chip]);

Hmm, the side effect of this change is this message will disappear if
DEBUG is undefined / dynamic debugging is disabled.

So I think we need to change all these pr_debug to pr_info, because this
is really just info.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Thu, 2011-06-09 at 09:53 +0300, Artem Bityutskiy wrote:
> On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote:
> > -		printk(KERN_DEBUG "Bad block table written to 0x%012llx, version "
> > +		pr_debug("Bad block table written to 0x%012llx, version "
> >  		       "0x%02X\n", (unsigned long long)to, td->version[chip]);
> 
> Hmm, the side effect of this change is this message will disappear if
> DEBUG is undefined / dynamic debugging is disabled.
> 
> So I think we need to change all these pr_debug to pr_info, because this
> is really just info.

I've pushed the following patch on top, is this OK with you?

>From b40c110df5943149892f9d6b2973a03f90e78e6c Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date: Thu, 9 Jun 2011 10:03:26 +0300
Subject: [PATCH] mtd: nand_bbt: turn pr_debug into pr_info

In the previous patch we changed all "prink(KERN_DEBUG" messages with
"pr_debug()", which means that the messages will now disappear unless
they are explicitly enabled with DEBUG macro and dynamic debugging means.

But if you look at those messages, they are information messages like "bad
block table found or re-written", so we do want to see them. This patch turns
those messages into 'pr_info()'.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/nand/nand_bbt.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index a1e51dc..e3b0d62 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -219,14 +219,14 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				if (tmp == msk)
 					continue;
 				if (reserved_block_code && (tmp == reserved_block_code)) {
-					pr_debug("nand_read_bbt: reserved block at 0x%012llx\n",
-						 (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
+					pr_info("nand_read_bbt: reserved block at 0x%012llx\n",
+						(loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
 					mtd->ecc_stats.bbtblocks++;
 					continue;
 				}
-				pr_debug("nand_read_bbt: bad block at 0x%012llx\n",
-					 (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
+				pr_info("nand_read_bbt: bad block at 0x%012llx\n",
+					(loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
 				if (tmp == 0)
 					this->bbt[offs + (act >> 3)] |= 0x3 << (act & 0x06);
@@ -379,8 +379,8 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
-		pr_debug("Bad block table at page %d, version 0x%02X\n",
-			 td->pages[0], td->version[0]);
+		pr_info("Bad block table at page %d, version 0x%02X\n",
+			td->pages[0], td->version[0]);
 	}
 
 	/* Read the mirror version, if available */
@@ -388,8 +388,8 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
-		pr_debug("Bad block table at page %d, version 0x%02X\n",
-			 md->pages[0], md->version[0]);
+		pr_info("Bad block table at page %d, version 0x%02X\n",
+			md->pages[0], md->version[0]);
 	}
 	return 1;
 }
@@ -492,7 +492,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	} else {
 		if (chip >= this->numchips) {
 			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
-			       chip + 1, this->numchips);
+				chip + 1, this->numchips);
 			return -EINVAL;
 		}
 		numblocks = this->chipsize >> (this->bbt_erase_shift - 1);
@@ -605,8 +605,8 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
 		if (td->pages[i] == -1)
 			pr_warn("Bad block table not found for chip %d\n", i);
 		else
-			pr_debug("Bad block table found at page %d, version "
-				 "0x%02X\n", td->pages[i], td->version[i]);
+			pr_info("Bad block table found at page %d, version "
+				"0x%02X\n", td->pages[i], td->version[i]);
 	}
 	return 0;
 }
@@ -834,8 +834,8 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 		if (res < 0)
 			goto outerr;
 
-		pr_debug("Bad block table written to 0x%012llx, version 0x%02X\n",
-			 (unsigned long long)to, td->version[chip]);
+		pr_info("Bad block table written to 0x%012llx, version 0x%02X\n",
+			(unsigned long long)to, td->version[chip]);
 
 		/* Mark it as used */
 		td->pages[chip] = page;
-- 
1.7.2.3


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Wed, 2011-06-08 at 11:25 -0700, Brian Norris wrote:
> Also fix some punctuation, indentation, and capitalization that went
> along with the affected lines.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/nand/nand_base.c |   65 ++++++++++++++++++++----------------------
>  drivers/mtd/nand/nand_bbt.c  |   39 ++++++++++++-------------
>  2 files changed, 50 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 019187b..7550cea 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2167,8 +2167,8 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>  
>  	/* Reject writes, which are not page aligned */
>  	if (NOTALIGNED(to) || NOTALIGNED(ops->len)) {
> -		printk(KERN_NOTICE "%s: Attempt to write not "
> -				"page aligned data\n", __func__);
> +		pr_notice("%s: attempt to write non page aligned data\n",
> +			   __func__);
>  		return -EINVAL;

Wait, and why are we using pr_* while it is better to use dbg_* ? :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Wait, and why are we using pr_* while it is better to use dbg_* ? :-)

Just so we're on the same page...did you say "dbg_*" when you really
meant "dev_*"?

Anyway, the answer is: I'm mostly trying to do as little shakeup as
possible, so "printk" could translate pretty directly to the "pr_*"
with shorter code, etc. If it makes as much (or more) sense to have
the device name on all the prints, then I can just as well do it that
way.

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote:
> On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Wait, and why are we using pr_* while it is better to use dbg_* ? :-)
> 
> Just so we're on the same page...did you say "dbg_*" when you really
> meant "dev_*"?

Yes, I meant dev_*

> Anyway, the answer is: I'm mostly trying to do as little shakeup as
> possible, so "printk" could translate pretty directly to the "pr_*"
> with shorter code, etc. If it makes as much (or more) sense to have
> the device name on all the prints, then I can just as well do it that
> way.

Yes, I think it makes much more sense to specify for which device (out
of possibly many!) this message belongs.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Thu, Jun 9, 2011 at 9:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote:
>> On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > Wait, and why are we using pr_* while it is better to use dbg_* ? :-)
>>
>> Just so we're on the same page...did you say "dbg_*" when you really
>> meant "dev_*"?
>
> Yes, I meant dev_*
>
>> Anyway, the answer is: I'm mostly trying to do as little shakeup as
>> possible, so "printk" could translate pretty directly to the "pr_*"
>> with shorter code, etc. If it makes as much (or more) sense to have
>> the device name on all the prints, then I can just as well do it that
>> way.
>
> Yes, I think it makes much more sense to specify for which device (out
> of possibly many!) this message belongs.

I've been testing some of the "dev_*" printing, and it seems as if our
mtd_info structs never have fully initialized "device" fields (i.e.,
mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name,
etc. are never filled in with anything meaningful). That means that
our dev_* messages do not have anything to work from and simply print
" (null): " references before our strings instead of device
driver/name information, for example:

 (null): ONFI flash detected

I'm a little new to the Linux device model, so I'm not sure if it's
safe and sensible to add lines to a driver's nand_probe function,
like, for plat_nand.c:
data->mtd.dev.driver = pdev->dev.driver;
data->mtd.dev.init_name = dev_name(&pdev->dev);

Is there some better way to initialize these device values? Or should
we scrap the whole dev_* thing since MTD doesn't match the device
model well enough?

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
> On Thu, Jun 9, 2011 at 9:03 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Thu, 2011-06-09 at 09:00 -0700, Brian Norris wrote:
> >> On Thu, Jun 9, 2011 at 12:44 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > Wait, and why are we using pr_* while it is better to use dbg_* ? :-)
> >>
> >> Just so we're on the same page...did you say "dbg_*" when you really
> >> meant "dev_*"?
> >
> > Yes, I meant dev_*
> >
> >> Anyway, the answer is: I'm mostly trying to do as little shakeup as
> >> possible, so "printk" could translate pretty directly to the "pr_*"
> >> with shorter code, etc. If it makes as much (or more) sense to have
> >> the device name on all the prints, then I can just as well do it that
> >> way.
> >
> > Yes, I think it makes much more sense to specify for which device (out
> > of possibly many!) this message belongs.
> 
> I've been testing some of the "dev_*" printing, and it seems as if our
> mtd_info structs never have fully initialized "device" fields (i.e.,
> mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name,
> etc. are never filled in with anything meaningful). That means that
> our dev_* messages do not have anything to work from and simply print
> " (null): " references before our strings instead of device
> driver/name information, for example:
> 
>  (null): ONFI flash detected

Ouch, good finding.

> I'm a little new to the Linux device model, so I'm not sure if it's
> safe and sensible to add lines to a driver's nand_probe function,
> like, for plat_nand.c:
> data->mtd.dev.driver = pdev->dev.driver;
> data->mtd.dev.init_name = dev_name(&pdev->dev);

I think so, I believe struct device was added to mtd by some non-MTD guy
to just make it complaint with one of interface changes which required
struct device. So please, go ahead and improve that a bit.

> Is there some better way to initialize these device values? Or should
> we scrap the whole dev_* thing since MTD doesn't match the device
> model well enough?

I think there is no reason why it would not match the model, so of
course fixing this is preferable...

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Dmitry Eremin-Solenikov]

Artem Bityutskiy wrote:

> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
>> I'm a little new to the Linux device model, so I'm not sure if it's
>> safe and sensible to add lines to a driver's nand_probe function,
>> like, for plat_nand.c:
>> data->mtd.dev.driver = pdev->dev.driver;

IIUC, we should not set driver, but it might be better to consult with
more experienced DM hackers (like Greg Kroah-Hartman).

>> data->mtd.dev.init_name = dev_name(&pdev->dev);
>
> I think so, I believe struct device was added to mtd by some non-MTD guy
> to just make it complaint with one of interface changes which required
> struct device. So please, go ahead and improve that a bit.
-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Wed, Jun 22, 2011 at 2:12 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> Artem Bityutskiy wrote:
>
>> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
>>> I'm a little new to the Linux device model, so I'm not sure if it's
>>> safe and sensible to add lines to a driver's nand_probe function,
>>> like, for plat_nand.c:
>>> data->mtd.dev.driver = pdev->dev.driver;
>
> IIUC, we should not set driver, but it might be better to consult with
> more experienced DM hackers (like Greg Kroah-Hartman).
>
>>> data->mtd.dev.init_name = dev_name(&pdev->dev);
>>
>> I think so, I believe struct device was added to mtd by some non-MTD guy
>> to just make it complaint with one of interface changes which required
>> struct device. So please, go ahead and improve that a bit.

I've dug a little more on this, especially in the RFC + mailing list
conversation that led to adding the "mtd_info.dev" field. See here:
http://lists.infradead.org/pipermail/linux-mtd/2009-April/025142.html

It seems that, according to the following comment, the master MTD node
(which is passed to most of our functions) is never registered and so
does not have valid information for printing dev_* information:
     /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
      * to have the same data be in two different partitions.
      */

So it seems that, by "design," mtd->dev will never be suitable for a
dev_* message (at least when "mtd" is the master node), so instead of:
   dev_info(&mtd->dev, "informational message\n");
we should do:
   dev_info(mtd->dev.parent, "informational message\n");
This utilizes the physical device instead of the MTD device for debug messages.

Unfortunately, this kinda makes some of our code longer, rather than
more concise (not a huge deal, I guess). Also, it relies on a
specific, potentially-flawed device tree layout. Perhaps there should
be a "mtd_to_dev" helper (accompanying "dev_to_mtd") to abstract this
a little and leave room for future tree changes? For example:

static inline struct device *mtd_to_dev(struct mtd_info *mtd)
{
	return mtd ? mtd->dev.parent : NULL;
}

That makes the previous example into:
   dev_info(mtd_to_dev(mtd), "informational message\n");

I'll send out a patch set if any of this makes sense.

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Wed, Jul 6, 2011 at 11:51 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> static inline struct device *mtd_to_dev(struct mtd_info *mtd)
> {
>        return mtd ? mtd->dev.parent : NULL;
> }

Second thought on this function; we may not even need to worry about
the "mtd == NULL" case. But we might want to handle the case that
mtd->dev actually *is* registered properly:

static inline struct device *mtd_to_dev(struct mtd_info *mtd)
{
        if (device_is_registered(&mtd->dev))
                return &mtd->dev;
        return mtd->dev.parent;
}

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Dmitry Eremin-Solenikov]

On 7/6/11, Brian Norris <computersforpeace@gmail.com> wrote:
> On Wed, Jul 6, 2011 at 11:51 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
>> static inline struct device *mtd_to_dev(struct mtd_info *mtd)
>> {
>>        return mtd ? mtd->dev.parent : NULL;
>> }
>
> Second thought on this function; we may not even need to worry about
> the "mtd == NULL" case. But we might want to handle the case that
> mtd->dev actually *is* registered properly:
>
> static inline struct device *mtd_to_dev(struct mtd_info *mtd)
> {
>         if (device_is_registered(&mtd->dev))
>                 return &mtd->dev;
>         return mtd->dev.parent;

And what if mtd->dev.parent is NULL?


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Wed, Jul 6, 2011 at 12:47 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> And what if mtd->dev.parent is NULL?

Then we're out of luck and simply return NULL.

NULL shouldn't cause an error for debug messages, and if mtd_to_dev is
used for something more serious, then you have a problem you must fix
if mtd->dev.parent is NULL.

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Wed, 2011-07-06 at 11:51 -0700, Brian Norris wrote:
> On Wed, Jun 22, 2011 at 2:12 AM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
> > Artem Bityutskiy wrote:
> >
> >> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
> >>> I'm a little new to the Linux device model, so I'm not sure if it's
> >>> safe and sensible to add lines to a driver's nand_probe function,
> >>> like, for plat_nand.c:
> >>> data->mtd.dev.driver = pdev->dev.driver;
> >
> > IIUC, we should not set driver, but it might be better to consult with
> > more experienced DM hackers (like Greg Kroah-Hartman).
> >
> >>> data->mtd.dev.init_name = dev_name(&pdev->dev);
> >>
> >> I think so, I believe struct device was added to mtd by some non-MTD guy
> >> to just make it complaint with one of interface changes which required
> >> struct device. So please, go ahead and improve that a bit.
> 
> I've dug a little more on this, especially in the RFC + mailing list
> conversation that led to adding the "mtd_info.dev" field. See here:
> http://lists.infradead.org/pipermail/linux-mtd/2009-April/025142.html
> 
> It seems that, according to the following comment, the master MTD node
> (which is passed to most of our functions) is never registered and so
> does not have valid information for printing dev_* information:
>      /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
>       * to have the same data be in two different partitions.
>       */

Sorry, I still don't get it, why mtd partitions do not have names. The
discussion above is about who will be the parent, while we care about
making dev_dbg(mtd->dev, ...) print names.

if we look at 'add_mtd_device()' in mtdcore.c, we see something like
full initialization:

        /* Caller should have set dev.parent to match the
         * physical device.
         */
        mtd->dev.type = &mtd_devtype;
        mtd->dev.class = &mtd_class;
        mtd->dev.devt = MTD_DEVT(i);
        dev_set_name(&mtd->dev, "mtd%d", i);
        dev_set_drvdata(&mtd->dev, mtd);
        if (device_register(&mtd->dev) != 0)
                goto fail_added;

        if (MTD_DEVT(i))
                device_create(&mtd_class, mtd->dev.parent,
                              MTD_DEVT(i) + 1,
                              NULL, "mtd%dro", i);

And if we look at 'mtd_add_partition()', we see that 'add_mtd_device()'
is also called, so mtd->dev should be initialized.

What exactly is the problem?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

Hi,

I included some comments along with some of you questions and tried to
summarize and answer more at the end. The end comments may explain the
middle...

On Wed, Jul 6, 2011 at 11:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-07-06 at 11:51 -0700, Brian Norris wrote:
>> It seems that, according to the following comment, the master MTD node
>> (which is passed to most of our functions) is never registered and so
>> does not have valid information for printing dev_* information:
>>      /* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
>>       * to have the same data be in two different partitions.
>>       */
>
> Sorry, I still don't get it, why mtd partitions do not have names. The
> discussion above is about who will be the parent, while we care about
> making dev_dbg(mtd->dev, ...) print names.

The *partitions* have names, but the master does not.

We end up with a device tree that's something like this:

     <physical_device>
     |_ mtd0 (slave->dev)
     |_ mtd0ro
     |_ mtd1
     |_ mtd1ro
     |_ ...
     ...
     master mtd->dev?

where each partition (and RO counterpart) in the tree is registered
using add_mtd_partitions() and each partition's parent is the physical
device. The master mtd->dev is floating in space, since it isn't
registered anywhere.

I think I meant to copy a different code comment onto the previous
e-mail as well:

     /* ...
      * We don't register the master, or expect the caller to have done so,
      * for reasons of data integrity.
      */

So the master mtd->dev, which is used in much of our MTD
initialization code, is never named and registered properly.

> if we look at 'add_mtd_device()' in mtdcore.c, we see something like
> full initialization:
>
>        /* Caller should have set dev.parent to match the
>         * physical device.
>         */
>        mtd->dev.type = &mtd_devtype;
>        mtd->dev.class = &mtd_class;
>        mtd->dev.devt = MTD_DEVT(i);
>        dev_set_name(&mtd->dev, "mtd%d", i);
>        dev_set_drvdata(&mtd->dev, mtd);
>        if (device_register(&mtd->dev) != 0)
>                goto fail_added;
>
>        if (MTD_DEVT(i))
>                device_create(&mtd_class, mtd->dev.parent,
>                              MTD_DEVT(i) + 1,
>                              NULL, "mtd%dro", i);

When called from mtd_add_partitions, this is only called for each
partition, not the master. That is the crux of the problem.

> And if we look at 'mtd_add_partition()', we see that 'add_mtd_device()'
> is also called, so mtd->dev should be initialized.
>
> What exactly is the problem?

I believe the problem is simply that partitions are named and
registered, whereas the master is not. During initialization, all
debug messages, etc. are run through the master MTD, not the named
partitions. This gives the "(null):" dev_* messages I saw on bootup.
I'm not sure if the master device causes much trouble after
initialization...I think it kinda fades away and leaves the partitions
for interaction with the user.

FYI, I'm looking mostly at cases where there *is* a partition layout.
In cases where there are no partitions, the driver will usually just
call "add_mtd_device" on the master MTD, and so this device be named
and registered properly. This isn't so much an issue, except that it
provides inconsistency between setups that use partitions and those
that don't. (Couldn't we just force everyone to use partitions? Users
who don't need them could just form a single partition for the whole
device...)

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Thu, 2011-07-07 at 10:00 -0700, Brian Norris wrote:
> I believe the problem is simply that partitions are named and
> registered, whereas the master is not. During initialization, all
> debug messages, etc. are run through the master MTD, not the named
> partitions. This gives the "(null):" dev_* messages I saw on bootup.
> I'm not sure if the master device causes much trouble after
> initialization...I think it kinda fades away and leaves the partitions
> for interaction with the user.
> 
> FYI, I'm looking mostly at cases where there *is* a partition layout.
> In cases where there are no partitions, the driver will usually just
> call "add_mtd_device" on the master MTD, and so this device be named
> and registered properly. This isn't so much an issue, except that it
> provides inconsistency between setups that use partitions and those
> that don't. (Couldn't we just force everyone to use partitions? Users
> who don't need them could just form a single partition for the whole
> device...)

Well, ok, indeed, in the driver level we have no idea about partitions,
we deal with the flash chip, by design...

Probably we should better use pr_* series of functions instead, I guess,
as dev_* functions seem to not be really suitable for us. That's what
you have originally done, sorry for bad suggestion.

On the other hand, why we cannot pass the partition struct mtd_info down
to the driver instead? Well, ideally, drivers should not use struct
mtd_info at all. But this is probably a different story...

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

Now I'm not sure I entirely understand all your comments :)

On Thu, Jul 7, 2011 at 12:56 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Well, ok, indeed, in the driver level we have no idea about partitions,
> we deal with the flash chip, by design...

What exactly are you referring to? That the inability for drivers to
distinguish between partitions and the master MTD prevents them from
handling our device structure correctly? I thought we're talking
mostly about the NAND layer, not the driver layer.

> Probably we should better use pr_* series of functions instead, I guess,
> as dev_* functions seem to not be really suitable for us. That's what
> you have originally done, sorry for bad suggestion.

OK, I can do that, but first, what about the code I posted earlier for
finding the correct device corresponding to mtd_info?

static inline struct device *mtd_to_dev(struct mtd_info *mtd)
{
       if (device_is_registered(&mtd->dev))
               return &mtd->dev;
       return mtd->dev.parent;
}

I think this would work for any NAND or MTD code, so that we can do
something like this:
     dev_info(mtd_to_dev(mtd), "ONFI flash detected\n");
In NAND/MTD code, we basically have two cases for the current mtd_info:
1) corresponds to the master device, which is not registered. Its
parent should be set to the physical device, so use mtd->dev.parent.
2) corresponds to a partition, which is registered. Use the partition
device &mtd->dev.
There's kind of a third case for non-partitioned flash:
3) corresponds to the master device, which is registered. Use &mtd->dev

These cases should all be covered, and I think the dev_* could would
be just fine. If that's not good enough though, then I'll just go back
to the pr_* code.

> On the other hand, why we cannot pass the partition struct mtd_info down
> to the driver instead? Well, ideally, drivers should not use struct
> mtd_info at all. But this is probably a different story...

Not quite sure where this is coming from. I don't believe I asked
about passing mtd_info to the driver (which happens all the time
anyway, AFAICT...). To be clear, what are the exact layers as you're
seeing them? It doesn't seem like we strictly follow them, but I see
something like this:
Filesystems
MTD
NAND
Driver
(Hardware)

Anyway, this thread has managed to diverge into several different
topics by now, and it all came from some simple printk stuff... maybe
it's worth wrapping up the printk's and handling any device issues
separately.

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

Hi,

sorry, I was very busy so could not reply.

On Fri, 2011-07-08 at 09:06 -0700, Brian Norris wrote:
> On Thu, Jul 7, 2011 at 12:56 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > Well, ok, indeed, in the driver level we have no idea about partitions,
> > we deal with the flash chip, by design...
> 
> What exactly are you referring to? That the inability for drivers to
> distinguish between partitions and the master MTD prevents them from
> handling our device structure correctly? I thought we're talking
> mostly about the NAND layer, not the driver layer.

I meant that at mtdparts hides translates requests to partitions into
requests to the master MTD device, and in nand_base all requests look
like requests to the master MTD device, so it is logical that and
nand_base level we cannot easily prefix our messages with "mtdX:", where
X is the partition number.

> > Probably we should better use pr_* series of functions instead, I guess,
> > as dev_* functions seem to not be really suitable for us. That's what
> > you have originally done, sorry for bad suggestion.
> 
> OK, I can do that, but first, what about the code I posted earlier for
> finding the correct device corresponding to mtd_info?
> 
> static inline struct device *mtd_to_dev(struct mtd_info *mtd)
> {
>        if (device_is_registered(&mtd->dev))
>                return &mtd->dev;
>        return mtd->dev.parent;
> }

I think I just do not really understand it. At nand_base
"struct mtd_info *mtd" will always be the master device, right? Then
using it will make messages confusing - they will all start with, say,
"mtd0" for all the partitions on this flash chip. So dev_*() become kind
of useless, right? Then why using them if we can use pr_*() which do not
require the device and will not have confusing prefix?

> 
> I think this would work for any NAND or MTD code, so that we can do
> something like this:
>      dev_info(mtd_to_dev(mtd), "ONFI flash detected\n");
> In NAND/MTD code, we basically have two cases for the current mtd_info:
> 1) corresponds to the master device, which is not registered. Its
> parent should be set to the physical device, so use mtd->dev.parent.
> 2) corresponds to a partition, which is registered. Use the partition
> device &mtd->dev.

Probably this is the point of confusion. How can mtd->dev in nand_base
correspond to an mtd partition if mtdpart is designed so that it
translates partition requests into master MTD device requests? E.g., see
part_read() in drivers/mtd/mtdpart.c

> There's kind of a third case for non-partitioned flash:
> 3) corresponds to the master device, which is registered. Use &mtd->dev
> 
> These cases should all be covered, and I think the dev_* could would
> be just fine. If that's not good enough though, then I'll just go back
> to the pr_* code.

IMO, the only advantage of using dev_* is that they prefix messages with
the name of device the messages belong to.

MTD is designed so that from user perspective partitions and the whole
device are not distinguishable, not like in hard drives with with, say,
sda,sda1,sda2 - we have mtd0, mtd1, mtd2...

So I think pr_*() is just more suitable.

> > On the other hand, why we cannot pass the partition struct mtd_info down
> > to the driver instead? Well, ideally, drivers should not use struct
> > mtd_info at all. But this is probably a different story...
> 
> Not quite sure where this is coming from. I don't believe I asked
> about passing mtd_info to the driver (which happens all the time
> anyway, AFAICT...). To be clear, what are the exact layers as you're
> seeing them? It doesn't seem like we strictly follow them, but I see
> something like this:

Oh, I just meant that we have struct mtd_info and struct nand_chip. I
think struct mtd_info should be only for MTD clients, and when we go
down to the nand_base we should not use it at all, we should have all
the information in struct nand_chip.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
> 
> I've been testing some of the "dev_*" printing, and it seems as if our
> mtd_info structs never have fully initialized "device" fields (i.e.,
> mtd->dev.driver, mtd->dev.bus, mtd->dev.class, mtd->dev.init_name,
> etc. are never filled in with anything meaningful). That means that
> our dev_* messages do not have anything to work from and simply print
> " (null): " references before our strings instead of device
> driver/name information, for example:
> 
>  (null): ONFI flash detected 

You mean that

dev_info(&mtd->dev, "ONFI flash detected");

produces this?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Thu, Jul 7, 2011 at 12:01 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2011-06-13 at 11:24 -0700, Brian Norris wrote:
>>  (null): ONFI flash detected
>
> You mean that
>
> dev_info(&mtd->dev, "ONFI flash detected");
>
> produces this?

Yes, sorry for not being specific.

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Artem Bityutskiy]

Brian, would you please send a new series? I suggest you the following
order of things.

1. clean-up of non-DEBUG() messages.
   1.1. go through all printks and check if KERN_* is ok, fix. E.g.,
        I can see that all KERN_DEBUG should become KERN_INFO
   1.2. go through all printks and check if the function name prefix
        makes sense there - kill those which do not.

   Also, while doing this, keep in mind that messages will be later
   turned into dev_* (dev_info(), dev_err(), etc) so they will be
   automatically prefixed with the device name which is already some
   identification of the source of the message.

   1.3. turn messages to dev_*
   1.4. make another pass and do all the lines consolidations,
        alignments, punctuation, etc etc.

2. clean-up of DEBUG() cruft.
   2.1 change DEBUG() with dev_dbg()

   Kill all function name prefixes as well. The rationale is: dev_dbg()
   already adds function name prefixe (and process id) if dynamic
   debugging is enabled. In case of debugging with DEBUG macro, you
   anyway have to change the code (adding #define DEBUG) so you might
   easily defind pr_fmt at the same time for youself.

How does this sound to you?

FWIW: I've moved the patches I pushed from the master branch to the
"brian" branch of l2-mtd-2.6.git tree. But they will not be useful
for you, I guess.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Thu, Jun 9, 2011 at 1:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Brian, would you please send a new series? I suggest you the following
> order of things.
>
> 1. clean-up of non-DEBUG() messages.
>   1.1. go through all printks and check if KERN_* is ok, fix. E.g.,
>        I can see that all KERN_DEBUG should become KERN_INFO
>   1.2. go through all printks and check if the function name prefix
>        makes sense there - kill those which do not.
>
>   Also, while doing this, keep in mind that messages will be later
>   turned into dev_* (dev_info(), dev_err(), etc) so they will be
>   automatically prefixed with the device name which is already some
>   identification of the source of the message.
>
>   1.3. turn messages to dev_*
>   1.4. make another pass and do all the lines consolidations,
>        alignments, punctuation, etc etc.
>
> 2. clean-up of DEBUG() cruft.
>   2.1 change DEBUG() with dev_dbg()
>
>   Kill all function name prefixes as well. The rationale is: dev_dbg()
>   already adds function name prefixe (and process id) if dynamic
>   debugging is enabled. In case of debugging with DEBUG macro, you
>   anyway have to change the code (adding #define DEBUG) so you might
>   easily defind pr_fmt at the same time for youself.
>
> How does this sound to you?

Sounds good. Will send patches later today, I think.

> FWIW: I've moved the patches I pushed from the master branch to the
> "brian" branch of l2-mtd-2.6.git tree. But they will not be useful
> for you, I guess.

Thanks, although I will have to rework all of them :)

Brian

Re: [PATCH v2 2/4] mtd: nand: convert printk() to pr_*()

[Brian Norris]

On Thu, Jun 9, 2011 at 9:22 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, Jun 9, 2011 at 1:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> Brian, would you please send a new series? I suggest you the following
>> order of things.
...
>> How does this sound to you?
>
> Sounds good. Will send patches later today, I think.

Hmm, actually, I'll get to this again on Monday! Have a good weekend.

Brian

[PATCH 3/4] mtd: nand: replace DEBUG() with dev_dbg()

[Brian Norris]

Start moving away from the MTD_DEBUG_LEVEL messages. The dynamic
debugging feature is a generic kernel feature that provides more
flexibility.

(See Documentation/dynamic-debug-howto.txt)

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   46 +++++++++++++++++++++---------------------
 drivers/mtd/nand/nand_bbt.c  |    9 ++-----
 2 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9a11eea..95a4e71 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -113,20 +113,20 @@ static int check_offs_len(struct mtd_info *mtd,
 
 	/* Start address must align on block boundary */
 	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		dev_dbg(&mtd->dev, "%s: Unaligned address\n", __func__);
 		ret = -EINVAL;
 	}
 
 	/* Length must align on block boundary */
 	if (len & ((1 << chip->phys_erase_shift) - 1)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
+		dev_dbg(&mtd->dev, "%s: Length not block aligned\n",
 					__func__);
 		ret = -EINVAL;
 	}
 
 	/* Do not allow past end of device */
 	if (ofs + len > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
+		dev_dbg(&mtd->dev, "%s: Past end of device\n",
 					__func__);
 		ret = -EINVAL;
 	}
@@ -912,7 +912,7 @@ static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
 	udelay(1000);
 	/* See if device thinks it succeeded */
 	if (status & 0x01) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+		dev_dbg(&mtd->dev, "%s: Error status = 0x%08x\n",
 					__func__, status);
 		ret = -EIO;
 	}
@@ -934,7 +934,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	int chipnr;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
 			__func__, (unsigned long long)ofs, len);
 
 	if (check_offs_len(mtd, ofs, len))
@@ -953,7 +953,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+		dev_dbg(&mtd->dev, "%s: Device is write protected!!!\n",
 					__func__);
 		ret = -EIO;
 		goto out;
@@ -987,7 +987,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	int chipnr, status, page;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
 			__func__, (unsigned long long)ofs, len);
 
 	if (check_offs_len(mtd, ofs, len))
@@ -1002,7 +1002,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+		dev_dbg(&mtd->dev, "%s: Device is write protected!!!\n",
 					__func__);
 		status = MTD_ERASE_FAILED;
 		ret = -EIO;
@@ -1018,7 +1018,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	udelay(1000);
 	/* See if device thinks it succeeded */
 	if (status & 0x01) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+		dev_dbg(&mtd->dev, "%s: Error status = 0x%08x\n",
 					__func__, status);
 		ret = -EIO;
 		goto out;
@@ -1757,7 +1757,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	int len;
 	uint8_t *buf = ops->oobbuf;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: from = 0x%08Lx, len = %i\n",
+	dev_dbg(&mtd->dev, "%s: from = 0x%08Lx, len = %i\n",
 			__func__, (unsigned long long)from, readlen);
 
 	if (ops->mode == MTD_OOB_AUTO)
@@ -1766,7 +1766,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 		len = mtd->oobsize;
 
 	if (unlikely(ops->ooboffs >= len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
+		dev_dbg(&mtd->dev, "%s: Attempt to start read "
 					"outside oob\n", __func__);
 		return -EINVAL;
 	}
@@ -1775,7 +1775,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	if (unlikely(from >= mtd->size ||
 		     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
 					(from >> chip->page_shift)) * len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
+		dev_dbg(&mtd->dev, "%s: Attempt read beyond end "
 					"of device\n", __func__);
 		return -EINVAL;
 	}
@@ -1851,7 +1851,7 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 
 	/* Do not allow reads past end of device */
 	if (ops->datbuf && (from + ops->len) > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read "
+		dev_dbg(&mtd->dev, "%s: Attempt read "
 				"beyond end of device\n", __func__);
 		return -EINVAL;
 	}
@@ -2341,7 +2341,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 	int chipnr, page, status, len;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: to = 0x%08x, len = %i\n",
+	dev_dbg(&mtd->dev, "%s: to = 0x%08x, len = %i\n",
 			 __func__, (unsigned int)to, (int)ops->ooblen);
 
 	if (ops->mode == MTD_OOB_AUTO)
@@ -2351,13 +2351,13 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 
 	/* Do not allow write past end of page */
 	if ((ops->ooboffs + ops->ooblen) > len) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to write "
+		dev_dbg(&mtd->dev, "%s: Attempt to write "
 				"past end of page\n", __func__);
 		return -EINVAL;
 	}
 
 	if (unlikely(ops->ooboffs >= len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start "
+		dev_dbg(&mtd->dev, "%s: Attempt to start "
 				"write outside oob\n", __func__);
 		return -EINVAL;
 	}
@@ -2367,7 +2367,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 		     ops->ooboffs + ops->ooblen >
 			((mtd->size >> chip->page_shift) -
 			 (to >> chip->page_shift)) * len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt write beyond "
+		dev_dbg(&mtd->dev, "%s: Attempt write beyond "
 				"end of device\n", __func__);
 		return -EINVAL;
 	}
@@ -2423,7 +2423,7 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
 
 	/* Do not allow writes past end of device */
 	if (ops->datbuf && (to + ops->len) > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt write beyond "
+		dev_dbg(&mtd->dev, "%s: Attempt write beyond "
 				"end of device\n", __func__);
 		return -EINVAL;
 	}
@@ -2513,7 +2513,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 	unsigned int bbt_masked_page = 0xffffffff;
 	loff_t len;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
 				__func__, (unsigned long long)instr->addr,
 				(unsigned long long)instr->len);
 
@@ -2537,7 +2537,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+		dev_dbg(&mtd->dev, "%s: Device is write protected!!!\n",
 					__func__);
 		instr->state = MTD_ERASE_FAILED;
 		goto erase_exit;
@@ -2589,7 +2589,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 
 		/* See if block erase succeeded */
 		if (status & NAND_STATUS_FAIL) {
-			DEBUG(MTD_DEBUG_LEVEL0, "%s: Failed erase, "
+			dev_dbg(&mtd->dev, "%s: Failed erase, "
 					"page 0x%08x\n", __func__, page);
 			instr->state = MTD_ERASE_FAILED;
 			instr->fail_addr =
@@ -2650,7 +2650,7 @@ erase_exit:
 		if (!rewrite_bbt[chipnr])
 			continue;
 		/* Update the BBT for chip */
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: nand_update_bbt "
+		dev_dbg(&mtd->dev, "%s: nand_update_bbt "
 			"(%d:0x%0llx 0x%0x)\n", __func__, chipnr,
 			rewrite_bbt[chipnr], chip->bbt_td->pages[chipnr]);
 		nand_update_bbt(mtd, rewrite_bbt[chipnr]);
@@ -2670,7 +2670,7 @@ static void nand_sync(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: called\n", __func__);
+	dev_dbg(&mtd->dev, "%s: called\n", __func__);
 
 	/* Grab the lock and see if the device is available */
 	nand_get_device(chip, mtd, FL_SYNCING);
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index b8551e9..4d18098 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -225,10 +225,6 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 					mtd->ecc_stats.bbtblocks++;
 					continue;
 				}
-				/*
-				 * Leave it for now, if it's matured we can
-				 * move this message to MTD_DEBUG_LEVEL0.
-				 */
 				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
@@ -1378,8 +1374,9 @@ int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)
 	block = (int)(offs >> (this->bbt_erase_shift - 1));
 	res = (this->bbt[block >> 3] >> (block & 0x06)) & 0x03;
 
-	DEBUG(MTD_DEBUG_LEVEL2, "nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n",
-	      (unsigned int)offs, block >> 1, res);
+	dev_dbg(&mtd->dev, "nand_isbad_bbt(): bbt info for offs 0x%08x: "
+			   "(block %d) 0x%02x\n",
+			   (unsigned int)offs, block >> 1, res);
 
 	switch ((int)res) {
 	case 0x00:
-- 
1.7.0.4

Re: [PATCH 3/4] mtd: nand: replace DEBUG() with dev_dbg()

[Artem Bityutskiy]

On Tue, 2011-06-07 at 16:01 -0700, Brian Norris wrote:
> -		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
> +		dev_dbg(&mtd->dev, "%s: Length not block aligned\n",
>  					__func__);

Ditto for this patch.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH v2 3/4] mtd: nand: replace DEBUG() with dev_dbg()

[Brian Norris]

Start moving away from the MTD_DEBUG_LEVEL messages. The dynamic
debugging feature is a generic kernel feature that provides more
flexibility.

(See Documentation/dynamic-debug-howto.txt)

Also fix some punctuation, indentation, and capitalization that went
along with the affected lines.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   74 ++++++++++++++++++++---------------------
 drivers/mtd/nand/nand_bbt.c  |    9 ++---
 2 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 7550cea..e124498 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -113,21 +113,19 @@ static int check_offs_len(struct mtd_info *mtd,
 
 	/* Start address must align on block boundary */
 	if (ofs & ((1 << chip->phys_erase_shift) - 1)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__);
+		dev_dbg(&mtd->dev, "%s: unaligned address\n", __func__);
 		ret = -EINVAL;
 	}
 
 	/* Length must align on block boundary */
 	if (len & ((1 << chip->phys_erase_shift) - 1)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n",
-					__func__);
+		dev_dbg(&mtd->dev, "%s: length not block aligned\n", __func__);
 		ret = -EINVAL;
 	}
 
 	/* Do not allow past end of device */
 	if (ofs + len > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n",
-					__func__);
+		dev_dbg(&mtd->dev, "%s: past end of device\n", __func__);
 		ret = -EINVAL;
 	}
 
@@ -912,7 +910,7 @@ static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
 	udelay(1000);
 	/* See if device thinks it succeeded */
 	if (status & 0x01) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+		dev_dbg(&mtd->dev, "%s: error status = 0x%08x\n",
 					__func__, status);
 		ret = -EIO;
 	}
@@ -934,7 +932,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	int chipnr;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
 			__func__, (unsigned long long)ofs, len);
 
 	if (check_offs_len(mtd, ofs, len))
@@ -953,7 +951,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+		dev_dbg(&mtd->dev, "%s: device is write protected!\n",
 					__func__);
 		ret = -EIO;
 		goto out;
@@ -987,7 +985,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	int chipnr, status, page;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
 			__func__, (unsigned long long)ofs, len);
 
 	if (check_offs_len(mtd, ofs, len))
@@ -1002,7 +1000,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
+		dev_dbg(&mtd->dev, "%s: device is write protected!\n",
 					__func__);
 		status = MTD_ERASE_FAILED;
 		ret = -EIO;
@@ -1018,7 +1016,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	udelay(1000);
 	/* See if device thinks it succeeded */
 	if (status & 0x01) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Error status = 0x%08x\n",
+		dev_dbg(&mtd->dev, "%s: error status = 0x%08x\n",
 					__func__, status);
 		ret = -EIO;
 		goto out;
@@ -1757,7 +1755,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	int len;
 	uint8_t *buf = ops->oobbuf;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: from = 0x%08Lx, len = %i\n",
+	dev_dbg(&mtd->dev, "%s: from = 0x%08Lx, len = %i\n",
 			__func__, (unsigned long long)from, readlen);
 
 	if (ops->mode == MTD_OOB_AUTO)
@@ -1766,8 +1764,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 		len = mtd->oobsize;
 
 	if (unlikely(ops->ooboffs >= len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
-					"outside oob\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to start read outside oob\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -1775,8 +1773,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	if (unlikely(from >= mtd->size ||
 		     ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
 					(from >> chip->page_shift)) * len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
-					"of device\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to read beyond end of device\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -1851,8 +1849,8 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 
 	/* Do not allow reads past end of device */
 	if (ops->datbuf && (from + ops->len) > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read "
-				"beyond end of device\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to read beyond end of device\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -2341,7 +2339,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 	int chipnr, page, status, len;
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: to = 0x%08x, len = %i\n",
+	dev_dbg(&mtd->dev, "%s: to = 0x%08x, len = %i\n",
 			 __func__, (unsigned int)to, (int)ops->ooblen);
 
 	if (ops->mode == MTD_OOB_AUTO)
@@ -2351,14 +2349,14 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 
 	/* Do not allow write past end of page */
 	if ((ops->ooboffs + ops->ooblen) > len) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to write "
-				"past end of page\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to write past end of page\n",
+				__func__);
 		return -EINVAL;
 	}
 
 	if (unlikely(ops->ooboffs >= len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start "
-				"write outside oob\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to start write outside oob\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -2367,8 +2365,8 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 		     ops->ooboffs + ops->ooblen >
 			((mtd->size >> chip->page_shift) -
 			 (to >> chip->page_shift)) * len)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt write beyond "
-				"end of device\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to write beyond end of device\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -2423,8 +2421,8 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
 
 	/* Do not allow writes past end of device */
 	if (ops->datbuf && (to + ops->len) > mtd->size) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt write beyond "
-				"end of device\n", __func__);
+		dev_dbg(&mtd->dev, "%s: attempt to write beyond end of device\n",
+				__func__);
 		return -EINVAL;
 	}
 
@@ -2513,9 +2511,9 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 	unsigned int bbt_masked_page = 0xffffffff;
 	loff_t len;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
-				__func__, (unsigned long long)instr->addr,
-				(unsigned long long)instr->len);
+	dev_dbg(&mtd->dev, "%s: start = 0x%012llx, len = %llu\n",
+			__func__, (unsigned long long)instr->addr,
+			(unsigned long long)instr->len);
 
 	if (check_offs_len(mtd, instr->addr, instr->len))
 		return -EINVAL;
@@ -2537,8 +2535,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: Device is write protected!!!\n",
-					__func__);
+		dev_dbg(&mtd->dev, "%s: device is write protected!\n",
+				__func__);
 		instr->state = MTD_ERASE_FAILED;
 		goto erase_exit;
 	}
@@ -2589,8 +2587,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
 
 		/* See if block erase succeeded */
 		if (status & NAND_STATUS_FAIL) {
-			DEBUG(MTD_DEBUG_LEVEL0, "%s: Failed erase, "
-					"page 0x%08x\n", __func__, page);
+			dev_dbg(&mtd->dev, "%s: failed erase, page 0x%08x\n",
+					__func__, page);
 			instr->state = MTD_ERASE_FAILED;
 			instr->fail_addr =
 				((loff_t)page << chip->page_shift);
@@ -2650,9 +2648,9 @@ erase_exit:
 		if (!rewrite_bbt[chipnr])
 			continue;
 		/* Update the BBT for chip */
-		DEBUG(MTD_DEBUG_LEVEL0, "%s: nand_update_bbt "
-			"(%d:0x%0llx 0x%0x)\n", __func__, chipnr,
-			rewrite_bbt[chipnr], chip->bbt_td->pages[chipnr]);
+		dev_dbg(&mtd->dev, "%s: nand_update_bbt (%d:0x%0llx 0x%0x)\n",
+				__func__, chipnr, rewrite_bbt[chipnr],
+				chip->bbt_td->pages[chipnr]);
 		nand_update_bbt(mtd, rewrite_bbt[chipnr]);
 	}
 
@@ -2670,7 +2668,7 @@ static void nand_sync(struct mtd_info *mtd)
 {
 	struct nand_chip *chip = mtd->priv;
 
-	DEBUG(MTD_DEBUG_LEVEL3, "%s: called\n", __func__);
+	dev_dbg(&mtd->dev, "%s: called\n", __func__);
 
 	/* Grab the lock and see if the device is available */
 	nand_get_device(chip, mtd, FL_SYNCING);
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index d7221df..5835404 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -225,10 +225,6 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 					mtd->ecc_stats.bbtblocks++;
 					continue;
 				}
-				/*
-				 * Leave it for now, if it's matured we can
-				 * move this message to MTD_DEBUG_LEVEL0.
-				 */
 				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
@@ -1378,8 +1374,9 @@ int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)
 	block = (int)(offs >> (this->bbt_erase_shift - 1));
 	res = (this->bbt[block >> 3] >> (block & 0x06)) & 0x03;
 
-	DEBUG(MTD_DEBUG_LEVEL2, "nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n",
-	      (unsigned int)offs, block >> 1, res);
+	dev_dbg(&mtd->dev, "nand_isbad_bbt(): bbt info for offs 0x%08x: "
+			   "(block %d) 0x%02x\n",
+			   (unsigned int)offs, block >> 1, res);
 
 	switch ((int)res) {
 	case 0x00:
-- 
1.7.0.4

Re: [PATCH v2 3/4] mtd: nand: replace DEBUG() with dev_dbg()

[Artem Bityutskiy]

On Wed, 2011-06-08 at 11:27 -0700, Brian Norris wrote:
> +               dev_dbg(&mtd->dev, "%s: error status = 0x%08x\n",
>                                         __func__, status); 

Aligned things like this and pushed, thank you!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 4d18098..e2c9948 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -59,6 +59,8 @@
  *
  */
 
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/mtd/mtd.h>
@@ -205,10 +207,10 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 		res = mtd->read(mtd, from, len, &retlen, buf);
 		if (res < 0) {
 			if (retlen != len) {
-				pr_info("nand_bbt: Error reading bad block table\n");
+				pr_info("Error reading bad block table\n");
 				return res;
 			}
-			pr_warn("nand_bbt: ECC error while reading bad block table\n");
+			pr_warn("ECC error while reading bad block table\n");
 		}
 
 		/* Analyse data */
@@ -219,13 +221,13 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				if (tmp == msk)
 					continue;
 				if (reserved_block_code && (tmp == reserved_block_code)) {
-					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
+					pr_debug("Reserved block at 0x%012llx\n",
 					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
 					mtd->ecc_stats.bbtblocks++;
 					continue;
 				}
-				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
+				pr_debug("Bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
 				if (tmp == 0)
@@ -491,7 +493,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = 0;
 	} else {
 		if (chip >= this->numchips) {
-			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
+			pr_warn("chipnr (%d) > available chips (%d)\n",
 			       chip + 1, this->numchips);
 			return -EINVAL;
 		}
@@ -754,14 +756,13 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			res = mtd->read(mtd, to, len, &retlen, buf);
 			if (res < 0) {
 				if (retlen != len) {
-					pr_info("nand_bbt: Error "
+					pr_info("Error "
 					       "reading block for writing "
 					       "the bad block table\n");
 					return res;
 				}
-				pr_warn("nand_bbt: ECC error "
-				       "while reading block for writing "
-				       "bad block table\n");
+				pr_warn("ECC error while reading block for "
+					"writing bad block table\n");
 			}
 			/* Read oob data */
 			ops.ooblen = (len >> this->page_shift) * mtd->oobsize;
@@ -845,7 +846,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	return 0;
 
  outerr:
-	pr_warning("nand_bbt: Error while writing bad block table %d\n", res);
+	pr_warning("Error while writing bad block table %d\n", res);
 	return res;
 }
 
@@ -1125,7 +1126,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	 */
 	if (!td) {
 		if ((res = nand_memory_bbt(mtd, bd))) {
-			pr_err("nand_bbt: Can't scan flash and build the RAM-based BBT\n");
+			pr_err("Can't scan flash and build the RAM-based BBT\n");
 			kfree(this->bbt);
 			this->bbt = NULL;
 		}
-- 
1.7.0.4

Re: [PATCH 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Artem Bityutskiy]

On Tue, 2011-06-07 at 16:01 -0700, Brian Norris wrote:
> -				pr_info("nand_bbt: Error reading bad block table\n");
> +				pr_info("Error reading bad block table\n");

Compare:

read_bbt: Error reading bad block table
read_bbt: error reading bad block table

isn't the second version more correct? Or this depends on the country? I
prefer the latter, but as long as all messages are consistent, I do not
care much. What do you think?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Brian Norris]

On Wed, Jun 8, 2011 at 7:47 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Compare:
>
> read_bbt: Error reading bad block table
> read_bbt: error reading bad block table
>
> isn't the second version more correct? Or this depends on the country? I
> prefer the latter, but as long as all messages are consistent, I do not
> care much. What do you think?

I'm not sure it's not so much language-related here, since these type
of messages are just fragments anyway. I think the lowercase version
is more consistent.

Brian

[PATCH v2 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Brian Norris]

Also fix some capitalization that went along with the affected lines.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   44 +++++++++++++++++++++---------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 5835404..3deced4 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -59,6 +59,8 @@
  *
  */
 
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/mtd/mtd.h>
@@ -205,10 +207,10 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 		res = mtd->read(mtd, from, len, &retlen, buf);
 		if (res < 0) {
 			if (retlen != len) {
-				pr_info("nand_bbt: Error reading bad block table\n");
+				pr_info("error reading bad block table\n");
 				return res;
 			}
-			pr_warn("nand_bbt: ECC error while reading bad block table\n");
+			pr_warn("ECC error while reading bad block table\n");
 		}
 
 		/* Analyse data */
@@ -219,13 +221,13 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
 				if (tmp == msk)
 					continue;
 				if (reserved_block_code && (tmp == reserved_block_code)) {
-					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
+					pr_debug("reserved block at 0x%012llx\n",
 					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 					this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
 					mtd->ecc_stats.bbtblocks++;
 					continue;
 				}
-				pr_debug("nand_read_bbt: Bad block at 0x%012llx\n",
+				pr_debug("bad block at 0x%012llx\n",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out? */
 				if (tmp == 0)
@@ -379,7 +381,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)td->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		td->version[0] = buf[bbt_get_ver_offs(mtd, td)];
-		pr_debug("Bad block table at page %d, version 0x%02X\n",
+		pr_debug("bad block table at page %d, version 0x%02X\n",
 		       td->pages[0], td->version[0]);
 	}
 
@@ -388,7 +390,7 @@ static int read_abs_bbts(struct mtd_info *mtd, uint8_t *buf,
 		scan_read_raw(mtd, buf, (loff_t)md->pages[0] << this->page_shift,
 			      mtd->writesize, td);
 		md->version[0] = buf[bbt_get_ver_offs(mtd, md)];
-		pr_debug("Bad block table at page %d, version 0x%02X\n",
+		pr_debug("bad block table at page %d, version 0x%02X\n",
 		       md->pages[0], md->version[0]);
 	}
 	return 1;
@@ -462,7 +464,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	loff_t from;
 	size_t readlen;
 
-	pr_info("Scanning device for bad blocks\n");
+	pr_info("scanning device for bad blocks\n");
 
 	if (bd->options & NAND_BBT_SCANALLPAGES)
 		len = 1 << (this->bbt_erase_shift - this->page_shift);
@@ -491,7 +493,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 		from = 0;
 	} else {
 		if (chip >= this->numchips) {
-			pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n",
+			pr_warn("chipnr (%d) > available chips (%d)\n",
 			       chip + 1, this->numchips);
 			return -EINVAL;
 		}
@@ -520,7 +522,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 
 		if (ret) {
 			this->bbt[i >> 3] |= 0x03 << (i & 0x6);
-			pr_warn("Bad eraseblock %d at 0x%012llx\n",
+			pr_warn("bad eraseblock %d at 0x%012llx\n",
 			       i >> 1, (unsigned long long)from);
 			mtd->ecc_stats.badblocks++;
 		}
@@ -603,9 +605,9 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
 	/* Check, if we found a bbt for each requested chip */
 	for (i = 0; i < chips; i++) {
 		if (td->pages[i] == -1)
-			pr_warn("Bad block table not found for chip %d\n", i);
+			pr_warn("bad block table not found for chip %d\n", i);
 		else
-			pr_debug("Bad block table found at page %d, version "
+			pr_debug("bad block table found at page %d, version "
 				 "0x%02X\n", td->pages[i], td->version[i]);
 	}
 	return 0;
@@ -719,7 +721,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			if (!md || md->pages[chip] != page)
 				goto write;
 		}
-		pr_err("No space left to write bad block table\n");
+		pr_err("no space left to write bad block table\n");
 		return -ENOSPC;
 	write:
 
@@ -754,14 +756,12 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			res = mtd->read(mtd, to, len, &retlen, buf);
 			if (res < 0) {
 				if (retlen != len) {
-					pr_info("nand_bbt: Error "
-					       "reading block for writing "
-					       "the bad block table\n");
+					pr_info("error reading block for writing"
+						" the bad block table\n");
 					return res;
 				}
-				pr_warn("nand_bbt: ECC error "
-				       "while reading block for writing "
-				       "bad block table\n");
+				pr_warn("ECC error while reading block for "
+					"writing bad block table\n");
 			}
 			/* Read oob data */
 			ops.ooblen = (len >> this->page_shift) * mtd->oobsize;
@@ -836,8 +836,8 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 		if (res < 0)
 			goto outerr;
 
-		pr_debug("Bad block table written to 0x%012llx, version "
-		       "0x%02X\n", (unsigned long long)to, td->version[chip]);
+		pr_debug("bad block table written to 0x%012llx, version "
+			 "0x%02X\n", (unsigned long long)to, td->version[chip]);
 
 		/* Mark it as used */
 		td->pages[chip] = page;
@@ -845,7 +845,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	return 0;
 
  outerr:
-	pr_warning("nand_bbt: Error while writing bad block table %d\n", res);
+	pr_warning("error while writing bad block table %d\n", res);
 	return res;
 }
 
@@ -1125,7 +1125,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	 */
 	if (!td) {
 		if ((res = nand_memory_bbt(mtd, bd))) {
-			pr_err("nand_bbt: Can't scan flash and build the RAM-based BBT\n");
+			pr_err("can't scan flash and build the RAM-based BBT\n");
 			kfree(this->bbt);
 			this->bbt = NULL;
 		}
-- 
1.7.0.4

Re: [PATCH v2 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Artem Bityutskiy]

On Wed, 2011-06-08 at 11:28 -0700, Brian Norris wrote:
> Also fix some capitalization that went along with the affected lines.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Thinking about this some more, I do not think it is good idea to
automatically prefix all messages with function name. In many cases this
just makes the kernel binary size larger without a real need.

> -				pr_info("nand_bbt: Error reading bad block table\n");
> +				pr_info("error reading bad block table\n");

Is nand_bbt: really needed here? May be this is not obvious, but isn't
this message unique anyway?

Well, this should be  pr_err(), because this is an error message :-) A
separate pass via all pr_* WRT this aspect would be nice.

>  				return res;
>  			}
> -			pr_warn("nand_bbt: ECC error while reading bad block table\n");
> +			pr_warn("ECC error while reading bad block table\n");

Is nand_bbt: really needed here?

>  		}
>  
>  		/* Analyse data */
> @@ -219,13 +221,13 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
>  				if (tmp == msk)
>  					continue;
>  				if (reserved_block_code && (tmp == reserved_block_code)) {
> -					pr_debug("nand_read_bbt: Reserved block at 0x%012llx\n",
> +					pr_debug("reserved block at 0x%012llx\n",
>  					       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);

OK, I turned this to pr_info, because this is not a debugging message.
And I do not think the function prefix is needed, I can grep the kernel
to find  it.

And really, in most places the function prefix is not needed.

In general, I think the function prefix is only needed in debugging
messages - dev_dbg() ones. All the other places should not require it in
most of the cases.

I'd do the following clean-ups instead:

1. Go through all messages and see if they are of proper level
(info/error/warning).
2. Go through all messages and thing if the function name prefix brings
   any value or only makes the kernel binary size larger.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output

[Mike Frysinger]

On Wed, Jun 8, 2011 at 14:23, Brian Norris wrote:
> On Wed, Jun 8, 2011 at 7:47 AM, Artem Bityutskiy wrote:
>> Compare:
>>
>> read_bbt: Error reading bad block table
>> read_bbt: error reading bad block table
>>
>> isn't the second version more correct? Or this depends on the country? I
>> prefer the latter, but as long as all messages are consistent, I do not
>> care much. What do you think?
>
> I'm not sure it's not so much language-related here, since these type
> of messages are just fragments anyway. I think the lowercase version
> is more consistent.

+1 to both
-mike