Received-SPF: pass (google.com: domain of linux-kernel+bounces-186559-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240516174357.26755-1-jim.cromie@gmail.com> <CALwA+NZxiapXe-BBY6CvpqT-Z1h_YyMQr=JBwybY55TWtDNiBA@mail.gmail.com>
 <CAJfuBxw3oDbdAwporJw0DXGRbpLqVXxGaBQmOra4vMGDzH94jA@mail.gmail.com>
In-Reply-To: <CAJfuBxw3oDbdAwporJw0DXGRbpLqVXxGaBQmOra4vMGDzH94jA@mail.gmail.com>
From: =?UTF-8?Q?=C5=81ukasz_Bartosik?= <ukaszb@chromium.org>
Date: Wed, 22 May 2024 19:36:15 +0200
Message-ID: <CALwA+Nbr15Wd_kkMr3bJCON5zhM+JAuhxwgFG654yd+nemzzTw@mail.gmail.com>
Subject: Re: [PATCH v8-RESEND 00/33] Fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression
To: jim.cromie@gmail.com
Cc: jbaron@akamai.com, gregkh@linuxfoundation.org, 
	linux-kernel@vger.kernel.org, linux@rasmusvillemoes.dk, joe@perches.com, 
	mcgrof@kernel.org, daniel.vetter@ffwll.ch, tvrtko.ursulin@linux.intel.com, 
	jani.nikula@intel.com, ville.syrjala@linux.intel.com, seanpaul@chromium.org, 
	robdclark@gmail.com, groeck@google.com, yanivt@google.com, bleung@google.com, 
	Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 21, 2024 at 9:11=E2=80=AFPM <jim.cromie@gmail.com> wrote:
>
> On Tue, May 21, 2024 at 5:41=E2=80=AFAM =C5=81ukasz Bartosik <ukaszb@chro=
mium.org> wrote:
> >
> > On Thu, May 16, 2024 at 7:44=E2=80=AFPM Jim Cromie <jim.cromie@gmail.co=
m> wrote:
> > >
> > > hi Greg, Jason,
> > >
> > > This patchset fixes the CONFIG_DRM_USE_DYNAMIC_DEBUG=3Dy regression,
> > > Fixes: bb2ff6c27bc9 ("drm: Disable dynamic debug as broken")
> > >
> > > Im calling it v8, to keep consistent with previous labels.
> > > v6 was what got committed, back in 9/2022.
> > > v7 had at least 2 problems that blocked its submission:
> > >
> > >  https://lore.kernel.org/lkml/20231101002609.3533731-1-jim.cromie@gma=
il.com/
> > >  https://patchwork.freedesktop.org/series/125066/
> > >
> > > 1. missing __align(8) in METATDATA macro, giving too much placement
> > > freedom to linker, caused weird segvs following non-ptr vals, but for
> > > builtin modules only. found by lkp-test.
> > >
> > > 2. the main patch touched 2 subsystems at once, which would require
> > > special handling.
> > >
> > > What was broken about DYNAMIC_DEBUG ?
> > >
> > > Booting a modular kernel with drm.debug=3D0x1ff enabled pr_debugs onl=
y
> > > in drm itself, not in the yet-to-be loaded driver + helpers.  Once
> > > loaded, the driver's pr_debugs are properly enabled by:
> > >
> > >    echo 0x1ff > /sys/module/drm/parameters/debug  # still worked
> > >
> > > I had tested with scripts doing lots of modprobes with various
> > > permutations of dyndbg=3D<> option, and I missed that I didn't test
> > > without them.
> > >
> > > The deeper cause was my design error, a violation of the K&R rule:
> > > "define once, refer many times".
> > >
> > > DECLARE_DYNDBG_CLASSMAP defined the classmap, and was used everywhere=
,
> > > re-declaring the same static classmap repeatedly. Jani Nikula actuall=
y
> > > picked up on this (iirc shortly after committed), but the problem
> > > hadn't been seen yet in CI.  One patchset across 2 subsystems didn't
> > > help either.
> > >
> >
> > Could  you summarize classmaps from the beginning ?
> >
>
> IMO, the best thing about dynamic-debug is that it uses
> JUMP_LABELs / static-keys , so it is NOPs except when you want them.
>
> Classmaps 1st purpose is to bring that to DRM,
> which has 10 mutually exclusive categories of debug output,
> all controlled from bits in /sys/modules/drm/parameters/debug.
> w/o dyndbg, __drm_debug is constantly checked during normal ops.
>
> verbose/relative is added for the other kind of debug levels
>
> classes =3D=3D=3D categories, with name change to avoid muddling
> the 2 sides of the drm <=3D> dyndbg relation
>
> classmaps to contain the classes, and to not just take the "class" name,
> which is way too generic a term.
>
> Since dyndbg's sole input is the >control file, the "class" keyword was a=
dded.
> the PARAM support constructs queries using class foo to effect its change=
s.
>
>    # change classes by name
>    echo class DRM_UT_CORE +p > /proc/dynamic_debug/control
>    # this cant touch them
>    echo  +p > /proc/dynamic_debug/control
>
> this syntax extension allows dyndbg to honor DRM's privacy expectations
> DRM owns its categories via sysfs knob (only)
> and should not tolerate things changing out from under it.
> (your bank account isnt useful if everyone can withdraw from it)
>
> Also, the class'd pr-dbgs are only manipulable by their classname,
> not by their corresponding class_id : a 64 value space, which is
> shared across all classmaps in a module.
> IOW a module could have 8 separate classmaps, each with 8 classes.
>
> >
> > > So the revised classmap API "splits" it to def & ref:
> > >
> > > DYNDBG_CLASSMAP_DEFINE fixes & updates the busted macro, EXPORTing th=
e
> > > classmap instead.  It gets invoked once per subsystem, by the
> > > parent/builtin, drm.ko for DRM.
> > >
> > > DYNDBG_CLASSMAP_USE in drivers and helpers refer to the classmap by
> > > name, which links the 2 modules, (like a driver's dependency on exter=
n
> > > __drm_debug).
>
> Summarizing,
> author(s) _DEFINE their categories / classes,
> other author(s) _USE that definition.
>
> Both these tell dyndbg that it is allowed to manipulate those classnames
> in the invoking module.
>
> Does that fill the gaps in the explanation / motivation ?
>

Yes it does. Thank you for elaborating on the topic.

> thanks
> ~jimc