[Pdbg] [PATCH 1/2] libpdbg: Create struct pdbg_target_priv
Amitay Isaacs
amitay at ozlabs.org
Wed Sep 4 13:30:42 AEST 2019
On Tue, 2019-09-03 at 17:59 +1000, Alistair Popple wrote:
> In order to allow external programs to register their own hardware
> units it is required to expose the definition of struct
> pdbg_target. However this means the struct definition now becomes
> part
> of the library ABI/API, making it difficult to change fields without
> breaking applications.
>
> It also means we would have to install the ccan list headers, which
> themselves depend on other ccan modules. Instead create a seperate
> struct definition (struct pdbg_target_priv) which encapsulates the
> libpdbg specific fields which should not be accessed directly by
> applications and switch the library to use those.
>
> This should allow a stable definition of struct pdbg_target to be
> exported to applications without preventing changes to the underlying
> library fields.
I agree with the idea, but I have few reservations about naming.
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.)
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.
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);
> --
> 2.20.1
>
Amitay.
--
When you have been wronged, a poor memory is your best response.
More information about the Pdbg
mailing list