Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks

[jim . cromie]

> >
> > There is still plenty of bikeshedding to do.
>
> > ---
> > v4+:
> >
> > . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> > . in query, replace hardcoded "i915" w kp->mod->name
> > . static inline the stubs
> > . const *str in structs, const array. -Emil
> > . dyndbg: add do-nothing DEFINE_DYNAMIC_DEBUG_CATEGORIES if !DD_CORE
> > . call MOD_PARM_DESC(name, "$desc") for users
> > . simplify callback, remove bit-change detection
> > . config errs reported by <lkp@xxxxxxxxx>
> >
> > ddh-helpers
>
> > Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
>
> So, it is signed or not? I didn't get (perhaps due to misplaced changlog?).
>

It might be my --- snip, and reliance on format-patch -s

would it work if I used --  2char snip ?

> ...
>
> >  } __attribute__((aligned(8)));
> >
> >
>
> Do we need two blank lines here?
>
> > +struct kernel_param;
>
> ...
>
> > +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> > +{
> > +     unsigned long inbits;
> > +     int rc, i, chgct = 0, totct = 0;
> > +     char query[OUR_QUERY_SIZE];
> > +     struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
>
> So you need space after ')' ?
>
> > +     rc = kstrtoul(instr, 0, &inbits);
> > +     if (rc) {
> > +             pr_err("set_dyndbg: failed\n");
>
> > +             return -EINVAL;
>
> Why not to return rc?
>
> > +     }
> > +     vpr_info("set_dyndbg: input 0x%lx\n", inbits);
> > +
> > +     for (i = 0; !!bitmap[i].prefix; i++) {
>
> Hmm... Why not simply
>
>         for (bitmap = ...; bitmap->prefix; bitmap++) {
>
> ?
>
> > +
>
> Redundant blank line.
>
> > +             sprintf(query, "format '^%s' %cp", bitmap[i].prefix,
> > +                     test_bit(i, &inbits) ? '+' : '-');
>
> snprintf() ?
>
> > +
> > +             chgct = dynamic_debug_exec_queries(query, kp->mod->name);
> > +
> > +             v2pr_info("bit-%d: %d changes by '%s'\n", i, chgct, query);
> > +             totct += chgct;
> > +     }
> > +     vpr_info("total changes: %d\n", totct);
> > +     return 0;
> > +}
>
> ...
>
> > +     return scnprintf(buffer, PAGE_SIZE, "%u\n",
> > +                      *((unsigned int *)kp->arg));
>
> One line.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>