On Tue, Mar 25, 2025 at 12:29 PM <jim.cro...@gmail.com> wrote: > > On Mon, Mar 24, 2025 at 9:20 AM Louis Chauvet <louis.chau...@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-973799727...@akamai.com/ > https://lore.kernel.org/lkml/0d9f644f-3d60-02c3-7ce0-01296757e...@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
