Re: [PATCH v2 30/59] dyndbg: drop "protection" of class'd pr_debugs from legacy queries

[jim.cromie@gmail.com]

On Tue, Mar 25, 2025 at 12:29 PM <jim.cromie@gmail.com> wrote:
>
> On Mon, Mar 24, 2025 at 9:20 AM Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >
> >
> >
> > Le 20/03/2025 à 19:52, Jim Cromie a écrit :
> > > Current classmap code protects class'd pr_debugs from unintended
> > > changes by "legacy" unclassed queries:
> > >
> > >    # this doesn't disable all of DRM_UT_* categories
> > >    echo "-p" > /proc/dynamic_debug/control
> > >
> > >    # name the class to change it - protective but tedious
> > >    echo "class DRM_UT_CORE +p" > /proc/dynamic_debug/control
> > >
> > >    # or do it the subsystem way
> > >    echo 1 > /sys/module/drm/parameters/debug
> > >
> > > This "name the class to change it" behavior gave a modicum of
> > > protection to classmap users (ie DRM) so their debug settings aren't
> > > trivially and unintentionally altered underneath them.
> > >
> > > But this made the class keyword special in some sense; the other
> > > keywords skip only on explicit mismatch, otherwize the code falls thru
> >
> > s/otherwize/otherwise/
>
> ack
>
> >
> > > to adjust the pr-debug site.
> > >
> > > So Jason Baron didn't like this special case when I 1st proposed it;
> > > I argued 2 points:
> > > - "protection gives stable-debug, improving utility"
> > > - __drm_debug is authoritative w/o dyndbg under it.
> > >
> > > I thought I'd convinced him back then, (and the patchset got merged),
> > > but he noted it again when he reviewed this series.  So this commit
> > > names the "special case": ddebug_client_module_protects_classes(), and
> > > reverts it to Jason's preference.
> >   >
> > > If a class mismatch is seen, code distinguishes whether the class was
> > > explicitly given (and always skips/continue), or the DFLT was assumed
> > > because no class was given.  Here we test
> > > ddebug_client_module_protects_classes(), skip if so.
> > >
> > > Later, if any user/module wants to protect its classes, we could add a
> > > flag to ddebug_table, a means to set it from CLASSMAP_DEFINE, and
> > > check it when applying a classless query/cmd.
> >
> > I don't really understand the goal of the protection, do you have the
> > discussion between you and Jason so I can have some context and some
> > answer to my questions?
> >
>
> The on-list discussion is here.
>
> https://lore.kernel.org/lkml/2d3846cb-ff9a-3484-61a8-973799727d8f@akamai.com/
> https://lore.kernel.org/lkml/0d9f644f-3d60-02c3-7ce0-01296757e181@akamai.com/#t
>
> At the time I thought it was unfinished business, and expected some
> more discussion,
> but that didnt happen, and later GregKH committed the set.
>
> Last summer I emailed him privately, and he made a 5-6 points Ive
> addressed in this rev,
> (reduction of repetitive code, enforcing classmap constraints,
> protecting against misuse)
> but it also became clear he still didnt like the "specialness" of the keyword,
> given by the _DFLT constraint applied to legacy callsites and queries.
>
> Since thats a bit of a philosophical debate, I looked for a technical solution,
> to have it either way with fairly trivial additions, and to yield
> until user experience
> dictates otherwise
>
> To be clear, I still think protecting the "classed" is proper.
> Without dyndbg, /sys/module/drm/parameters/debug is authoritative, full stop.
> I'm surprised any customer would give away that certainty,
> it looks like a (small caliber) footgun to me.
> But its not worth disagreeing on.
> Hence this patch reverts that "protection"
>
> > With the example you gave above, I think this could lead to a very odd
> > behavior: if I enable dyndbg, I expect any pr_dbg to be managed by
> > dyndbg settings.
>
> if by "any" you mean ALL the ones that currently exist,
> before we add a whole new "CLASS" of user,
> (with ~5k uses, all comfortable with their exclusive control)
> I can agree.
>
> echo class FOO +p > control
> gives full control.  You just have to say so for the new classes of users.
>
> >
> > If a user writes stuff on dyndbg control, he clearly knows what he is
> > doing, and he wants to control what logs he wants.
> >
> > And if you allow multiple "protected" users, the normal way to disable
> > all dyndbg logs will be:
> >
> >         ddcmd -p
> >         ddcmd class DRM_UT_CORE -p
> >         ddcmd class DRM_... -p # all drm classes
> >         ddcmd class SPI_... -p # all spi classes
> >         ddcmd class WHATEVER_... -p # all other subsystem
> >
> >         # And only now you can enable only what you want
> >         ddcmd module my_mod +p
> >
> > This is clearly annoying to write.
>
> It is clearly annoying.
> It doesnt need to be handy.
> thats what /sys/module/drm/parameters/debug is for.
> with modest "protection" of explicit naming,
> the sysfs knob can reasonably be expected
> to reflect whats going on underneath.
> Without it, bets are misplaced.
>

Heres an improvement:

a use of CLASSMAP_PARAM means user wants a sysfs knob.
We can reasonably conclude they prefer that mode of control
(its what DRM users would expect, since a long time ago).

In that case, protect the PARAM settings
(from unqualified settings, use of class FOO still works)
otherwise no protection.
simple to explain, no extra knobs.



> >
> > If DRM (or whatever subsystem) wants to add a debug parameter and use it
> > to control their logs without being impacted by dyndbg, I believe it
> > should not use dyndbg classes to do it.
>
> hmm - dyndbg's 1st value is its NOOP cost when off.
> If thats not worth something, you wouldnt bother using it.
>
>
> In any case, its pretty clear that my viewpoint isnt prevailing here,
> and as I said, I dont care enough to disagree.
> the reversion here can stand.
>
>

apologies, since Im sounding kinda argumentative.
Jim