Re: [PATCH] ubi: Fix early logging

Re: [PATCH] ubi: Fix early logging

[Joe Perches]

From:	Richard Weinberger <richard@nod.at>
[]
> We must not use ubi_* log functions before the ubi_device
> struct is initialized.
> And while we are here, define a sane pr_fmt and add new lines to
> existing pr_* calls.

I think it'd be better to use and extend the patch I sent
for this problem.

https://patchwork.ozlabs.org/patch/587057/

Add a state check of the ubi pointer in the ubi_<level>
functions and do the right thing there.

Re: [PATCH] ubi: Fix early logging

[Richard Weinberger]

Am 25.02.2016 um 14:24 schrieb Joe Perches:
> From:	Richard Weinberger <richard@nod.at>
> []
>> We must not use ubi_* log functions before the ubi_device
>> struct is initialized.
>> And while we are here, define a sane pr_fmt and add new lines to
>> existing pr_* calls.
> 
> I think it'd be better to use and extend the patch I sent
> for this problem.
> 
> https://patchwork.ozlabs.org/patch/587057/
> 
> Add a state check of the ubi pointer in the ubi_<level>
> functions and do the right thing there.

Please send such a patch for UBI. :-)

Thanks,
//richard

[PATCH] mtd: ubi: Add logging functions ubi_msg, ubi_warn and ubi_err

[Joe Perches]

Using logging functions instead of macros can reduce overall object size.

$ size drivers/mtd/ubi/built-in.o*
   text	   data	    bss	    dec	    hex	filename
 271620	 163364	  73696	 508680	  7c308	drivers/mtd/ubi/built-in.o.allyesconfig.new
 287638	 165380	  73504	 526522	  808ba	drivers/mtd/ubi/built-in.o.allyesconfig.old
  87728	   3780	    504	  92012	  1676c	drivers/mtd/ubi/built-in.o.defconfig.new
  97084	   3780	    504	 101368	  18bf8	drivers/mtd/ubi/built-in.o.defconfig.old

Signed-off-by: Joe Perches <joe@perches.com>
---

>> I think it'd be better to use and extend the patch I sent
>> for this problem.
>> https://patchwork.ozlabs.org/patch/587057/
>> Add a state check of the ubi pointer in the ubi_<level>
>> functions and do the right thing there.

> Please send such a patch for UBI. :-)

ubifs/ubi, no big diff emirite?

And, OK.  This is a start on it anyway.

 drivers/mtd/ubi/misc.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/ubi.h  | 16 ++++++++++------
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/ubi/misc.c b/drivers/mtd/ubi/misc.c
index 2a45ac2..989036c 100644
--- a/drivers/mtd/ubi/misc.c
+++ b/drivers/mtd/ubi/misc.c
@@ -153,3 +153,52 @@ int ubi_check_pattern(const void *buf, uint8_t patt, int size)
 			return 0;
 	return 1;
 }
+
+/* Normal UBI messages */
+void ubi_msg(const struct ubi_device *ubi, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	pr_notice(UBI_NAME_STR "%d: %pV\n", ubi->ubi_num, &vaf);
+
+	va_end(args);
+}
+
+/* UBI warning messages */
+void ubi_warn(const struct ubi_device *ubi, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	pr_warn(UBI_NAME_STR "%d warning: %ps: %pV\n",
+		ubi->ubi_num, __builtin_return_address(0), &vaf);
+
+	va_end(args);
+}
+
+/* UBI error messages */
+void ubi_err(const struct ubi_device *ubi, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	pr_err(UBI_NAME_STR "%d error: %ps: %pV\n",
+	       ubi->ubi_num, __builtin_return_address(0), &vaf);
+	va_end(args);
+}
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 2974b67..dadc6a9 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -49,15 +49,19 @@
 /* UBI name used for character devices, sysfs, etc */
 #define UBI_NAME_STR "ubi"
 
+struct ubi_device;
+
 /* Normal UBI messages */
-#define ubi_msg(ubi, fmt, ...) pr_notice(UBI_NAME_STR "%d: " fmt "\n", \
-					 ubi->ubi_num, ##__VA_ARGS__)
+__printf(2, 3)
+void ubi_msg(const struct ubi_device *ubi, const char *fmt, ...);
+
 /* UBI warning messages */
-#define ubi_warn(ubi, fmt, ...) pr_warn(UBI_NAME_STR "%d warning: %s: " fmt "\n", \
-					ubi->ubi_num, __func__, ##__VA_ARGS__)
+__printf(2, 3)
+void ubi_warn(const struct ubi_device *ubi, const char *fmt, ...);
+
 /* UBI error messages */
-#define ubi_err(ubi, fmt, ...) pr_err(UBI_NAME_STR "%d error: %s: " fmt "\n", \
-				      ubi->ubi_num, __func__, ##__VA_ARGS__)
+__printf(2, 3)
+void ubi_err(const struct ubi_device *ubi, const char *fmt, ...);
 
 /* Background thread name pattern */
 #define UBI_BGT_NAME_PATTERN "ubi_bgt%dd"
-- 
2.6.3.368.gf34be46

Re: [PATCH] mtd: ubi: Add logging functions ubi_msg, ubi_warn and ubi_err

[Richard Weinberger]

On Thu, Feb 25, 2016 at 6:25 PM, Joe Perches <joe@perches.com> wrote:
> Using logging functions instead of macros can reduce overall object size.
>
> $ size drivers/mtd/ubi/built-in.o*
>    text    data     bss     dec     hex filename
>  271620  163364   73696  508680   7c308 drivers/mtd/ubi/built-in.o.allyesconfig.new
>  287638  165380   73504  526522   808ba drivers/mtd/ubi/built-in.o.allyesconfig.old
>   87728    3780     504   92012   1676c drivers/mtd/ubi/built-in.o.defconfig.new
>   97084    3780     504  101368   18bf8 drivers/mtd/ubi/built-in.o.defconfig.old
>
> Signed-off-by: Joe Perches <joe@perches.com>

Applied.

-- 
Thanks,
//richard

[PATCH] ubi: Fix early logging

[Richard Weinberger]

We must not use ubi_* log functions before the ubi_device
struct is initialized.
And while we are here, define a sane pr_fmt and add new lines to
existing pr_* calls.

Fixes: 32608703 ("UBI: Extend UBI layer debug/messaging capabilities")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/build.c | 24 ++++++++++++------------
 drivers/mtd/ubi/ubi.h   |  3 +++
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 59e1384..4945db4 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -876,7 +876,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	for (i = 0; i < UBI_MAX_DEVICES; i++) {
 		ubi = ubi_devices[i];
 		if (ubi && mtd->index == ubi->mtd->index) {
-			ubi_err(ubi, "mtd%d is already attached to ubi%d",
+			pr_err("mtd%d is already attached to ubi%d\n",
 				mtd->index, i);
 			return -EEXIST;
 		}
@@ -891,7 +891,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	 * no sense to attach emulated MTD devices, so we prohibit this.
 	 */
 	if (mtd->type == MTD_UBIVOLUME) {
-		ubi_err(ubi, "refuse attaching mtd%d - it is already emulated on top of UBI",
+		pr_err("refuse attaching mtd%d - it is already emulated on top of UBI\n",
 			mtd->index);
 		return -EINVAL;
 	}
@@ -902,7 +902,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 			if (!ubi_devices[ubi_num])
 				break;
 		if (ubi_num == UBI_MAX_DEVICES) {
-			ubi_err(ubi, "only %d UBI devices may be created",
+			pr_err("only %d UBI devices may be created\n",
 				UBI_MAX_DEVICES);
 			return -ENFILE;
 		}
@@ -912,7 +912,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 
 		/* Make sure ubi_num is not busy */
 		if (ubi_devices[ubi_num]) {
-			ubi_err(ubi, "already exists");
+			pr_err("already exists\n");
 			return -EEXIST;
 		}
 	}
@@ -1220,7 +1220,7 @@ static int __init ubi_init(void)
 	BUILD_BUG_ON(sizeof(struct ubi_vid_hdr) != 64);
 
 	if (mtd_devs > UBI_MAX_DEVICES) {
-		pr_err("UBI error: too many MTD devices, maximum is %d",
+		pr_err("UBI error: too many MTD devices, maximum is %d\n",
 		       UBI_MAX_DEVICES);
 		return -EINVAL;
 	}
@@ -1232,7 +1232,7 @@ static int __init ubi_init(void)
 
 	err = misc_register(&ubi_ctrl_cdev);
 	if (err) {
-		pr_err("UBI error: cannot register device");
+		pr_err("UBI error: cannot register device\n");
 		goto out;
 	}
 
@@ -1259,7 +1259,7 @@ static int __init ubi_init(void)
 		mtd = open_mtd_device(p->name);
 		if (IS_ERR(mtd)) {
 			err = PTR_ERR(mtd);
-			pr_err("UBI error: cannot open mtd %s, error %d",
+			pr_err("UBI error: cannot open mtd %s, error %d\n",
 			       p->name, err);
 			/* See comment below re-ubi_is_module(). */
 			if (ubi_is_module())
@@ -1272,7 +1272,7 @@ static int __init ubi_init(void)
 					 p->vid_hdr_offs, p->max_beb_per1024);
 		mutex_unlock(&ubi_devices_mutex);
 		if (err < 0) {
-			pr_err("UBI error: cannot attach mtd%d",
+			pr_err("UBI error: cannot attach mtd%d\n",
 			       mtd->index);
 			put_mtd_device(mtd);
 
@@ -1296,7 +1296,7 @@ static int __init ubi_init(void)
 
 	err = ubiblock_init();
 	if (err) {
-		pr_err("UBI error: block: cannot initialize, error %d", err);
+		pr_err("UBI error: block: cannot initialize, error %d\n", err);
 
 		/* See comment above re-ubi_is_module(). */
 		if (ubi_is_module())
@@ -1319,7 +1319,7 @@ out_dev_unreg:
 	misc_deregister(&ubi_ctrl_cdev);
 out:
 	class_unregister(&ubi_class);
-	pr_err("UBI error: cannot initialize UBI, error %d", err);
+	pr_err("UBI error: cannot initialize UBI, error %d\n", err);
 	return err;
 }
 late_initcall(ubi_init);
@@ -1447,7 +1447,7 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
 		int err = kstrtoint(token, 10, &p->max_beb_per1024);
 
 		if (err) {
-			pr_err("UBI error: bad value for max_beb_per1024 parameter: %s",
+			pr_err("UBI error: bad value for max_beb_per1024 parameter: %s\n",
 			       token);
 			return -EINVAL;
 		}
@@ -1458,7 +1458,7 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
 		int err = kstrtoint(token, 10, &p->ubi_num);
 
 		if (err) {
-			pr_err("UBI error: bad value for ubi_num parameter: %s",
+			pr_err("UBI error: bad value for ubi_num parameter: %s\n",
 			       token);
 			return -EINVAL;
 		}
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 915e4dd..93872d4 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -22,6 +22,9 @@
 #ifndef __UBI_UBI_H__
 #define __UBI_UBI_H__
 
+#undef pr_fmt
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/rbtree.h>
-- 
1.8.4.5