On Fri, 2014-10-24 at 11:33 +0800, hujianyang wrote:
> > 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.
Well-spotted, thanks.
> > -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.
Yeah, let's remove the unneeded argument indeed.
> > - 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~
Well, printing 'ubi_num' explicitely is not needed anymore, so it would
be good to make the code consistent and remove it from other places,
where it is not needed.
> > - 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.
Yes, please, remove these messages or turn them into debugging messages.
And yes, these should have been added in a separate patch.
> > @@ -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.
Yeah, lets' correct this too.
Thanks for ferview Hujianyang!
Tanya, would you send a follow-up patch these?
Artem.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
