On Mon, Mar 24, 2025 at 9:08 AM Louis Chauvet <louis.chau...@bootlin.com> wrote:
>
>
>
> Le 20/03/2025 à 19:51, Jim Cromie a écrit :
> Thanks for your explanation of __outvar! It makes sense. I never seen
> this pattern anywhere in the kernel, maybe a simple doc comment is
> enough to carry the information:
Im gonna pull it - Ive decided its misleading,
since it looks like it does something that it cant do.
a plain old comment would be enough, if its needed at all.
__outvar was at least brief ;-)
In fact I did, but in a later patch. cleaning now.
> /**
> * ddebug_find_valid_class - Find a valid class for a
> * given string
> * @dt: debug table to inspect
> * @class_string: string to match on
> * @class_id: output pointer for the class_id value
> *
> * Returns: Pointer to the ddebug_class_map instance, if found.
> * @class_id will be set to the id of this class. If no class
> * matching @class_string is found, returns NULL and class_id is
> * set to -ENOENT.
> */
>
> Or at maybe change the documentation of __outvar to be a bit more
> explicit about how it works (I had difficulties to understand because a
> lot of "magic macro" exist in the kernel and sometimes carry information
> to the compiler, but this one is always empty):
>
> /**
> * __outvar - Denotes that an argument is used as an output
> * value
> * This macro does nothing apart clarify for the reader that
> * the parameter will be filled by the callee
> */
>
> With any of the modifications (feel free to reword them as you want):
>
> Reviewed-by: Louis Chauvet <louis.chau...@bootlin.com>
>
> > {
> > struct ddebug_class_map *map;
> > - int idx;
> > + int i, idx;
> >
> > - list_for_each_entry(map, &dt->maps, link) {
> > + for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
> > idx = match_string(map->class_names, map->length,
> > class_string);
> > if (idx >= 0) {
> > *class_id = idx + map->base;
> > @@ -164,7 +167,6 @@ static struct ddebug_class_map
> > *ddebug_find_valid_class(struct ddebug_table cons
> > return NULL;
> > }
> >
> > -#define __outvar /* filled by callee */
> > /*
> > * Search the tables for _ddebug's which match the given `query' and
> > * apply the `flags' and `mask' to them. Returns number of matching
> > @@ -1122,9 +1124,10 @@ static void *ddebug_proc_next(struct seq_file *m,
> > void *p, loff_t *pos)
> >
> > static const char *ddebug_class_name(struct ddebug_iter *iter, struct
> > _ddebug *dp)
> > {
> > - struct ddebug_class_map *map;
> > + struct ddebug_class_map *map = iter->table->classes;
> > + int i, nc = iter->table->num_classes;
> >
> > - list_for_each_entry(map, &iter->table->maps, link)
> > + for (i = 0; i < nc; i++, map++)
> > if (class_in_range(dp->class_id, map))
> > return map->class_names[dp->class_id - map->base];
> >
> > @@ -1208,30 +1211,31 @@ static const struct proc_ops proc_fops = {
> > .proc_write = ddebug_proc_write
> > };
> >
> > -static void ddebug_attach_module_classes(struct ddebug_table *dt,
> > - struct ddebug_class_map *classes,
> > - int num_classes)
> > +static void ddebug_attach_module_classes(struct ddebug_table *dt, struct
> > _ddebug_info *di)
> > {
> > struct ddebug_class_map *cm;
> > - int i, j, ct = 0;
> > + int i, nc = 0;
> >
> > - for (cm = classes, i = 0; i < num_classes; i++, cm++) {
> > + /*
> > + * Find this module's classmaps in a subrange/wholerange of
> > + * the builtin/modular classmap vector/section. Save the start
> > + * and length of the subrange at its edges.
> > + */
> > + for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {
> >
> > if (!strcmp(cm->mod_name, dt->mod_name)) {
> > -
> > - v2pr_info("class[%d]: module:%s base:%d len:%d
> > ty:%d\n", i,
> > - cm->mod_name, cm->base, cm->length,
> > cm->map_type);
> > -
> > - for (j = 0; j < cm->length; j++)
> > - v3pr_info(" %d: %d %s\n", j + cm->base, j,
> > - cm->class_names[j]);
> > -
> > - list_add(&cm->link, &dt->maps);
> > - ct++;
> > + if (!nc) {
> > + v2pr_info("start subrange, class[%d]:
> > module:%s base:%d len:%d ty:%d\n",
> > + i, cm->mod_name, cm->base,
> > cm->length, cm->map_type);
> > + dt->classes = cm;
> > + }
> > + nc++;
> > }
> > }
> > - if (ct)
> > - vpr_info("module:%s attached %d classes\n", dt->mod_name, ct);
> > + if (nc) {
> > + dt->num_classes = nc;
> > + vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
> > + }
> > }
> >
> > /*
> > @@ -1263,10 +1267,9 @@ static int ddebug_add_module(struct _ddebug_info
> > *di, const char *modname)
> > dt->num_ddebugs = di->num_descs;
> >
> > INIT_LIST_HEAD(&dt->link);
> > - INIT_LIST_HEAD(&dt->maps);
> >
> > if (di->classes && di->num_classes)
> > - ddebug_attach_module_classes(dt, di->classes,
> > di->num_classes);
> > + ddebug_attach_module_classes(dt, di);
> >
> > mutex_lock(&ddebug_lock);
> > list_add_tail(&dt->link, &ddebug_tables);
> > @@ -1379,8 +1382,8 @@ static void ddebug_remove_all_tables(void)
> > mutex_lock(&ddebug_lock);
> > while (!list_empty(&ddebug_tables)) {
> > struct ddebug_table *dt = list_entry(ddebug_tables.next,
> > - struct ddebug_table,
> > - link);
> > + struct ddebug_table,
> > + link);
> > ddebug_table_free(dt);
> > }
> > mutex_unlock(&ddebug_lock);
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>
