lkml.org 
[lkml]   [2014]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V5] mtd: ubi: Extend UBI layer debug/messaging capabilities
From
Date
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.



\
 
 \ /
  Last update: 2014-10-30 10:01    [W:0.163 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site