On Sun, Mar 16, 2025 at 3:14 PM <jim.cro...@gmail.com> wrote:
>
> On Tue, Feb 25, 2025 at 7:29 AM Louis Chauvet <louis.chau...@bootlin.com>
> wrote:
> >
> >
> >
> > Le 25/01/2025 à 07:45, Jim Cromie a écrit :
> > > move the DYNDBG_CLASSMAP_PARAM macro from test-dynamic-debug.c into
> > > the header, and refine it, by distinguishing the 2 use cases:
> > >
> > > 1.DYNDBG_CLASSMAP_PARAM_REF
> > > for DRM, to pass in extern __drm_debug by name.
> > > dyndbg keeps bits in it, so drm can still use it as before
> > >
> > > 2.DYNDBG_CLASSMAP_PARAM
> > > new user (test_dynamic_debug) doesn't need to share state,
> > > decls a static long unsigned int to store the bitvec.
> > >
> > > __DYNDBG_CLASSMAP_PARAM
> > > bottom layer - allocate,init a ddebug-class-param, module-param-cb.
> > >
> > > Modify ddebug_sync_classbits() argtype deref inside the fn, to give
> > > access to all kp members.
> > >
> > > Also clean up and improve comments in test-code, and add
> > > MODULE_DESCRIPTIONs.
> > >
> > > Signed-off-by: Jim Cromie <jim.cro...@gmail.com>
> > > ---
> > >
> > > -v9
> > > - fixup drm-print.h add PARAM_REF forwarding macros
> > > with DYNDBG_CLASSMAP_PARAM_REF in the API, add DRM_ variant
> > > ---
> > > include/linux/dynamic_debug.h | 38 +++++++++++++++++++++
> > > lib/dynamic_debug.c | 60 ++++++++++++++++++++++-----------
> > > lib/test_dynamic_debug.c | 59 +++++++++++++-------------------
> > > lib/test_dynamic_debug_submod.c | 9 ++++-
> > > 4 files changed, 111 insertions(+), 55 deletions(-)
> > >
> > > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> > > index 48d76a273f68..b47d1088b7ad 100644
> > > --- a/include/linux/dynamic_debug.h
> > > +++ b/include/linux/dynamic_debug.h
> > > @@ -205,6 +205,44 @@ struct ddebug_class_param {
> > > const struct ddebug_class_map *map;
> > > };
> > >
> > > +/**
> > > + * DYNDBG_CLASSMAP_PARAM - control a ddebug-classmap from a sys-param
> > > + * @_name: sysfs node name
> > > + * @_var: name of the classmap var defining the controlled classes/bits
> > > + * @_flags: flags to be toggled, typically just 'p'
> > > + *
> > > + * Creates a sysfs-param to control the classes defined by the
> > > + * exported classmap, with bits 0..N-1 mapped to the classes named.
> > > + * This version keeps class-state in a private long int.
> > > + */
> > > +#define DYNDBG_CLASSMAP_PARAM(_name, _var, _flags) \
> > > + static unsigned long _name##_bvec; \
> > > + __DYNDBG_CLASSMAP_PARAM(_name, _name##_bvec, _var, _flags)
> > > +
> > > +/**
> > > + * DYNDBG_CLASSMAP_PARAM_REF - wrap a classmap with a controlling
> > > sys-param
> > > + * @_name: sysfs node name
> > > + * @_bits: name of the module's unsigned long bit-vector, ex:
> > > __drm_debug
> > > + * @_var: name of the (exported) classmap var defining the classes/bits
> > > + * @_flags: flags to be toggled, typically just 'p'
> > > + *
> > > + * Creates a sysfs-param to control the classes defined by the
> > > + * exported clasmap, with bits 0..N-1 mapped to the classes named.
> > > + * This version keeps class-state in user @_bits. This lets drm check
> > > + * __drm_debug elsewhere too.
> > > + */
> > > +#define DYNDBG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags)
> > > \
> > > + __DYNDBG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
> > > +
> > > +#define __DYNDBG_CLASSMAP_PARAM(_name, _bits, _var, _flags) \
> > > + static struct ddebug_class_param _name##_##_flags = { \
> > > + .bits = &(_bits), \
> > > + .flags = #_flags, \
> > > + .map = &(_var), \
> > > + }; \
> > > + module_param_cb(_name, ¶m_ops_dyndbg_classes, \
> > > + &_name##_##_flags, 0600)
> > > +
> > > /*
> > > * pr_debug() and friends are globally enabled or modules have
> > > selectively
> > > * enabled them.
> > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > > index 781781835094..9283f2866415 100644
> > > --- a/lib/dynamic_debug.c
> > > +++ b/lib/dynamic_debug.c
> > > @@ -660,6 +660,30 @@ static int ddebug_apply_class_bitmap(const struct
> > > ddebug_class_param *dcp,
> > >
> > > #define CLASSMAP_BITMASK(width) ((1UL << (width)) - 1)
> > >
> > > +static void ddebug_class_param_clamp_input(unsigned long *inrep, const
> > > struct kernel_param *kp)
> > > +{
> > > + const struct ddebug_class_param *dcp = kp->arg;
> > > + const struct ddebug_class_map *map = dcp->map;
> > > +
> > > + switch (map->map_type) {
> > > + case DD_CLASS_TYPE_DISJOINT_BITS:
> > > + /* expect bits. mask and warn if too many */
> > > + if (*inrep & ~CLASSMAP_BITMASK(map->length)) {
> > > + pr_warn("%s: input: 0x%lx exceeds mask: 0x%lx,
> > > masking\n",
> > > + KP_NAME(kp), *inrep,
> > > CLASSMAP_BITMASK(map->length));
> > > + *inrep &= CLASSMAP_BITMASK(map->length);
> > > + }
> > > + break;
> > > + case DD_CLASS_TYPE_LEVEL_NUM:
> > > + /* input is bitpos, of highest verbosity to be enabled */
> > > + if (*inrep > map->length) {
> > > + pr_warn("%s: level:%ld exceeds max:%d, clamping\n",
> > > + KP_NAME(kp), *inrep, map->length);
> > > + *inrep = map->length;
> > > + }
> > > + break;
> > > + }
> > > +}
> > > static int param_set_dyndbg_module_classes(const char *instr,
> > > const struct kernel_param *kp,
> > > const char *modnm)
> > > @@ -678,26 +702,15 @@ static int param_set_dyndbg_module_classes(const
> > > char *instr,
> > > pr_err("expecting numeric input, not: %s > %s\n", instr,
> > > KP_NAME(kp));
> > > return -EINVAL;
> > > }
> > > + ddebug_class_param_clamp_input(&inrep, kp);
> > >
> > > switch (map->map_type) {
> > > case DD_CLASS_TYPE_DISJOINT_BITS:
> > > - /* expect bits. mask and warn if too many */
> > > - if (inrep & ~CLASSMAP_BITMASK(map->length)) {
> > > - pr_warn("%s: input: 0x%lx exceeds mask: 0x%lx,
> > > masking\n",
> > > - KP_NAME(kp), inrep,
> > > CLASSMAP_BITMASK(map->length));
> > > - inrep &= CLASSMAP_BITMASK(map->length);
> > > - }
> > > v2pr_info("bits:0x%lx > %s.%s\n", inrep, modnm ?: "*",
> > > KP_NAME(kp));
> > > totct += ddebug_apply_class_bitmap(dcp, &inrep, *dcp->bits,
> > > modnm);
> > > *dcp->bits = inrep;
> > > break;
> > > case DD_CLASS_TYPE_LEVEL_NUM:
> > > - /* input is bitpos, of highest verbosity to be enabled */
> > > - if (inrep > map->length) {
> > > - pr_warn("%s: level:%ld exceeds max:%d, clamping\n",
> > > - KP_NAME(kp), inrep, map->length);
> > > - inrep = map->length;
> > > - }
> > > old_bits = CLASSMAP_BITMASK(*dcp->lvl);
> > > new_bits = CLASSMAP_BITMASK(inrep);
> > > v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits,
> > > KP_NAME(kp));
> > > @@ -1163,15 +1176,24 @@ static const struct proc_ops proc_fops = {
> > > static void ddebug_sync_classbits(const struct kernel_param *kp, const
> > > char *modname)
> > > {
> > > const struct ddebug_class_param *dcp = kp->arg;
> > > + unsigned long new_bits;
> > >
> > > - /* clamp initial bitvec, mask off hi-bits */
> > > - if (*dcp->bits & ~CLASSMAP_BITMASK(dcp->map->length)) {
> > > - *dcp->bits &= CLASSMAP_BITMASK(dcp->map->length);
> > > - v2pr_info("preset classbits: %lx\n", *dcp->bits);
> > > + ddebug_class_param_clamp_input(dcp->bits, kp);
> > > +
> > > + switch (dcp->map->map_type) {
> > > + case DD_CLASS_TYPE_DISJOINT_BITS:
> > > + v2pr_info(" %s: classbits: 0x%lx\n", KP_NAME(kp),
> > > *dcp->bits);
> > > + ddebug_apply_class_bitmap(dcp, dcp->bits, 0UL, modname);
> > > + break;
> > > + case DD_CLASS_TYPE_LEVEL_NUM:
> > > + new_bits = CLASSMAP_BITMASK(*dcp->lvl);
> > > + v2pr_info(" %s: lvl:%ld bits:0x%lx\n", KP_NAME(kp),
> > > *dcp->lvl, new_bits);
> > > + ddebug_apply_class_bitmap(dcp, &new_bits, 0UL, modname);
> > > + break;
> > > + default:
> > > + pr_err("bad map type %d\n", dcp->map->map_type);
> > > + return;
> > > }
> > > - /* force class'd prdbgs (in USEr module) to match (DEFINEr module)
> > > class-param */
> > > - ddebug_apply_class_bitmap(dcp, dcp->bits, ~0, modname);
> > > - ddebug_apply_class_bitmap(dcp, dcp->bits, 0, modname);
> >
> > Hi Jim,
> >
> > We lost the double call with ~0/0, is it normal?
>
> Good catch,
>
> I thought so, since it guarantees the pr_debugs' state to
> comport with sysfs settings on modprobe.
>
> I will review.
>
Ok, Im pretty sure I put those in to override DEBUG settings,
ie reset the default print state to whatever the controlling sysfs node,
if it was declared with CLASSMAP_PARAM*, has the var set at.
It now seems like extra work to clean up a corner case
which they asked for, by defining DEBUG
