[Pdbg] [PATCH 1/2] libpdbg: Create struct pdbg_target_priv
Alistair Popple
alistair at popple.id.au
Wed Sep 4 13:50:25 AEST 2019
Amitay,
> Even though internally libpdbg uses pdbg_target to store part of
> hardware unit, exposing pdbg_target as a hardware unit is a bad idea.
> From libpdbg api it's clear that pdbg_target represents an instance of
> a hardware unit. So overloading pdbg_target as a hardware unit adds
> more confusion.
>
> It would make more sense (to me at least) if we call new structure
> pdbg_hwunit (or similar but different from pdbg_target). This will
> make the libpdbg api cleaner and less confusing. (What we really need
> is virtual base class and every hardware unit is derived from that.
> Need a good way to do that in C.)
Ok, it took me a moment to work out what the issue was and what you were
suggesting but now that I've figured it out I agree - it would be much clearer/
cleaner to differentiate between the two. Will do a v2 that fixes it.
Next up, converting to C++ (-:
> Also, let's not add any structures to libpdbg API without pdbg_ prefix
> (e.g. hw_unit_info). I guess this will involve a bit of churn through
> libpdbg, but in the long run it will be better.
Thanks, that is also my intent but I missed this one. As they all (should)
only get created by a macro I doubt it will involve too much churn.
- Alistair
> Amitay.
>
> >
> > Signed-off-by: Alistair Popple <alistair at popple.id.au>
> > ---
> > libpdbg/device.c | 95 +++++++++++++++++++++++++++----------------
> > ----
> > libpdbg/hwunit.c | 3 ++
> > libpdbg/libpdbg.c | 30 +++++++++------
> > libpdbg/target.c | 12 +++---
> > libpdbg/target.h | 8 +++-
> > 5 files changed, 89 insertions(+), 59 deletions(-)
> >
> > diff --git a/libpdbg/device.c b/libpdbg/device.c
> > index efa9ce4..e009a1a 100644
> > --- a/libpdbg/device.c
> > +++ b/libpdbg/device.c
> > @@ -67,6 +67,7 @@ static const char *take_name(const char *name)
> > /* Adds information representing an actual target */
> > static struct pdbg_target *dt_pdbg_target_new(const void *fdt, int
> > node_offset)
> > {
> > + struct pdbg_target_priv *target_priv;
> > struct pdbg_target *target;
> > struct pdbg_target_class *target_class;
> > const struct hw_unit_info *hw_info = NULL;
> > @@ -98,19 +99,21 @@ static struct pdbg_target
> > *dt_pdbg_target_new(const void *fdt, int node_offset)
> > /* Couldn't find anything implementing this target */
> > return NULL;
> >
> > - target = calloc(1, size);
> > - if (!target) {
> > + target_priv = calloc(1, sizeof(struct pdbg_target_priv) -
> > sizeof(struct pdbg_target) + size);
> > + if (!target_priv) {
> > prerror("Failed to allocate node\n");
> > abort();
> > }
> >
> > + target = &target_priv->target;
> > +
> > /* hw_info->hw_unit points to a per-target struct type. This
> > * works because the first member in the per-target struct is
> > * guaranteed to be the struct pdbg_target (see the comment
> > * above DECLARE_HW_UNIT). */
> > memcpy(target, hw_info->hw_unit, size);
> > target_class = get_target_class(target);
> > - list_add_tail(&target_class->targets, &target->class_link);
> > + list_add_tail(&target_class->targets, &target_priv-
> > >class_link);
> >
> > return target;
> > }
> > @@ -118,31 +121,36 @@ static struct pdbg_target
> > *dt_pdbg_target_new(const void *fdt, int node_offset)
> > static struct pdbg_target *dt_new_node(const char *name, const void
> > *fdt, int node_offset)
> > {
> > struct pdbg_target *node = NULL;
> > - size_t size = sizeof(*node);
> > + struct pdbg_target_priv *target_priv;
> >
> > - if (fdt)
> > + if (fdt) {
> > node = dt_pdbg_target_new(fdt, node_offset);
> > -
> > - if (!node)
> > - node = calloc(1, size);
> > + target_priv = pdbg_target_to_priv(node);
> > + }
> >
> > if (!node) {
> > + target_priv = calloc(1, sizeof(*target_priv));
> > + node = &target_priv->target;
> > + }
> > +
> > + if (!node && !target_priv) {
> > prerror("Failed to allocate node\n");
> > abort();
> > }
> >
> > - node->dn_name = take_name(name);
> > + target_priv->dn_name = take_name(name);
> > node->parent = NULL;
> > - list_head_init(&node->properties);
> > - list_head_init(&node->children);
> > - node->phandle = ++last_phandle;
> > + list_head_init(&target_priv->properties);
> > + list_head_init(&target_priv->children);
> > + target_priv->phandle = ++last_phandle;
> >
> > return node;
> > }
> >
> > static const char *get_unitname(const struct pdbg_target *node)
> > {
> > - const char *c = strchr(node->dn_name, '@');
> > + struct pdbg_target_priv *target_priv =
> > pdbg_target_to_priv(node);
> > + const char *c = strchr(target_priv->dn_name, '@');
> >
> > if (!c)
> > return NULL;
> > @@ -152,13 +160,14 @@ static const char *get_unitname(const struct
> > pdbg_target *node)
> >
> > static int dt_cmp_subnodes(const struct pdbg_target *a, const struct
> > pdbg_target *b)
> > {
> > + struct pdbg_target_priv *priv_a = pdbg_target_to_priv(a),
> > *priv_b = pdbg_target_to_priv(b);
> > const char *a_unit = get_unitname(a);
> > const char *b_unit = get_unitname(b);
> >
> > - ptrdiff_t basenamelen = a_unit - a->dn_name;
> > + ptrdiff_t basenamelen = a_unit - priv_a->dn_name;
> >
> > /* sort hex unit addresses by number */
> > - if (a_unit && b_unit && !strncmp(a->dn_name, b->dn_name,
> > basenamelen)) {
> > + if (a_unit && b_unit && !strncmp(priv_a->dn_name, priv_b-
> > >dn_name, basenamelen)) {
> > unsigned long long a_num, b_num;
> > char *a_end, *b_end;
> >
> > @@ -170,29 +179,31 @@ static int dt_cmp_subnodes(const struct
> > pdbg_target *a, const struct pdbg_target
> > return (a_num > b_num) - (a_num < b_num);
> > }
> >
> > - return strcmp(a->dn_name, b->dn_name);
> > + return strcmp(priv_a->dn_name, priv_b->dn_name);
> > }
> >
> > static bool dt_attach_root(struct pdbg_target *parent, struct
> > pdbg_target *root)
> > {
> > - struct pdbg_target *node = NULL;
> > + struct pdbg_target_priv *root_priv = pdbg_target_to_priv(root);
> > + struct pdbg_target_priv *parent_priv =
> > pdbg_target_to_priv(parent);
> > + struct pdbg_target_priv *node = NULL;
> >
> > assert(!root->parent);
> >
> > - if (list_empty(&parent->children)) {
> > - list_add(&parent->children, &root->list);
> > + if (list_empty(&parent_priv->children)) {
> > + list_add(&parent_priv->children, &root_priv->list);
> > root->parent = parent;
> >
> > return true;
> > }
> >
> > - dt_for_each_child(parent, node) {
> > - int cmp = dt_cmp_subnodes(node, root);
> > + dt_for_each_child(parent_priv, node) {
> > + int cmp = dt_cmp_subnodes(&node->target, root);
> >
> > /* Look for duplicates */
> > if (cmp == 0) {
> > prerror("DT: %s failed, duplicate %s\n",
> > - __func__, root->dn_name);
> > + __func__, root_priv->dn_name);
> > return false;
> > }
> >
> > @@ -202,7 +213,7 @@ static bool dt_attach_root(struct pdbg_target
> > *parent, struct pdbg_target *root)
> > break;
> > }
> >
> > - list_add_before(&parent->children, &root->list, &node->list);
> > + list_add_before(&parent_priv->children, &root_priv->list,
> > &node->list);
> > root->parent = parent;
> >
> > return true;
> > @@ -219,7 +230,7 @@ static char *dt_get_path(const struct pdbg_target
> > *node)
> > return strdup("<NULL>");
> >
> > for (n = node; n; n = n->parent) {
> > - len += strlen(n->dn_name);
> > + len += strlen(pdbg_target_to_priv(n)->dn_name);
> > if (n->parent || n == node)
> > len++;
> > }
> > @@ -227,9 +238,9 @@ static char *dt_get_path(const struct pdbg_target
> > *node)
> > assert(path);
> > p = path + len;
> > for (n = node; n; n = n->parent) {
> > - len = strlen(n->dn_name);
> > + len = strlen(pdbg_target_to_priv(n)->dn_name);
> > p -= len;
> > - memcpy(p, n->dn_name, len);
> > + memcpy(p, pdbg_target_to_priv(n)->dn_name, len);
> > if (n->parent || n == node)
> > *(--p) = '/';
> > }
> > @@ -272,7 +283,8 @@ static const char *__dt_path_split(const char *p,
> >
> > static struct pdbg_target *dt_find_by_path(struct pdbg_target *root,
> > const char *path)
> > {
> > - struct pdbg_target *n;
> > + struct pdbg_target_priv *n;
> > + struct pdbg_target_priv *root_priv = pdbg_target_to_priv(root);
> > const char *pn, *pa = NULL, *p = path, *nn = NULL, *na = NULL;
> > unsigned int pnl, pal, nnl, nal;
> > bool match;
> > @@ -286,7 +298,7 @@ static struct pdbg_target *dt_find_by_path(struct
> > pdbg_target *root, const char
> >
> > /* Compare with each child node */
> > match = false;
> > - list_for_each(&root->children, n, list) {
> > + list_for_each(&root_priv->children, n, list) {
> > match = true;
> > __dt_path_split(n->dn_name, &nn, &nnl, &na,
> > &nal);
> > if (pnl && (pnl != nnl || strncmp(pn, nn,
> > pnl)))
> > @@ -294,7 +306,7 @@ static struct pdbg_target *dt_find_by_path(struct
> > pdbg_target *root, const char
> > if (pal && (pal != nal || strncmp(pa, na,
> > pal)))
> > match = false;
> > if (match) {
> > - root = n;
> > + root = &n->target;
> > break;
> > }
> > }
> > @@ -311,7 +323,7 @@ static struct dt_property *dt_find_property(const
> > struct pdbg_target *node,
> > {
> > struct dt_property *i = NULL;
> >
> > - list_for_each(&node->properties, i, list)
> > + list_for_each(&pdbg_target_to_priv(node)->properties, i, list)
> > if (strcmp(i->name, name) == 0)
> > return i;
> > return NULL;
> > @@ -341,7 +353,7 @@ static struct dt_property *new_property(struct
> > pdbg_target *node,
> >
> > p->name = take_name(name);
> > p->len = size;
> > - list_add_tail(&node->properties, &p->list);
> > + list_add_tail(&pdbg_target_to_priv(node)->properties, &p-
> > >list);
> > return p;
> > }
> >
> > @@ -349,6 +361,7 @@ static struct dt_property *dt_add_property(struct
> > pdbg_target *node,
> > const char *name,
> > const void *val, size_t size)
> > {
> > + struct pdbg_target_priv *node_priv = pdbg_target_to_priv(node);
> > struct dt_property *p;
> >
> > /*
> > @@ -358,9 +371,9 @@ static struct dt_property *dt_add_property(struct
> > pdbg_target *node,
> > if (strcmp(name, "linux,phandle") == 0 ||
> > strcmp(name, "phandle") == 0) {
> > assert(size == 4);
> > - node->phandle = *(const u32 *)val;
> > - if (node->phandle >= last_phandle)
> > - last_phandle = node->phandle;
> > + node_priv->phandle = *(const u32 *)val;
> > + if (node_priv->phandle >= last_phandle)
> > + last_phandle = node_priv->phandle;
> > return NULL;
> > }
> >
> > @@ -422,21 +435,23 @@ static u32 dt_property_get_cell(const struct
> > dt_property *prop, u32 index)
> > /* First child of this node. */
> > static struct pdbg_target *dt_first(const struct pdbg_target *root)
> > {
> > - return list_top(&root->children, struct pdbg_target, list);
> > + return &list_top(&pdbg_target_to_priv(root)->children, struct
> > pdbg_target_priv, list)->target;
> > }
> >
> > /* Return next node, or NULL. */
> > static struct pdbg_target *dt_next(const struct pdbg_target *root,
> > const struct pdbg_target *prev)
> > {
> > + struct pdbg_target_priv *prev_priv = pdbg_target_to_priv(prev);
> > +
> > /* Children? */
> > - if (!list_empty(&prev->children))
> > + if (!list_empty(&prev_priv->children))
> > return dt_first(prev);
> >
> > do {
> > /* More siblings? */
> > - if (prev->list.next != &prev->parent->children.n)
> > - return list_entry(prev->list.next, struct
> > pdbg_target,list);
> > + if (prev_priv->list.next != &pdbg_target_to_priv(prev-
> > >parent)->children.n)
> > + return &list_entry(prev_priv->list.next, struct
> > pdbg_target_priv, list)->target;
> >
> > /* No more siblings, move up to parent. */
> > prev = prev->parent;
> > @@ -563,11 +578,11 @@ static int dt_expand_node(struct pdbg_target
> > *node, const void *fdt, int fdt_nod
> > name = fdt_string(fdt, fdt32_to_cpu(prop-
> > >nameoff));
> > if (strcmp("index", name) == 0) {
> > memcpy(&data, prop->data,
> > sizeof(data));
> > - node->index = fdt32_to_cpu(data);
> > + pdbg_target_to_priv(node)->index =
> > fdt32_to_cpu(data);
> > }
> >
> > if (strcmp("status", name) == 0)
> > - node->status = str_to_status(prop-
> > >data);
> > + pdbg_target_to_priv(node)->status =
> > str_to_status(prop->data);
> >
> > dt_add_property(node, name, prop->data,
> > fdt32_to_cpu(prop->len));
> > diff --git a/libpdbg/hwunit.c b/libpdbg/hwunit.c
> > index c7ec63d..5c40d23 100644
> > --- a/libpdbg/hwunit.c
> > +++ b/libpdbg/hwunit.c
> > @@ -21,6 +21,9 @@
> >
> > #define MAX_HW_UNITS 1024
> >
> > +struct pdbg_target pdbg_target_size;
> > +uint8_t pdbg_target_size_test[sizeof(struct pdbg_target)];
> > +
> > static const struct hw_unit_info *g_hw_unit[MAX_HW_UNITS];
> > static int g_hw_unit_count;
> >
> > diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> > index 0c5a451..bec2488 100644
> > --- a/libpdbg/libpdbg.c
> > +++ b/libpdbg/libpdbg.c
> > @@ -9,6 +9,7 @@ static pdbg_progress_tick_t progress_tick;
> > struct pdbg_target *__pdbg_next_target(const char *class, struct
> > pdbg_target *parent, struct pdbg_target *last)
> > {
> > struct pdbg_target *next, *tmp;
> > + struct pdbg_target_priv *last_priv;
> > struct pdbg_target_class *target_class;
> >
> > if (class && !find_target_class(class))
> > @@ -17,15 +18,17 @@ struct pdbg_target *__pdbg_next_target(const char
> > *class, struct pdbg_target *pa
> > target_class = require_target_class(class);
> >
> > retry:
> > + last_priv = pdbg_target_to_priv(last);
> > +
> > /* No more targets left to check in this class */
> > - if ((last && last->class_link.next == &target_class->targets.n)
> > ||
> > + if ((last && last_priv->class_link.next == &target_class-
> > >targets.n) ||
> > list_empty(&target_class->targets))
> > return NULL;
> >
> > if (last)
> > - next = list_entry(last->class_link.next, struct
> > pdbg_target, class_link);
> > + next = &list_entry(last_priv->class_link.next, struct
> > pdbg_target_priv, class_link)->target;
> > else
> > - if (!(next = list_top(&target_class->targets, struct
> > pdbg_target, class_link)))
> > + if (!(next = &list_top(&target_class->targets, struct
> > pdbg_target_priv, class_link)->target))
> > return NULL;
> >
> > if (!parent)
> > @@ -45,21 +48,24 @@ retry:
> >
> > struct pdbg_target *__pdbg_next_child_target(struct pdbg_target
> > *parent, struct pdbg_target *last)
> > {
> > - if (!parent || list_empty(&parent->children))
> > + struct pdbg_target_priv *parent_priv =
> > pdbg_target_to_priv(parent);
> > + struct pdbg_target_priv *last_priv = pdbg_target_to_priv(last);
> > +
> > + if (!parent || list_empty(&parent_priv->children))
> > return NULL;
> >
> > if (!last)
> > - return list_top(&parent->children, struct pdbg_target,
> > list);
> > + return &list_top(&parent_priv->children, struct
> > pdbg_target_priv, list)->target;
> >
> > - if (last->list.next == &parent->children.n)
> > + if (last_priv->list.next == &parent_priv->children.n)
> > return NULL;
> >
> > - return list_entry(last->list.next, struct pdbg_target, list);
> > + return &list_entry(last_priv->list.next, struct
> > pdbg_target_priv, list)->target;
> > }
> >
> > enum pdbg_target_status pdbg_target_status(struct pdbg_target
> > *target)
> > {
> > - return target->status;
> > + return pdbg_target_to_priv(target)->status;
> > }
> >
> > void pdbg_target_status_set(struct pdbg_target *target, enum
> > pdbg_target_status status)
> > @@ -68,7 +74,7 @@ void pdbg_target_status_set(struct pdbg_target
> > *target, enum pdbg_target_status
> > * blow up obviously if this happens */
> > assert(status == PDBG_TARGET_DISABLED || status ==
> > PDBG_TARGET_MUSTEXIST);
> >
> > - target->status = status;
> > + pdbg_target_to_priv(target)->status = status;
> > }
> >
> > /* Searches up the tree and returns the first valid index found */
> > @@ -76,12 +82,12 @@ uint32_t pdbg_target_index(struct pdbg_target
> > *target)
> > {
> > struct pdbg_target *dn;
> >
> > - for (dn = target; dn && dn->index == -1; dn = dn->parent);
> > + for (dn = target; dn && pdbg_target_to_priv(dn)->index == -1;
> > dn = dn->parent);
> >
> > if (!dn)
> > return -1;
> > else
> > - return dn->index;
> > + return pdbg_target_to_priv(dn)->index;
> > }
> >
> > /* Find a target parent from the given class */
> > @@ -132,7 +138,7 @@ char *pdbg_target_name(struct pdbg_target
> > *target)
> >
> > const char *pdbg_target_dn_name(struct pdbg_target *target)
> > {
> > - return target->dn_name;
> > + return pdbg_target_to_priv(target)->dn_name;
> > }
> >
> > int pdbg_target_u32_property(struct pdbg_target *target, const char
> > *name, uint32_t *val)
> > diff --git a/libpdbg/target.c b/libpdbg/target.c
> > index 5808d02..ce2bb2d 100644
> > --- a/libpdbg/target.c
> > +++ b/libpdbg/target.c
> > @@ -400,7 +400,7 @@ enum pdbg_target_status pdbg_target_probe(struct
> > pdbg_target *target)
> > case PDBG_TARGET_NONEXISTENT:
> > /* The parent doesn't exist neither does it's
> > * children */
> > - target->status = PDBG_TARGET_NONEXISTENT;
> > + pdbg_target_to_priv(target)->status =
> > PDBG_TARGET_NONEXISTENT;
> > return PDBG_TARGET_NONEXISTENT;
> >
> > case PDBG_TARGET_DISABLED:
> > @@ -427,11 +427,11 @@ enum pdbg_target_status
> > pdbg_target_probe(struct pdbg_target *target)
> > if (target->probe && target->probe(target)) {
> > /* Could not find the target */
> > assert(pdbg_target_status(target) !=
> > PDBG_TARGET_MUSTEXIST);
> > - target->status = PDBG_TARGET_NONEXISTENT;
> > + pdbg_target_to_priv(target)->status =
> > PDBG_TARGET_NONEXISTENT;
> > return PDBG_TARGET_NONEXISTENT;
> > }
> >
> > - target->status = PDBG_TARGET_ENABLED;
> > + pdbg_target_to_priv(target)->status = PDBG_TARGET_ENABLED;
> > return PDBG_TARGET_ENABLED;
> > }
> >
> > @@ -449,7 +449,7 @@ void pdbg_target_release(struct pdbg_target
> > *target)
> > /* Release the target */
> > if (target->release)
> > target->release(target);
> > - target->status = PDBG_TARGET_RELEASED;
> > + pdbg_target_to_priv(target)->status = PDBG_TARGET_RELEASED;
> > }
> >
> > /*
> > @@ -477,10 +477,10 @@ bool pdbg_target_is_class(struct pdbg_target
> > *target, const char *class)
> >
> > void *pdbg_target_priv(struct pdbg_target *target)
> > {
> > - return target->priv;
> > + return pdbg_target_to_priv(target)->priv;
> > }
> >
> > void pdbg_target_priv_set(struct pdbg_target *target, void *priv)
> > {
> > - target->priv = priv;
> > + pdbg_target_to_priv(target)->priv = priv;
> > }
> > diff --git a/libpdbg/target.h b/libpdbg/target.h
> > index 2ecbf98..ba2b20d 100644
> > --- a/libpdbg/target.h
> > +++ b/libpdbg/target.h
> > @@ -39,19 +39,25 @@ struct pdbg_target {
> > int (*probe)(struct pdbg_target *target);
> > void (*release)(struct pdbg_target *target);
> > uint64_t (*translate)(struct pdbg_target *target, uint64_t
> > addr);
> > + struct pdbg_target *parent;
> > +};
> > +
> > +struct pdbg_target_priv {
> > int index;
> > enum pdbg_target_status status;
> > const char *dn_name;
> > struct list_node list;
> > struct list_head properties;
> > struct list_head children;
> > - struct pdbg_target *parent;
> > u32 phandle;
> > bool probed;
> > struct list_node class_link;
> > + struct pdbg_target target;
> > void *priv;
> > };
> >
> > +#define pdbg_target_to_priv(x) (x ? container_of(x, struct
> > pdbg_target_priv, target) : NULL)
> > +
> > struct pdbg_target *require_target_parent(struct pdbg_target
> > *target);
> > struct pdbg_target_class *find_target_class(const char *name);
> > struct pdbg_target_class *require_target_class(const char *name);
>
> Amitay.
>
More information about the Pdbg
mailing list