On Tue, Feb 25, 2025 at 7:26 AM Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:
>
>
>
> Le 25/01/2025 à 07:45, Jim Cromie a écrit :
> > To prep for failing modprobe on classid conflicts, upgrade the
> > call-chain around ddebug_add_module(), in 2 ways:
> >
> > 1. in ddebug_add_module() add local reserved_ids to accumulate
> > reservations, pass it by ref to ddebug_attach_{,user_}module_classes()
> > so they can examine the reservations as they work.
> >
> > 2. return int from both ddebug_attach_{,user_}module_classes(), up to
> > ddebug_add_module(), then to ddebug_module_notify().
> >
> > No conflicts are currently detected or returned.
> >
> > TBD: This is updated further by hoisting the reservation-check, which
> > obsoletes part of 2, creating churn, maybe squash it away.
>
this now done locally as
05b0eed12dcc dyndbg: hoist classmap-filter-by-modname up to ddebug_add_module
sha will change yet..
Though Ive dropped the fail-on-modprobe,
I didn't want to open up another failure mode
Some WARN or pr_error should suffice for now.
> Hi Jim,
>
> It could be very nice to squash when possible yes!
>
> > Signed-off-by: Jim Cromie <jim.cromie@xxxxxxxxx>
>
> Reviewed-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
>
> > ---
> > lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 16c9b752822b..0ef243e30663 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -1216,8 +1216,9 @@ static void ddebug_apply_params(const struct ddebug_class_map *cm, const char *m
> > * modular classmap vector/section. Save the start and length of the
> > * subrange at its edges.
> > */
> > -static void ddebug_attach_module_classes(struct ddebug_table *dt,
> > - const struct _ddebug_info *di)
> > +static int ddebug_attach_module_classes(struct ddebug_table *dt,
> > + const struct _ddebug_info *di,
> > + u64 *reserved_ids)
> > {
> > struct ddebug_class_map *cm;
> > int i, nc = 0;
> > @@ -1230,13 +1231,14 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
> > }
> > }
> > if (!nc)
> > - return;
> > + return 0;
> >
> > vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
> > dt->info.maps.len = nc;
> >
> > for_subvec(i, cm, &dt->info, maps)
> > ddebug_apply_params(cm, cm->mod_name);
> > + return 0;
> > }
> >
> > /*
> > @@ -1244,8 +1246,9 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
> > * means a query against the dt/module, which means it must be on the
> > * list to be seen by ddebug_change.
> > */
> > -static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
> > - const struct _ddebug_info *di)
> > +static int ddebug_attach_user_module_classes(struct ddebug_table *dt,
> > + const struct _ddebug_info *di,
> > + u64 *reserved_ids)
> > {
> > struct ddebug_class_user *cli;
> > int i, nc = 0;
> > @@ -1266,7 +1269,7 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
> > }
> > }
> > if (!nc)
> > - return;
> > + return 0;
> >
> > dt->info.users.len = nc;
> >
> > @@ -1275,6 +1278,7 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
> > ddebug_apply_params(cli->map, cli->mod_name);
> >
> > vpr_dt_info(dt, "attach-client-module: ");
> > + return 0;
> > }
> >
> > /*
> > @@ -1284,6 +1288,8 @@ static void ddebug_attach_user_module_classes(struct ddebug_table *dt,
> > static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
> > {
> > struct ddebug_table *dt;
> > + u64 reserved_ids = 0;
> > + int rc;
> >
> > if (!di->descs.len)
> > return 0;
> > @@ -1306,16 +1312,23 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
> >
> > INIT_LIST_HEAD(&dt->link);
> >
> > - if (di->maps.len)
> > - ddebug_attach_module_classes(dt, di);
> > -
> > + if (di->maps.len) {
> > + rc = ddebug_attach_module_classes(dt, di, &reserved_ids);
> > + if (rc) {
> > + kfree(dt);
> > + return rc;
> > + }
> > + }
> > mutex_lock(&ddebug_lock);
> > list_add_tail(&dt->link, &ddebug_tables);
> > mutex_unlock(&ddebug_lock);
> >
> > - if (di->users.len)
> > - ddebug_attach_user_module_classes(dt, di);
> >
> > + if (di->users.len) {
> > + rc = ddebug_attach_user_module_classes(dt, di, &reserved_ids);
> > + if (rc)
> > + return rc;
> > + }
> > vpr_info("%3u debug prints in module %s\n", di->descs.len, modname);
> > return 0;
> > }
> > @@ -1400,6 +1413,11 @@ static int ddebug_module_notify(struct notifier_block *self, unsigned long val,
> > switch (val) {
> > case MODULE_STATE_COMING:
> > ret = ddebug_add_module(&mod->dyndbg_info, mod->name);
> > + if (ret == -EINVAL) {
> > + pr_err("conflicting dyndbg-classmap reservations\n");
> > + ddebug_remove_module(mod->name);
> > + break;
> > + }
> > if (ret)
> > WARN(1, "Failed to allocate memory: dyndbg may not work properly.\n");
> > break;
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
