[PATCH 1/2] mtd: nand: generalized error messages with __func__

[PATCH 1/2] mtd: nand: generalized error messages with __func__

[Brian Norris]

These simple printk error messages can be a little simpler to maintain
when they use the __func__ identifier.

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

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ccbeaa1..7936a6c 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -1164,7 +1164,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 	/* Allocate memory (2bit per block) and clear the memory bad block table */
 	this->bbt = kzalloc(len, GFP_KERNEL);
 	if (!this->bbt) {
-		printk(KERN_ERR "nand_scan_bbt: Out of memory\n");
+		printk(KERN_ERR "%s: Out of memory\n", __func__);
 		return -ENOMEM;
 	}
 
@@ -1187,7 +1187,7 @@ 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");
+		printk(KERN_ERR "%s: Out of memory\n", __func__);
 		kfree(this->bbt);
 		this->bbt = NULL;
 		return -ENOMEM;
@@ -1237,7 +1237,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
 	len += (len >> this->page_shift) * mtd->oobsize;
 	buf = kmalloc(len, GFP_KERNEL);
 	if (!buf) {
-		printk(KERN_ERR "nand_update_bbt: Out of memory\n");
+		printk(KERN_ERR "%s: Out of memory\n", __func__);
 		return -ENOMEM;
 	}
 
@@ -1351,7 +1351,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
 	}
 	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
 	if (!bd) {
-		printk(KERN_ERR "nand_create_default_bbt_descr: Out of memory\n");
+		printk(KERN_ERR "%s: Out of memory\n", __func__);
 		return -ENOMEM;
 	}
 	bd->options = this->options & BBT_SCAN_OPTIONS;
diff --git a/drivers/mtd/nand/rtc_from4.c b/drivers/mtd/nand/rtc_from4.c
index c9f9127..7e02c94 100644
--- a/drivers/mtd/nand/rtc_from4.c
+++ b/drivers/mtd/nand/rtc_from4.c
@@ -444,7 +444,7 @@ 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");
+			printk(KERN_ERR "%s: Out of memory!\n", __func__);
 			er_stat = 1;
 			goto out;
 		}
-- 
1.7.0.4

[PATCH 2/2] mtd: nand: multi-line comment style fixups

[Brian Norris]

From: Brian Norris <norris@broadcom.com>

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a46e9bb..f591c21 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -410,7 +410,8 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	else {
 		nand_get_device(chip, mtd, FL_WRITING);
 
-		/* Write to first two pages and to byte 1 and 6 if necessary.
+		/*
+		 * Write to first two pages and to byte 1 and 6 if necessary.
 		 * If we write to more than one location, the first error
 		 * encountered quits the procedure. We write two bytes per
 		 * location, so we dont have to mess with 16 bit access.
@@ -625,8 +626,10 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 			return;
 		}
 	}
-	/* Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine. */
+	/*
+	 * Apply this short delay always to ensure that we do wait tWB in
+	 * any case on any machine.
+	 */
 	ndelay(100);
 
 	nand_wait_ready(mtd);
@@ -747,8 +750,10 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 		}
 	}
 
-	/* Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine. */
+	/*
+	 * Apply this short delay always to ensure that we do wait tWB in
+	 * any case on any machine.
+	 */
 	ndelay(100);
 
 	nand_wait_ready(mtd);
@@ -859,8 +864,10 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 
 	led_trigger_event(nand_led_trigger, LED_FULL);
 
-	/* Apply this short delay always to ensure that we do wait tWB in
-	 * any case on any machine. */
+	/*
+	 * Apply this short delay always to ensure that we do wait tWB in
+	 * any case on any machine.
+	 */
 	ndelay(100);
 
 	if ((state == FL_ERASING) && (chip->options & NAND_IS_AND))
@@ -1187,9 +1194,10 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	for (i = 0; i < eccfrag_len ; i += chip->ecc.bytes, p += chip->ecc.size)
 		chip->ecc.calculate(mtd, p, &chip->buffers->ecccalc[i]);
 
-	/* The performance is faster if to position offsets
-	   according to ecc.pos. Let make sure here that
-	   there are no gaps in ecc positions */
+	/*
+	 * The performance is faster if we position offsets according to
+	 * ecc.pos. Let's make sure that there are no gaps in ecc positions.
+	 */
 	for (i = 0; i < eccfrag_len - 1; i++) {
 		if (eccpos[i + start_step * chip->ecc.bytes] + 1 !=
 			eccpos[i + start_step * chip->ecc.bytes + 1]) {
@@ -1552,7 +1560,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 			chip->select_chip(mtd, chipnr);
 		}
 
-		/* Check, if the chip supports auto page increment
+		/*
+		 * Check, if the chip supports auto page increment
 		 * or if we have hit a block boundary.
 		 */
 		if (!NAND_CANAUTOINCR(chip) || !(page & blkcheck))
@@ -1830,7 +1839,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 			chip->select_chip(mtd, chipnr);
 		}
 
-		/* Check, if the chip supports auto page increment
+		/*
+		 * Check, if the chip supports auto page increment
 		 * or if we have hit a block boundary.
 		 */
 		if (!NAND_CANAUTOINCR(chip) || !(page & blkcheck))
@@ -2924,7 +2934,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	*maf_id = chip->read_byte(mtd);
 	*dev_id = chip->read_byte(mtd);
 
-	/* Try again to make sure, as some systems the bus-hold or other
+	/*
+	 * Try again to make sure, as some systems the bus-hold or other
 	 * interface concerns can cause random data which looks like a
 	 * possibly credible NAND flash to appear. If the two results do
 	 * not match, ignore the device completely.
@@ -2973,7 +2984,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	chip->chipsize = (uint64_t)type->chipsize << 20;
 
 	if (!type->pagesize && chip->init_size) {
-		/* set the pagesize, oobsize, erasesize by the driver*/
+		/* set the pagesize, oobsize, erasesize by the driver */
 		busw = chip->init_size(mtd, chip, id_data);
 	} else if (!type->pagesize) {
 		int extid;
@@ -3057,8 +3068,9 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	chip->options &= ~NAND_CHIPOPTIONS_MSK;
 	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
 
-	/* Check if chip is a not a samsung device. Do not clear the
-	 * options for chips which are not having an extended id.
+	/*
+	 * Check if chip is not a Samsung device. Do not clear the
+	 * options for chips which do not have an extended id.
 	 */
 	if (*maf_id != NAND_MFR_SAMSUNG && !type->pagesize)
 		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
@@ -3481,9 +3493,11 @@ int nand_scan_tail(struct mtd_info *mtd)
 }
 EXPORT_SYMBOL(nand_scan_tail);
 
-/* is_module_text_address() isn't exported, and it's mostly a pointless
+/*
+ * is_module_text_address() isn't exported, and it's mostly a pointless
  * test if this is a module _anyway_ -- they'd have to try _really_ hard
- * to call us from in-kernel code if the core NAND support is modular. */
+ * to call us from in-kernel code if the core NAND support is modular.
+ */
 #ifdef MODULE
 #define caller_is_module() (1)
 #else
diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 7936a6c..64a3cd5 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -258,8 +258,10 @@ 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 its matured we can move this
-				 * message to MTD_DEBUG_LEVEL0 */
+				/*
+				 * 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",
 				       (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
 				/* Factory marked bad or worn out ? */
@@ -529,8 +531,10 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
 	}
 
 	if (chip == -1) {
-		/* Note that numblocks is 2 * (real numblocks) here, see i+=2
-		 * below as it makes shifting and masking less painful */
+		/*
+		 * Note that numblocks is 2 * (real numblocks) here, see i+=2
+		 * below as it makes shifting and masking less painful
+		 */
 		numblocks = mtd->size >> (this->bbt_erase_shift - 1);
 		startblock = 0;
 		from = 0;
@@ -732,7 +736,8 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	/* Loop through the chips */
 	for (; chip < nrchips; chip++) {
 
-		/* There was already a version of the table, reuse the page
+		/*
+		 * There was already a version of the table, reuse the page
 		 * This applies for absolute placement too, as we have the
 		 * page nr. in td->pages.
 		 */
@@ -1082,9 +1087,11 @@ static void mark_bbt_region(struct mtd_info *mtd, struct nand_bbt_descr *td)
 				update = 1;
 			block += 2;
 		}
-		/* If we want reserved blocks to be recorded to flash, and some
-		   new ones have been marked, then we need to update the stored
-		   bbts.  This should only happen once. */
+		/*
+		 * If we want reserved blocks to be recorded to flash, and some
+		 * new ones have been marked, then we need to update the stored
+		 * bbts.  This should only happen once.
+		 */
 		if (update && td->reserved_block_code)
 			nand_update_bbt(mtd, (loff_t)(block - 2) << (this->bbt_erase_shift - 1));
 	}
@@ -1168,7 +1175,8 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
 		return -ENOMEM;
 	}
 
-	/* If no primary table decriptor is given, scan the device
+	/*
+	 * If no primary table decriptor is given, scan the device
 	 * to build a memory based bad block table
 	 */
 	if (!td) {
@@ -1272,8 +1280,10 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
 	return res;
 }
 
-/* Define some generic bad / good block scan pattern which are used
- * while scanning a device for factory marked good / bad blocks. */
+/*
+ * Define some generic bad / good block scan pattern which are used
+ * while scanning a device for factory marked good / bad blocks.
+ */
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static uint8_t scan_agand_pattern[] = { 0x1C, 0x71, 0xC7, 0x1C, 0x71, 0xC7 };
@@ -1285,8 +1295,7 @@ static struct nand_bbt_descr agand_flashbased = {
 	.pattern = scan_agand_pattern
 };
 
-/* Generic flash bbt decriptors
-*/
+/* Generic flash bbt decriptors */
 static uint8_t bbt_pattern[] = {'B', 'b', 't', '0' };
 static uint8_t mirror_pattern[] = {'1', 't', 'b', 'B' };
 
@@ -1375,7 +1384,8 @@ int nand_default_bbt(struct mtd_info *mtd)
 {
 	struct nand_chip *this = mtd->priv;
 
-	/* Default for AG-AND. We must use a flash based
+	/*
+	 * Default for AG-AND. We must use a flash based
 	 * bad block table as the devices have factory marked
 	 * _good_ blocks. Erasing those blocks leads to loss
 	 * of the good / bad information, so we _must_ store
-- 
1.7.0.4

Re: [PATCH 2/2] mtd: nand: multi-line comment style fixups

[Artem Bityutskiy]

On Wed, 2011-05-25 at 14:59 -0700, Brian Norris wrote:
> From: Brian Norris <norris@broadcom.com>
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I kind of rejected the first patch in this series, but forgot about this
one. I've now pushed it to the l2 tree with additional comments
clean-ups from me - just did them while looking at your patch and to the
code, and accidentally used git commit --amend, so they were merged with
yours. But this should not be a big deal - we ended up with one big
comments cleanup patch.

Thanks - it is in the l2-mtd-2.6.git now.

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

Re: [PATCH 2/2] mtd: nand: multi-line comment style fixups

[Brian Norris]

On Mon, Jun 6, 2011 at 7:26 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> I kind of rejected the first patch in this series, but forgot about this
> one. I've now pushed it to the l2 tree with additional comments
> clean-ups from me - just did them while looking at your patch and to the
> code, and accidentally used git commit --amend, so they were merged with
> yours. But this should not be a big deal - we ended up with one big
> comments cleanup patch.
>
> Thanks - it is in the l2-mtd-2.6.git now.

No problem. Thanks.

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Igor Grinberg]

On 05/26/11 00:59, Brian Norris wrote:

> These simple printk error messages can be a little simpler to maintain
> when they use the __func__ identifier.

While this is a good thing you are doing, I'd suggest using pr_err macro
and may be even pr_fmt.

pr_err() will save you from the need to define the log level each time.
pr_fmt will save you the need to add %s: and __func__.

> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/nand/nand_bbt.c  |    8 ++++----
>  drivers/mtd/nand/rtc_from4.c |    2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index ccbeaa1..7936a6c 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -1164,7 +1164,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
>  	/* Allocate memory (2bit per block) and clear the memory bad block table */
>  	this->bbt = kzalloc(len, GFP_KERNEL);
>  	if (!this->bbt) {
> -		printk(KERN_ERR "nand_scan_bbt: Out of memory\n");
> +		printk(KERN_ERR "%s: Out of memory\n", __func__);
>  		return -ENOMEM;
>  	}
>  
> @@ -1187,7 +1187,7 @@ 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");
> +		printk(KERN_ERR "%s: Out of memory\n", __func__);
>  		kfree(this->bbt);
>  		this->bbt = NULL;
>  		return -ENOMEM;
> @@ -1237,7 +1237,7 @@ int nand_update_bbt(struct mtd_info *mtd, loff_t offs)
>  	len += (len >> this->page_shift) * mtd->oobsize;
>  	buf = kmalloc(len, GFP_KERNEL);
>  	if (!buf) {
> -		printk(KERN_ERR "nand_update_bbt: Out of memory\n");
> +		printk(KERN_ERR "%s: Out of memory\n", __func__);
>  		return -ENOMEM;
>  	}
>  
> @@ -1351,7 +1351,7 @@ static int nand_create_default_bbt_descr(struct nand_chip *this)
>  	}
>  	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
>  	if (!bd) {
> -		printk(KERN_ERR "nand_create_default_bbt_descr: Out of memory\n");
> +		printk(KERN_ERR "%s: Out of memory\n", __func__);
>  		return -ENOMEM;
>  	}
>  	bd->options = this->options & BBT_SCAN_OPTIONS;
> diff --git a/drivers/mtd/nand/rtc_from4.c b/drivers/mtd/nand/rtc_from4.c
> index c9f9127..7e02c94 100644
> --- a/drivers/mtd/nand/rtc_from4.c
> +++ b/drivers/mtd/nand/rtc_from4.c
> @@ -444,7 +444,7 @@ 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");
> +			printk(KERN_ERR "%s: Out of memory!\n", __func__);
>  			er_stat = 1;
>  			goto out;
>  		}

-- 
Regards,
Igor.

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Artem Bityutskiy]

On Thu, 2011-05-26 at 08:14 +0300, Igor Grinberg wrote:
> On 05/26/11 00:59, Brian Norris wrote:
> 
> > These simple printk error messages can be a little simpler to maintain
> > when they use the __func__ identifier.
> 
> While this is a good thing you are doing, I'd suggest using pr_err macro
> and may be even pr_fmt.
> 
> pr_err() will save you from the need to define the log level each time.
> pr_fmt will save you the need to add %s: and __func__.

Right, but in this _particular_ case the best thing to do is to just
kill the error messages - if any of the allocation functions fail they
print a large scary warning with a backtrace anyway, and the backtrace
will contain the caller function names.

Brian, would you please instead just zap the prints?

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

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Brian Norris]

Hi,

Sorry if this is a second message anyone, I didn't hit Reply-All.

On Wed, May 25, 2011 at 10:16 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Right, but in this _particular_ case the best thing to do is to just
> kill the error messages - if any of the allocation functions fail they
> print a large scary warning with a backtrace anyway, and the backtrace
> will contain the caller function names.
>
> Brian, would you please instead just zap the prints?

Sure, can do. In general, then, is it best practice to simply 'return
-ENOMEM' or similar (without any printing) in all MTD code?

> On Thu, 2011-05-26 at 08:14 +0300, Igor Grinberg wrote:
>> On 05/26/11 00:59, Brian Norris wrote:
>> pr_err() will save you from the need to define the log level each time.
>> pr_fmt will save you the need to add %s: and __func__.

Also, it seems that the second statement is not necessarily true. In
fact, in include/linux/printk.h, we default to the following
declaration:
     #define pr_fmt(fmt) fmt
So it seems that, when we desire to print out the relevant function
name, we should manually use __func__ instead of relying on
pr_fmt...or should we define our own pr_fmt in include/linux/mtd.h?

Brian

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Mike Frysinger]

On Tue, May 31, 2011 at 14:52, Brian Norris wrote:
>> On Thu, 2011-05-26 at 08:14 +0300, Igor Grinberg wrote:
>>> On 05/26/11 00:59, Brian Norris wrote:
>>> pr_err() will save you from the need to define the log level each time.
>>> pr_fmt will save you the need to add %s: and __func__.
>
> Also, it seems that the second statement is not necessarily true. In
> fact, in include/linux/printk.h, we default to the following
> declaration:
>     #define pr_fmt(fmt) fmt
> So it seems that, when we desire to print out the relevant function
> name, we should manually use __func__ instead of relying on
> pr_fmt...or should we define our own pr_fmt in include/linux/mtd.h?

while it might run afoul of some people's sense of style, it is
technically possible.  try something like this:
#define pr_fmt(fmt) "%s: " fmt, __func__
-mike

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Artem Bityutskiy]

Hi,

On Tue, 2011-05-31 at 11:52 -0700, Brian Norris wrote:
> > Brian, would you please instead just zap the prints?
> 
> Sure, can do. In general, then, is it best practice to simply 'return
> -ENOMEM' or similar (without any printing) in all MTD code?

Yes.

> > On Thu, 2011-05-26 at 08:14 +0300, Igor Grinberg wrote:
> >> On 05/26/11 00:59, Brian Norris wrote:
> >> pr_err() will save you from the need to define the log level each time.
> >> pr_fmt will save you the need to add %s: and __func__.
> 
> Also, it seems that the second statement is not necessarily true. In
> fact, in include/linux/printk.h, we default to the following
> declaration:
>      #define pr_fmt(fmt) fmt

To be more precise:

#ifndef pr_fmt
#define pr_fmt(fmt) fmt
#endif

> So it seems that, when we desire to print out the relevant function
> name, we should manually use __func__ instead of relying on
> pr_fmt...or should we define our own pr_fmt in include/linux/mtd.h?

Yes, before you include files.

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

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Brian Norris]

On Wed, Jun 1, 2011 at 3:30 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> or should we define our own pr_fmt in include/linux/mtd.h?
>
> Yes, before you include files.

Well, I've tried this, and as I suspected, I can't just include it in
mtd.h at the top, since we can't guarantee that <linux/mtd/mtd.h> is
going to be the first included header in other files. Specifically, in
include/linux/mtd/nand.h, we include other headers before mtd.h and so
printk.h has already defined pr_fmt() for us, so this doesn't work.
I'm not sure if there's a nice place to define pr_fmt() right now...

Brian

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Artem Bityutskiy]

On Wed, 2011-06-01 at 11:37 -0700, Brian Norris wrote:
> On Wed, Jun 1, 2011 at 3:30 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> or should we define our own pr_fmt in include/linux/mtd.h?
> >
> > Yes, before you include files.
> 
> Well, I've tried this, and as I suspected, I can't just include it in
> mtd.h at the top, since we can't guarantee that <linux/mtd/mtd.h> is
> going to be the first included header in other files.

In any case we do not want to blow the kernel binary size by too many
__func__ strings. They should be used only when needed.

>  Specifically, in
> include/linux/mtd/nand.h, we include other headers before mtd.h and so
> printk.h has already defined pr_fmt() for us, so this doesn't work.
> I'm not sure if there's a nice place to define pr_fmt() right now...

.c files, AFAIU.

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

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Igor Grinberg]

On 06/01/11 21:37, Brian Norris wrote:

> On Wed, Jun 1, 2011 at 3:30 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>>> or should we define our own pr_fmt in include/linux/mtd.h?
>> Yes, before you include files.
> Well, I've tried this, and as I suspected, I can't just include it in
> mtd.h at the top, since we can't guarantee that <linux/mtd/mtd.h> is
> going to be the first included header in other files. Specifically, in
> include/linux/mtd/nand.h, we include other headers before mtd.h and so
> printk.h has already defined pr_fmt() for us, so this doesn't work.
> I'm not sure if there's a nice place to define pr_fmt() right now...

You should not define pr_fmt() in .h file.
pr_fmt() should be defined in _.c_ file at the top - _before any include is done_.
And yes, like Artem already said, _only_ in those _.c_ files where we want to use it.


-- 
Regards,
Igor.

Re: [PATCH 1/2] mtd: nand: generalized error messages with __func__

[Brian Norris]

On Tue, Jun 7, 2011 at 12:26 AM, Igor Grinberg <grinberg@compulab.co.il> wrote:
> You should not define pr_fmt() in .h file.
> pr_fmt() should be defined in _.c_ file at the top - _before any include is done_.
> And yes, like Artem already said, _only_ in those _.c_ files where we want to use it.

Hmm, well I guess I'll try defining pr_fmt() in nand_bbt.c only. Look
for the following patchset shortly:
* removing those printk's when a memory allocations fail
* changing other printk's to pr_*'s
* changing some DEBUG messages to dev_dbg
* define pr_fmt() to include __func__ in debug messages for nand_bbt.c

The only inconsistency with the 4th patch is that it 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