[Pdbg] [PATCH v2 08/11] libpdbg: Lazy probing
Alistair Popple
alistair at popple.id.au
Fri Apr 13 11:55:48 AEST 2018
Something here is not quite right:
$ ./pdbg -b fake -p0 getscom 0xf000f
pdbg: libpdbg/target.c:186: require_target_parent: Assertion `target->parent' failed.
Aborted
We will need to do some more digging to figure out what is wrong before we can
merge this. In general I need to review the approach taken here as I'm not sure
I like it, although our target selection code is a complete mess so perhaps we
should start fixing it there instead.
- Alistair
On Thursday, 12 April 2018 4:01:57 PM AEST Amitay Isaacs wrote:
> From: Cyril Bur <cyrilbur at gmail.com>
>
> When libpdbg runs though the device tree and calls probe() on all the
> targets it marks any node (and all its children) which fails to probe
> as "disabled".
>
> When pdbg recieves arguments to specifiy a pib, core or thread (-p -c
> -t) it runs though the device tree and marks nodes which do not match
> the specified -p -c or -t as "disabled".
>
> These two scenarios are related - that is we don't want to be operating
> on any disabled nodes. However, they are not the same.
>
> Consider HTM wanting to know if the machine has powersave disabled. A
> simple way would be to loop through all the threads and if any of them
> are not in "active" state then powersave must be enabled. In this
> scenario HTM want to be able to get the status of every thread on the
> machine regardless of -p -c -t flags but if the machine isn't fully
> speced out and has cores which didn't probe, HTM wants to know to
> ignore those threads.
>
> This patch introduces a "nonexistant" pdbg target status which is set
> during libpdbg probe() time. Later pdbg can mark nodes to targets that
> do 'exist' but it would like to avoid as "disabled".
>
> Attempting to solve the above problem with only a "nonexistant" flag
> would also require everything to be probed before selecting out chips
> and threads that the user is not interested in. This would be highly
> inefficient.
>
> This patch probes the system lazily
>
> Signed-off-by: Cyril Bur <cyrilbur at gmail.com>
> ---
> libpdbg/libpdbg.c | 2 ++
> libpdbg/libpdbg.h | 4 ++--
> libpdbg/target.c | 28 ++++++++++++++++++++--------
> libpdbg/target.h | 1 +
> src/htm.c | 5 +++++
> src/main.c | 39 ++++++++++++++++++++++-----------------
> src/main.h | 3 ++-
> 7 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> index 86f3dc4..007642b 100644
> --- a/libpdbg/libpdbg.c
> +++ b/libpdbg/libpdbg.c
> @@ -69,6 +69,8 @@ enum pdbg_target_status pdbg_target_status(struct pdbg_target *target)
> return PDBG_TARGET_DISABLED;
> else if (!strcmp(p->prop, "hidden"))
> return PDBG_TARGET_HIDDEN;
> + else if (!strcmp(p->prop, "nonexistent"))
> + return PDBG_TARGET_NONEXISTENT;
> else
> assert(0);
> }
> diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> index 8dec016..ae8f7c0 100644
> --- a/libpdbg/libpdbg.h
> +++ b/libpdbg/libpdbg.h
> @@ -9,7 +9,7 @@ struct pdbg_taget *pdbg_root_target;
> /* loops/iterators */
> struct pdbg_target *__pdbg_next_target(const char *klass, struct pdbg_target *parent, struct pdbg_target *last);
> struct pdbg_target *__pdbg_next_child_target(struct pdbg_target *parent, struct pdbg_target *last);
> -enum pdbg_target_status {PDBG_TARGET_ENABLED, PDBG_TARGET_DISABLED, PDBG_TARGET_HIDDEN};
> +enum pdbg_target_status {PDBG_TARGET_ENABLED, PDBG_TARGET_DISABLED, PDBG_TARGET_HIDDEN, PDBG_TARGET_NONEXISTENT};
>
> #define pdbg_for_each_target(class, parent, target) \
> for (target = __pdbg_next_target(class, parent, NULL); \
> @@ -36,7 +36,7 @@ uint64_t pdbg_get_address(struct pdbg_target *target, uint64_t *size);
>
> /* Misc. */
> void pdbg_targets_init(void *fdt);
> -void pdbg_target_probe(void);
> +void pdbg_target_probe(struct pdbg_target *target);
> void pdbg_enable_target(struct pdbg_target *target);
> void pdbg_disable_target(struct pdbg_target *target);
> enum pdbg_target_status pdbg_target_status(struct pdbg_target *target);
> diff --git a/libpdbg/target.c b/libpdbg/target.c
> index 984dcd2..a9ab3b9 100644
> --- a/libpdbg/target.c
> +++ b/libpdbg/target.c
> @@ -254,7 +254,7 @@ void pdbg_targets_init(void *fdt)
> }
>
> /* Disable a node and all it's children */
> -static void disable_node(struct pdbg_target *target)
> +static void deprobe(struct pdbg_target *target)
> {
> struct pdbg_target *t;
> struct dt_property *p;
> @@ -263,9 +263,9 @@ static void disable_node(struct pdbg_target *target)
> if (p)
> dt_del_property(target, p);
>
> - dt_add_property_string(target, "status", "disabled");
> + dt_add_property_string(target, "status", "nonexistent");
> dt_for_each_child(target, t)
> - disable_node(t);
> + deprobe(t);
> }
>
> static void _target_probe(struct pdbg_target *target)
> @@ -280,25 +280,37 @@ static void _target_probe(struct pdbg_target *target)
> }
>
> p = dt_find_property(target, "status");
> - if ((p && !strcmp(p->prop, "disabled")) || (target->probe && (rc = target->probe(target)))) {
> + if ((p && !strcmp(p->prop, "nonexistent")) || (target->probe && (rc = target->probe(target)))) {
> if (rc)
> PR_DEBUG("not found\n");
> else
> PR_DEBUG("disabled\n");
>
> - disable_node(target);
> + deprobe(target);
> } else {
> + if (!p)
> + dt_add_property_string(target, "status", "enabled");
> + target->probed = true;
> PR_DEBUG("success\n");
> }
> }
>
> /* We walk the tree root down disabling targets which might/should
> * exist but don't */
> -void pdbg_target_probe(void)
> +void pdbg_target_probe(struct pdbg_target *target)
> {
> - struct pdbg_target *target;
> + struct pdbg_target *parent;
> + struct dt_property *p;
> +
> + if (!target || target->probed)
> + return;
> +
> + parent = require_target_parent(target);
> + if (parent && !parent->probed)
> + pdbg_target_probe(parent);
>
> - dt_for_each_node(dt_root, target)
> + p = dt_find_property(target, "status");
> + if (!p || strcmp(p->prop, "nonexistent") != 0)
> _target_probe(target);
> }
>
> diff --git a/libpdbg/target.h b/libpdbg/target.h
> index e7f793c..0b3a65c 100644
> --- a/libpdbg/target.h
> +++ b/libpdbg/target.h
> @@ -51,6 +51,7 @@ struct pdbg_target {
> struct list_head children;
> struct pdbg_target *parent;
> u32 phandle;
> + bool probed;
> struct list_node class_link;
> };
>
> diff --git a/src/htm.c b/src/htm.c
> index 8c25a18..ab63108 100644
> --- a/src/htm.c
> +++ b/src/htm.c
> @@ -80,6 +80,7 @@ static int run_start(enum htm_type type, int optind, int argc, char *argv[])
> int rc = 0;
>
> pdbg_for_each_class_target(HTM_ENUM_TO_STRING(type), target) {
> + pdbg_target_probe(target);
> if (target_is_disabled(target))
> continue;
>
> @@ -101,6 +102,7 @@ static int run_stop(enum htm_type type, int optind, int argc, char *argv[])
> int rc = 0;
>
> pdbg_for_each_class_target(HTM_ENUM_TO_STRING(type), target) {
> + pdbg_target_probe(target);
> if (target_is_disabled(target))
> continue;
>
> @@ -122,6 +124,7 @@ static int run_status(enum htm_type type, int optind, int argc, char *argv[])
> int rc = 0;
>
> pdbg_for_each_class_target(HTM_ENUM_TO_STRING(type), target) {
> + pdbg_target_probe(target);
> if (target_is_disabled(target))
> continue;
>
> @@ -145,6 +148,7 @@ static int run_reset(enum htm_type type, int optind, int argc, char *argv[])
> int rc = 0;
>
> pdbg_for_each_class_target(HTM_ENUM_TO_STRING(type), target) {
> + pdbg_target_probe(target);
> if (target_is_disabled(target))
> continue;
>
> @@ -181,6 +185,7 @@ static int run_dump(enum htm_type type, int optind, int argc, char *argv[])
> /* size = 0 will dump everything */
> printf("Dumping HTM trace to file [chip].[#]%s\n", filename);
> pdbg_for_each_class_target(HTM_ENUM_TO_STRING(type), target) {
> + pdbg_target_probe(target);
> if (target_is_disabled(target))
> continue;
>
> diff --git a/src/main.c b/src/main.c
> index c6b645e..f4c03ae 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -268,8 +268,9 @@ int for_each_child_target(char *class, struct pdbg_target *parent,
> pdbg_for_each_target(class, parent, target) {
> index = pdbg_target_index(target);
> assert(index != -1);
> + pdbg_target_probe(target);
> status = pdbg_target_status(target);
> - if (status == PDBG_TARGET_DISABLED || status == PDBG_TARGET_HIDDEN)
> + if (status == PDBG_TARGET_DISABLED || status == PDBG_TARGET_HIDDEN || status == PDBG_TARGET_NONEXISTENT)
> continue;
>
> rc += cb(target, index, arg1, arg2);
> @@ -288,8 +289,9 @@ int for_each_target(char *class, int (*cb)(struct pdbg_target *, uint32_t, uint6
> pdbg_for_each_class_target(class, target) {
> index = pdbg_target_index(target);
> assert(index != -1);
> + pdbg_target_probe(target);
> status = pdbg_target_status(target);
> - if (status == PDBG_TARGET_DISABLED || status == PDBG_TARGET_HIDDEN)
> + if (status == PDBG_TARGET_DISABLED || status == PDBG_TARGET_HIDDEN || status == PDBG_TARGET_NONEXISTENT)
> continue;
>
> rc += cb(target, index, arg1, arg2);
> @@ -317,6 +319,16 @@ extern unsigned char _binary_p8_host_dtb_o_start;
> extern unsigned char _binary_p8_host_dtb_o_end;
> extern unsigned char _binary_p9_host_dtb_o_start;
> extern unsigned char _binary_p9_host_dtb_o_end;
> +
> +static void mark_disabled(struct pdbg_target *target)
> +{
> + struct pdbg_target *child;
> +
> + pdbg_disable_target(target);
> +
> + pdbg_for_each_child_target(target, child)
> + pdbg_disable_target(child);
> +}
> static int target_select(void)
> {
> struct pdbg_target *fsi, *pib, *chip, *thread;
> @@ -383,31 +395,25 @@ static int target_select(void)
> pdbg_set_target_property(pib, "bus", device_node, strlen(device_node) + 1);
>
> if (processorsel[proc_index]) {
> - pdbg_enable_target(pib);
> pdbg_for_each_target("core", pib, chip) {
> int chip_index = pdbg_target_index(chip);
> if (chipsel[proc_index][chip_index]) {
> - pdbg_enable_target(chip);
> pdbg_for_each_target("thread", chip, thread) {
> int thread_index = pdbg_target_index(thread);
> - if (threadsel[proc_index][chip_index][thread_index])
> - pdbg_enable_target(thread);
> - else
> - pdbg_disable_target(thread);
> + if (!threadsel[proc_index][chip_index][thread_index])
> + mark_disabled(thread);
> }
> } else
> - pdbg_disable_target(chip);
> + mark_disabled(chip);
> }
> } else
> - pdbg_disable_target(pib);
> + mark_disabled(pib);
> }
>
> pdbg_for_each_class_target("fsi", fsi) {
> int index = pdbg_target_index(fsi);
> - if (processorsel[index])
> - pdbg_enable_target(fsi);
> - else
> - pdbg_disable_target(fsi);
> + if (!processorsel[index])
> + mark_disabled(fsi);
> }
>
> return 0;
> @@ -419,8 +425,9 @@ void print_target(struct pdbg_target *target, int level)
> struct pdbg_target *next;
> enum pdbg_target_status status;
>
> + pdbg_target_probe(target);
> status = pdbg_target_status(target);
> - if (status == PDBG_TARGET_DISABLED)
> + if (status == PDBG_TARGET_DISABLED || status == PDBG_TARGET_NONEXISTENT)
> return;
>
> if (status == PDBG_TARGET_ENABLED) {
> @@ -493,8 +500,6 @@ int main(int argc, char *argv[])
> if (target_select())
> return 1;
>
> - pdbg_target_probe();
> -
> for (i = 0; i < ARRAY_SIZE(actions); i++) {
> if (strcmp(argv[optind], actions[i].name) == 0) {
> rc = actions[i].fn(optind, argc, argv);
> diff --git a/src/main.h b/src/main.h
> index 71599ee..d5b7dd6 100644
> --- a/src/main.h
> +++ b/src/main.h
> @@ -21,7 +21,8 @@ enum backend { FSI, I2C, KERNEL, FAKE, HOST };
>
> static inline bool target_is_disabled(struct pdbg_target *target)
> {
> - return pdbg_target_status(target) == PDBG_TARGET_DISABLED;
> + return pdbg_target_status(target) == PDBG_TARGET_DISABLED ||
> + pdbg_target_status(target) == PDBG_TARGET_NONEXISTENT;
> }
>
> /* Returns the sum of return codes. This can be used to count how many targets the callback was run on. */
>
More information about the Pdbg
mailing list