| Message ID | 1415531185-2343-1-git-send-email-tlinder@codeaurora.org |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: > > /* 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__) 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? > > - 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. > - 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.
On 11/10/2014 2:18 PM, Artem Bityutskiy wrote: > On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: >> >> /* 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__) > > 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. > >> >> - 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. 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. > > >> - 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." > > Artem. > Thanks, Tanya Brokhman
On Mon, 2014-11-10 at 14:53 +0200, Tanya Brokhman wrote: > On 11/10/2014 2:18 PM, Artem Bityutskiy wrote: > > On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: > >> > >> /* 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__) > > > > 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. 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 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. > > 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.
On Mon, Nov 10, 2014 at 1:53 PM, Tanya Brokhman <tlinder@codeaurora.org> wrote: > On 11/10/2014 2:18 PM, Artem Bityutskiy wrote: >> >> On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: >>> >>> >>> /* 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__) >> >> >> 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. > >> >>> >>> - 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. > > > 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". >> >> >>> - 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." > >> >> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Mon, 2014-11-10 at 15:14 +0200, Artem Bityutskiy wrote: > On Mon, 2014-11-10 at 14:53 +0200, Tanya Brokhman wrote: > > On 11/10/2014 2:18 PM, Artem Bityutskiy wrote: > > > On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: > > >> > > >> /* 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__) > > > > > > 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. > > 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); }
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote: > Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging > capabilities". > > Signed-off-by: Tanya Brokhman <tlinder@codeaurora.org> > --- Pushed this patch, but without the hunk which changes the printing helpers. Thanks!
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 3405be4..ba01a8d 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -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; } diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c index 3410ea81..bbef168 100644 --- a/drivers/mtd/ubi/cdev.c +++ b/drivers/mtd/ubi/cdev.c @@ -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; diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index 396aaa5..ed0bcb3 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -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: diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index f80ffab..88c2e9f 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -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" diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c index f8fc308..68c9c5ea 100644 --- a/drivers/mtd/ubi/vtbl.c +++ b/drivers/mtd/ubi/vtbl.c @@ -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; } diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 834f6fe..8f7bde6 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -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;
Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging capabilities". Signed-off-by: Tanya Brokhman <tlinder@codeaurora.org> --- 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(-)