UBI: Extend UBI layer debug/messaging capabilities - cosmetics
PATCH 1/1 UBI: Extend UBI layer debug/messaging capabilities - cosmetics
Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging capabilities".
---
Changes from original patch:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn Removed extra printing of ubi number Removed new messages.
drivers/mtd/ubi/build.c | 6 +++--- drivers/mtd/ubi/cdev.c | 9 ++++----- drivers/mtd/ubi/io.c | 3 +-- drivers/mtd/ubi/ubi.h | 9 ++++++--- drivers/mtd/ubi/vtbl.c | 7 +++---- drivers/mtd/ubi/wl.c | 10 ++-------- 6 files changed, 19 insertions(+), 25 deletions(-)
drivers/mtd/ubi/build.c +3 -3
@@ -923,7 +923,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,"ubi%d already exists" , ubi_num); +ubi_err (ubi,"already exists" );return -EEXIST ; } }
@@ -973,7 +973,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,mutex_init (&ubi->fm_mutex);init_rwsem (&ubi->fm_sem); -ubi_msg (ubi,"attaching mtd%d to ubi%d" , mtd->index, ubi_num); +ubi_msg (ubi,"attaching mtd%d" , mtd->index); err =io_init (ubi, max_beb_per1024);if (err)
@@ -1428,7 +1428,7 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp) }if (len ==0 ) { -pr_err ("UBI warning: empty 'mtd=' parameter - ignored\n" ); +pr_warn ("UBI warning: empty 'mtd=' parameter - ignored\n" );return 0 ; }
drivers/mtd/ubi/cdev.c +4 -5
@@ -48,14 +48,13 @@/** * get_exclusive - get exclusive access to an UBI volume. -* @ubi: UBI device description object * @desc: volume descriptor * * This function changes UBI volume open mode to "exclusive". Returns previous * mode value (positive integer) in case of success and a negative error code * in case of failure. */ -static int get_exclusive (struct ubi_device *ubi,struct ubi_volume_desc *desc) +static int get_exclusive (struct ubi_volume_desc *desc) {int users, err;struct ubi_volume *vol = desc->vol;
@@ -64,7 +63,7 @@ static int get_exclusive(struct ubi_device *ubi, struct ubi_volume_desc *desc) users = vol->readers + vol->writers + vol->exclusive;ubi_assert (users >0 );if (users >1 ) { -ubi_err (ubi,"%d users for volume %d" , users, vol->vol_id); +ubi_err (vol->ubi,"%d users for volume %d" , users, vol->vol_id); err = -EBUSY ; }else { vol->readers = vol->writers =0 ;
@@ -421,7 +420,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,break ; } - err =get_exclusive (ubi, desc); + err =get_exclusive (desc);if (err <0 )break ;
@@ -457,7 +456,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd, req.bytes < 0 || req.lnum >= vol->usable_leb_size)break ; - err =get_exclusive (ubi, desc); + err =get_exclusive (desc);if (err <0 )break ;
drivers/mtd/ubi/io.c +1 -2
@@ -1419,8 +1419,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len) fail:ubi_err (ubi,"self-check failed for PEB %d" , pnum); -ubi_msg (ubi,"hex dump of the %d-%d region" , - offset, offset + len); +ubi_msg (ubi,"hex dump of the %d-%d region" , offset, offset + len);print_hex_dump (KERN_DEBUG ,"" ,DUMP_PREFIX_OFFSET ,32 ,1 , buf, len,1 ); err = -EINVAL ; error:
drivers/mtd/ubi/ubi.h +6 -3
@@ -51,13 +51,16 @@/* Normal UBI messages */ #define ubi_msg (ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* Background thread name pattern */ #define UBI_BGT_NAME_PATTERN "ubi_bgt%dd"
drivers/mtd/ubi/vtbl.c +3 -4
@@ -655,14 +655,13 @@ static int init_volumes(struct ubi_device *ubi,/** * check_av - check volume attaching information. -* @ubi: UBI device description object * @vol: UBI volume description object * @av: volume attaching information * * This function returns zero if the volume attaching information is consistent * to the data read from the volume tabla, and %-EINVAL if not. */ -static int check_av (const struct ubi_device *ubi,const struct ubi_volume *vol, +static int check_av (const struct ubi_volume *vol,const struct ubi_ainf_volume *av) {int err;
@@ -690,7 +689,7 @@ static int check_av(const struct ubi_device *ubi, const struct ubi_volume *vol,return 0 ; bad: -ubi_err (ubi,"bad attaching information, error %d" , err); +ubi_err (vol->ubi,"bad attaching information, error %d" , err);ubi_dump_av (av);ubi_dump_vol_info (vol);return -EINVAL ;
@@ -753,7 +752,7 @@ static int check_attaching_info(const struct ubi_device *ubi,ubi_msg (ubi,"finish volume %d removal" , av->vol_id);ubi_remove_av (ai, av); }else if (av) { - err =check_av (ubi, vol, av); + err =check_av (vol, av);if (err)return err; }
drivers/mtd/ubi/wl.c +2 -8
@@ -470,11 +470,8 @@ struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device *ubi, int anchor) {struct ubi_wl_entry *e =NULL ; -if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) { -ubi_warn (ubi,"Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d" , - anchor, ubi->free_count, ubi->beb_rsvd_pebs); +if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) goto out; - }if (anchor) e =find_anchor_wl_entry (&ubi->free);
@@ -1806,11 +1803,8 @@ int ubi_thread(void *u)for (;;) {int err; -if (kthread_should_stop ()) { -ubi_msg (ubi,"background thread \"%s\" should stop, PID %d" , - ubi->bgt_name,task_pid_nr (current)); +if (kthread_should_stop ())break ; - }if (try_to_freeze ())continue ;
Signature · 3 lines
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 6 lines · Tanya Brokhman
/* Normal UBI messages */
#defineubi_msg(ubi, fmt, ...)pr_notice("UBI-%d: %s:" fmt "\n", \drivers/mtd/ubi/ubi.h +2
-1- ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num :UBI_MAX_DEVICES ), \ + __func__, ##__VA_ARGS__)
drivers/mtd/ubi/ubi.h line 63
Quoted 10 lines
drivers/mtd/ubi/ubi.h +4
-2/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)
Why did you make these changes? It is preferable to not add another 'if' statement to this macro to handle one or 2 cases - much bloat, little gain.
Could we please avoid this?
drivers/mtd/ubi/wl.c struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device *ubi, int anchor) · line 474
Quoted 6 lines
drivers/mtd/ubi/wl.c +1
-3-if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) { -ubi_warn (ubi,"Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d" , - anchor, ubi->free_count, ubi->beb_rsvd_pebs); +if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) goto out;
The warning looks pretty poor, so I do not mind to remove it, but I thought your patch is about adding a parameter, but you mix different kinds of things there. Please, be stricter to the similar UBIFS patch which you was going to send.
drivers/mtd/ubi/wl.c int ubi_thread(void *u) · line 1813
Quoted 6 lines
drivers/mtd/ubi/wl.c +1
-4-if (kthread_should_stop ()) { -ubi_msg (ubi,"background thread \"%s\" should stop, PID %d" , - ubi->bgt_name,task_pid_nr (current)); +if (kthread_should_stop ())break ; - }
How about just turning this into a debug message, not removing?
Artem.
Quoted 18 lines · Artem Bityutskiy
Quoted 6 lines · Tanya Brokhman
/* Normal UBI messages */ #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \drivers/mtd/ubi/ubi.h +2
-1- ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num :UBI_MAX_DEVICES ), \ + __func__, ##__VA_ARGS__)drivers/mtd/ubi/ubi.h line 63
Quoted 10 lines
drivers/mtd/ubi/ubi.h +4
-2/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)
Quoted 5 lines
Why did you make these changes? It is preferable to not add another 'if' statement to this macro to handle one or 2 cases - much bloat, little gain.
Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called with ubi=NULL that may crash the system. If you still prefer the "if" removed will do.
Quoted 8 lines
drivers/mtd/ubi/wl.c struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device *ubi, int anchor) · line 474
Quoted 7 lines
drivers/mtd/ubi/wl.c +1
-3-if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) { -ubi_warn (ubi,"Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d" , - anchor, ubi->free_count, ubi->beb_rsvd_pebs); +if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1 )) goto out;
Quoted 4 lines
The warning looks pretty poor, so I do not mind to remove it, but I thought your patch is about adding a parameter, but you mix different kinds of things there. Please, be stricter to the similar UBIFS patch which you was going to send.
Now I'm confused. I added this msg as part of the patch you already pushed to your branch but later you requested NOT to add additional msgs and if required add it in a different patch. So this was added by me and now removed by me - as per your request.
Quoted 10 lines
drivers/mtd/ubi/wl.c int ubi_thread(void *u) · line 1813
Quoted 8 lines
drivers/mtd/ubi/wl.c +1
-4-if (kthread_should_stop ()) { -ubi_msg (ubi,"background thread \"%s\" should stop, PID %d" , - ubi->bgt_name,task_pid_nr (current)); +if (kthread_should_stop ())break ; - }How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
"Yes, please, remove these messages or turn them into debugging messages. And yes, these should have been added in a separate patch."
Quoted 3 lines
Artem.
Thanks,
Tanya Brokhman
Signature · 3 lines
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 23 lines · Tanya Brokhman
Quoted 22 lines · Artem Bityutskiy
Quoted 6 lines · Tanya Brokhman
/* Normal UBI messages */ #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \drivers/mtd/ubi/ubi.h +2
-1- ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num :UBI_MAX_DEVICES ), \ + __func__, ##__VA_ARGS__)drivers/mtd/ubi/ubi.h line 63
Quoted 10 lines
drivers/mtd/ubi/ubi.h +4
-2/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)Why did you make these changes? It is preferable to not add another 'if' statement to this macro to handle one or 2 cases - much bloat, little gain.
Quoted 5 lines
Quoted 1 line
Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called with ubi=NULL that may crash the system. If you still prefer the "if" removed will do.
On the other hand, these are macros, and this if gets duplicated in many places and translate into few additional assembly instructions per message.
Quoted 9 lines
Quoted 4 lines
The warning looks pretty poor, so I do not mind to remove it, but I thought your patch is about adding a parameter, but you mix different kinds of things there. Please, be stricter to the similar UBIFS patch which you was going to send.
Now I'm confused. I added this msg as part of the patch you already pushed to your branch but later you requested NOT to add additional msgs and if required add it in a different patch. So this was added by me and now removed by me - as per your request.
This comment of mine just repeats that request. It talks about being stricter in the future patches and not add/remove messages. It does not request to modify this patch. IOW, this change is OK, but please, let's make sure we do not have them in the UBIFS patch.
Quoted 6 lines
Quoted 1 line
How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
"Yes, please, remove these messages or turn them into debugging messages. And yes, these should have been added in a separate patch."
OK, just asking.
Quoted 26 lines · Artem Bityutskiy
Quoted 25 lines · Tanya Brokhman
Quoted 23 lines · Artem Bityutskiy
Quoted 6 lines · Tanya Brokhman
/* Normal UBI messages */ #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \drivers/mtd/ubi/ubi.h +2
-1- ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num :UBI_MAX_DEVICES ), \ + __func__, ##__VA_ARGS__)drivers/mtd/ubi/ubi.h line 63
Quoted 10 lines
drivers/mtd/ubi/ubi.h +4
-2/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__, ##__VA_ARGS__) + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)Why did you make these changes? It is preferable to not add another 'if' statement to this macro to handle one or 2 cases - much bloat, little gain.
Could we please avoid this?
Quoted 7 lines
Quoted 3 lines
I just wanted to be on the safe side and prevent this macro being called with ubi=NULL that may crash the system. If you still prefer the "if" removed will do.
On the other hand, these are macros, and this if gets duplicated in many places and translate into few additional assembly instructions per message.
The thing that will make these uses smaller is to
convert them to functions.
There is a lot of extra duplicated "UBI-%s <msg_type>: " constant string .text added.
Using a function uses a single copy of each prefix.
The __func__ variable can also be removed. __builtin_return_address(0) may be substituted to save a few more bytes per instance.
Something like:
(prototype)__printf(2, 3)
void ubi_warn(struct ubi *ubi, const char *fmt, ...);
(implementation)__printf(2, 3)
void ubi_warn(struct ubi *ubi, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
int device;
va_start(args, format);
vaf.fmt = format;
vaf.va = &args;
if (!ubi)
device = UBI_MAX_DEVICE;
else
device = ubi->ubi_num;
pr_warn("UBI-%d warning: %pf: %pV",
device, __builtin_return_address(0), &vaf);
va_end(args);
}Quoted 55 lines · Tanya Brokhman
Quoted 25 lines · Artem Bityutskiy
Quoted 19 lines · Tanya Brokhman
/* Normal UBI messages */
#defineubi_msg(ubi, fmt, ...)pr_notice("UBI-%d: %s:" fmt "\n", \
-ubi->ubi_num,__func__,
##__VA_ARGS__)drivers/mtd/ubi/ubi.h +2
-0+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI warning messages */ #define ubi_warn (ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt"\n", \
-ubi->ubi_num,__func__,
##__VA_ARGS__)drivers/mtd/ubi/ubi.h +2
-1+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)/* UBI error messages */ #define ubi_err (ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \ - ubi->ubi_num, __func__,##__VA_ARGS__)
Review
Quoted 2 lines
(quoted diff) +2
-0+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \ + __func__, ##__VA_ARGS__)Quoted 5 lines
Why did you make these changes? It is preferable to not add another 'if' statement to this macro to handle one or 2 cases - much bloat, little gain.
Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called with ubi=NULL that may crash the system. If you still prefer the "if" removed will do.
Quoted 12 lines
Quoted 10 lines
- if (!ubi->free.rb_node || (
ubi->free_count-ubi->beb_rsvd_pebs<
1)) {
- ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d,
free_cnt=%d, reserved=%d",(quoted diff) +1
-1- anchor, ubi->free_count, ubi->beb_rsvd_pebs); + if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <1))
goto out;Quoted 4 lines
The warning looks pretty poor, so I do not mind to remove it, but I thought your patch is about adding a parameter, but you mix different kinds of things there. Please, be stricter to the similar UBIFS patch which you was going to send.
Quoted 4 lines
Now I'm confused. I added this msg as part of the patch you already pushed to your branch but later you requested NOT to add additional msgs and if required add it in a different patch. So this was added by me and now removed by me - as per your request.
Why do you need that new warning anyways?
It was added by "UBI: Extend UBI layer debug/messaging capabilities".
Quoted 35 lines
Quoted 11 lines
drivers/mtd/ubi/wl.c int ubi_thread(void *u) · line 1813
Quoted 9 lines
(quoted diff) +0
-2- if (kthread_should_stop()) { - ubi_msg(ubi, "background thread \"%s\" shouldstop, PID %d",
drivers/mtd/ubi/wl.c +1
-2- ubi->bgt_name, task_pid_nr(current)); +if (kthread_should_stop ())break ; - }Quoted 1 line
How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
"Yes, please, remove these messages or turn them into debugging messages. And yes, these should have been added in a separate patch."Quoted 3 lines
Artem.
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 2 lines
Thanks, //richard
Quoted 5 lines · Tanya Brokhman
Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging capabilities".
Signed-off-by Tanya Brokhman <[email protected]>---
Pushed this patch, but without the hunk which changes the printing helpers. Thanks!