Re: [PATCH v3 27/54] dyndbg: drop "protection" of class'd pr_debugs from legacy queries

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 02/04/2025 à 19:41, 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 (old school) 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.
> 
> And by "symmetry", if they're not picked by "class FOO", then they're
> excluded from adjustment.  This allowed all previously conceived
> queries to work the way they always had; ie select the same set of
> pr_debugs, despite the inclusion of whole new classes of pr_debugs.
> 
> That had 2 downsides:
> 
> 1. "name the class to change it" means that every class must be
> individually modified, quickly becoming long-winded and tedious to
> adjust all the classes in a map via >control.
> 
> 2. It made the class keyword special in some sense; the other keywords
> skip only on explicit mismatch, otherwise the code falls thru to
> adjust the pr-debug site.
> 
> So this patch reverts to the traditional view, it drops protection of
> classes from default/legacy queries.
> 
> But it also refactors the skip/continue choice to allow the module
> defining the classmap to protect its classes from unintended
> alterations by legacy/class-less queries.
> 
> Next:
> 
> Author choice: use of DYNAMIC_DEBUG_CLASSMAP_PARAM() means they want
> the drm.debug style control point.  We should presume they want it to
> reflect whats set underneath, with only "class FOO" qualified queries
> changing the callsites beneath.

This explanation is way better, thanks a lot! It is way easier to 
understand this protection.

I continue to think that this "protect its classes from unintended 
alterations" is not a good user api. But it is not a blocking point for 
me, so I would like to get the point of view from other people (mainly 
from DRM, they are the first users).

With or without this modification:

Acked-by: Louis Chauvet <louis.chauvet@bootlin.com>

> CC: jbaron@akamai.com
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> --
> v3- s/slctd_/selected_/
>      pitch the PARAM control of protection.
> ---
>   lib/dynamic_debug.c | 31 +++++++++++++++++++++++--------
>   1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 7a60088a1b5c..54f462cf41b0 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -197,6 +197,17 @@ ddebug_find_valid_class(struct _ddebug_info const *di, const char *query_class,
>   	return NULL;
>   }
>   
> +/*
> + * classmaps-v1 protected classes from changes by legacy commands
> + * (those selecting _DPRINTK_CLASS_DFLT by omission), v2 undoes that
> + * special treatment.  State so explicitly.  Later we could give
> + * modules the choice to protect their classes or to keep v2 behavior.
> + */
> +static inline bool ddebug_client_module_protects_classes(const struct ddebug_table *dt)
> +{
> +	return false;
> +}
> +
>   /*
>    * Search the tables for _ddebug's which match the given `query' and
>    * apply the `flags' and `mask' to them.  Returns number of matching
> @@ -211,7 +222,7 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
>   	unsigned int nfound = 0;
>   	struct flagsbuf fbuf, nbuf;
>   	struct _ddebug_class_map *map = NULL;
> -	int valid_class;
> +	int selected_class;
>   
>   	/* search for matching ddebugs */
>   	mutex_lock(&ddebug_lock);
> @@ -224,21 +235,25 @@ static int ddebug_change(const struct ddebug_query *query, struct flag_settings
>   
>   		if (query->class_string) {
>   			map = ddebug_find_valid_class(&dt->info, query->class_string,
> -						      &valid_class);
> +						      &selected_class);
>   			if (!map)
>   				continue;
>   		} else {
> -			/* constrain query, do not touch class'd callsites */
> -			valid_class = _DPRINTK_CLASS_DFLT;
> +			selected_class = _DPRINTK_CLASS_DFLT;
>   		}
>   
>   		for (i = 0; i < dt->info.descs.len; i++) {
>   			struct _ddebug *dp = &dt->info.descs.start[i];
>   
> -			/* match site against query-class */
> -			if (dp->class_id != valid_class)
> -				continue;
> -
> +			if (dp->class_id != selected_class) {
> +				if (query->class_string)
> +					/* site.class != given class */
> +					continue;
> +				/* legacy query, class'd site */
> +				else if (ddebug_client_module_protects_classes(dt))
> +					continue;
> +				/* allow change on class'd pr_debug */
> +			}
>   			/* match against the source filename */
>   			if (query->filename &&
>   			    !match_wildcard(query->filename, dp->filename) &&

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com