Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210813151734.1236324-1-jim.cromie@gmail.com>
 <20210813151734.1236324-4-jim.cromie@gmail.com> <YRaU6fbGjcV7BGC/@smile.fi.intel.com>
In-Reply-To: <YRaU6fbGjcV7BGC/@smile.fi.intel.com>
From:   jim.cromie@gmail.com
Date:   Fri, 13 Aug 2021 11:49:57 -0600
Message-ID: <CAJfuBxzkWuK-Xh-2gynmLeHx+gb6bwhbKTG02pydYvQzNBiPjA@mail.gmail.com>
Subject: Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> >
> > 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@intel.com>
> >
> > ddh-helpers
>
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>
> 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
>
>