| Message ID | 20250125064619.8305-22-jim.cromie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y | expand |
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. Hi Jim, It could be very nice to squash when possible yes! > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > 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;
On Tue, Feb 25, 2025 at 7:26 AM Louis Chauvet <louis.chauvet@bootlin.com> 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@gmail.com> > > Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com> > > > --- > > 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 >
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;
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. Signed-off-by: Jim Cromie <jim.cromie@gmail.com> --- lib/dynamic_debug.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-)