Messages in this thread Patch in this message |  | | Date | Sun, 02 Nov 2014 19:14:58 +0200 | From | Tanya Brokhman <> | Subject | Re: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities |
| |
On 10/24/2014 6:33 AM, hujianyang wrote: > Hi Tanya, > > When I was trying to push this patch to my product, I reviewed this patch > and found some small problems. I wish it's not too late to report these. > > The patch I get from linux-ubifs.git is amended a bit by Artem. I'd like to > quote your V5 patch for simplification. Some line numbers may mismatching. > >> @@ -1408,20 +1416,20 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp) >> return -EINVAL; >> >> if (mtd_devs == UBI_MAX_DEVICES) { >> - ubi_err("too many parameters, max. is %d\n", >> + pr_err("UBI error: too many parameters, max. is %d\n", >> UBI_MAX_DEVICES); >> return -EINVAL; >> } >> >> len = strnlen(val, MTD_PARAM_LEN_MAX); >> if (len == MTD_PARAM_LEN_MAX) { >> - ubi_err("parameter \"%s\" is too long, max. is %d\n", >> + pr_err("UBI error: parameter \"%s\" is too long, max. is %d\n", >> val, MTD_PARAM_LEN_MAX); >> return -EINVAL; >> } >> >> if (len == 0) { >> - pr_warn("UBI warning: empty 'mtd=' parameter - ignored\n"); >> + pr_err("UBI warning: empty 'mtd=' parameter - ignored\n"); >> return 0; >> } > > Why the last 'pr_warn()' need to be changed into 'pr_err()'? I looked up your > V1 and V2 patches, I think it's not your purpose.
It slipped somehow. Thanks! fixed.
> > > >> @@ -176,6 +176,7 @@ static int add_corrupted(struct ubi_attach_info *ai, int pnum, int ec) >> >> /** >> * validate_vid_hdr - check volume identifier header. >> + * @ubi: UBI device description object >> * @vid_hdr: the volume identifier header to check >> * @av: information about the volume this logical eraseblock belongs to >> * @pnum: physical eraseblock number the VID header came from > >> @@ -48,13 +48,14 @@ >> >> /** >> * 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. >> */ > >> @@ -660,13 +660,14 @@ 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_volume *vol, >> +static int check_av(const struct ubi_device *ubi, const struct ubi_volume *vol, >> const struct ubi_ainf_volume *av) >> { >> int err; > > This patch add 'struct ubi_device *' for 3 functions. We can get 'ubi_device' from > 'ubi_volume'. So I think it's because when we call these functions, the '->ubi' > pointer of 'ubi_volume' is not initialized, am I right? This patch use 'vol->ubi' > to indicate a 'struct ubi_device *' pointer in some places, I think you are sure > of using them. >
1. for validate_vid_hdr() we don;t have a ubi_volume yet since its part of the attach process so we need struct ubi_device 2. for get_exclusive() - you're right. Will fetch dev number from the volume 3. for check_av() - you're right. fixed
> > >> @@ -1010,28 +1015,28 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, >> ubi->bgt_thread = kthread_create(ubi_thread, ubi, "%s", ubi->bgt_name); >> if (IS_ERR(ubi->bgt_thread)) { >> err = PTR_ERR(ubi->bgt_thread); >> - ubi_err("cannot spawn \"%s\", error %d", ubi->bgt_name, >> - err); >> + ubi_err(ubi, "cannot spawn \"%s\", error %d", >> + ubi->bgt_name, err); >> goto out_debugfs; >> } >> >> - ubi_msg("attached mtd%d (name \"%s\", size %llu MiB) to ubi%d", >> - mtd->index, mtd->name, ubi->flash_size >> 20, ubi_num); >> - ubi_msg("PEB size: %d bytes (%d KiB), LEB size: %d bytes", >> + ubi_msg(ubi, "attached mtd%d (name \"%s\", size %llu MiB)", >> + mtd->index, mtd->name, ubi->flash_size >> 20); >> + ubi_msg(ubi, "PEB size: %d bytes (%d KiB), LEB size: %d bytes", >> ubi->peb_size, ubi->peb_size >> 10, ubi->leb_size); > > We have the parameter 'ubi_num' for log in some functions like 'ubi_attach_mtd_dev' > before. This patch remove 'ubi_num' in upper changes but keep it in other changes. > Do we have a discussed rule to deal with this situation? It's not a big problem~
I removed it because it made no sense printing it twice: "ubi-0: attached mtd-0 (...) to ubi0"? so I shortned the message: "ubi-0: attched mtd..." All the info is still there.... Same for other messages that printed ubi number.
> > > >> @@ -1798,15 +1803,18 @@ int ubi_thread(void *u) >> int failures = 0; >> struct ubi_device *ubi = u; >> >> - ubi_msg("background thread \"%s\" started, PID %d", >> + ubi_msg(ubi, "background thread \"%s\" started, PID %d", >> ubi->bgt_name, task_pid_nr(current)); >> >> set_freezable(); >> for (;;) { >> int err; >> >> - if (kthread_should_stop()) >> + if (kthread_should_stop()) { >> + ubi_msg(ubi, "background thread \"%s\" should stop, PID %d", >> + ubi->bgt_name, task_pid_nr(current)); >> break; >> + } >> >> if (try_to_freeze()) >> continue; > >> @@ -1798,15 +1803,18 @@ int ubi_thread(void *u) >> int failures = 0; >> struct ubi_device *ubi = u; >> >> - ubi_msg("background thread \"%s\" started, PID %d", >> + ubi_msg(ubi, "background thread \"%s\" started, PID %d", >> ubi->bgt_name, task_pid_nr(current)); >> >> set_freezable(); >> for (;;) { >> int err; >> >> - if (kthread_should_stop()) >> + if (kthread_should_stop()) { >> + ubi_msg(ubi, "background thread \"%s\" should stop, PID %d", >> + ubi->bgt_name, task_pid_nr(current)); >> break; >> + } >> >> if (try_to_freeze()) >> continue; > > Here are two new adding messages. Maybe a separate patch is better? Just a > suggestion.
Done.
> > >> @@ -1415,8 +1418,9 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len) >> return 0; >> >> fail: >> - ubi_err("self-check failed for PEB %d", pnum); >> - ubi_msg("hex dump of the %d-%d region", offset, offset + len); >> + ubi_err(ubi, "self-check failed for PEB %d", pnum); >> + 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: > > Artem, I know you have tried to align the message code in different lines, maybe > you can check if you lose this one. >
hmmm... not sure I understand what is wrong here....
> > Thanks~! > > Hu > > >
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
|  |