| Message ID | 20181109071011.253734-9-amitay@ozlabs.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Device tree path based targeting | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-pdbg | success | Test snowpatch/job/snowpatch-pdbg on branch master |
Looks good, happy to see this code start to disappear. > @@ -523,19 +519,12 @@ static bool parse_options(int argc, char *argv[]) > > void target_select(struct pdbg_target *target) > { > - /* We abuse the private data pointer atm to indicate the target is > - * selected */ > - pdbg_target_priv_set(target, (void *) 1); > -} > - > -void target_unselect(struct pdbg_target *target) > -{ > - pdbg_target_priv_set(target, NULL); > + path_target_add(target); Do you think there's any value in keeping this function rather than just changing the existing code to call path_target_add() directly? It's not part of any external API so it seems to just add an unnecessary indirection. > } > > bool target_selected(struct pdbg_target *target) > { > - return (bool) pdbg_target_priv(target); > + return path_target_selected(target); Ditto. - Alistair > } > > /* Returns the sum of return codes. This can be used to count how many > targets the callback was run on. */ @@ -554,7 +543,6 @@ int > for_each_child_target(char *class, struct pdbg_target *parent, > > index = pdbg_target_index(target); > assert(index != -1); > - pdbg_target_probe(target); > status = pdbg_target_status(target); > if (status != PDBG_TARGET_ENABLED) > continue; > @@ -578,7 +566,6 @@ int for_each_target(char *class, int (*cb)(struct > pdbg_target *, uint32_t, uint6 > > index = pdbg_target_index(target); > assert(index != -1); > - pdbg_target_probe(target); > status = pdbg_target_status(target); > if (status != PDBG_TARGET_ENABLED) > continue; > @@ -603,8 +590,6 @@ void for_each_target_release(char *class) > > static bool target_selection(void) > { > - struct pdbg_target *fsi, *pib, *chip, *thread; > - > switch (backend) { > #ifdef TARGET_ARM > case I2C: > @@ -671,63 +656,17 @@ static bool target_selection(void) > return false; > } > > - /* At this point we should have a device-tree loaded. We want > - * to walk the tree and disabled nodes we don't care about > - * prior to probing. */ > - pdbg_for_each_class_target("pib", pib) { > - int proc_index = pdbg_target_index(pib); > - > - if (backend == I2C && device_node) > - pdbg_target_set_property(pib, "bus", device_node, strlen(device_node) + > 1); - > - if (processorsel[proc_index]) { > - target_select(pib); > - pdbg_for_each_target("core", pib, chip) { > - int chip_index = pdbg_target_index(chip); > - if (pdbg_parent_index(chip, "pib") != proc_index) > - continue; > - > - if (chipsel[proc_index][chip_index]) { > - target_select(chip); > - pdbg_for_each_target("thread", chip, thread) { > - int thread_index = pdbg_target_index(thread); > - if (threadsel[proc_index][chip_index][thread_index]) > - target_select(thread); > - else > - target_unselect(thread); > - } > - } else > - target_unselect(chip); > - } > - > - /* This is kinda broken as we're overloading what '-c' > - * means - it's now up to each command to select targets > - * based on core/chiplet. We really need a better > - * solution to target selection. */ > - pdbg_for_each_target("chiplet", pib, chip) { > - int chip_index = pdbg_target_index(chip); > - if (chipsel[proc_index][chip_index]) { > - target_select(chip); > - } else > - target_unselect(chip); > - } > - } else > - target_unselect(pib); > - } > - > - pdbg_for_each_class_target("fsi", fsi) { > - int index = pdbg_target_index(fsi); > - if (processorsel[index]) > - target_select(fsi); > - else > - target_unselect(fsi); > - } > - > if (pathsel_count) { > if (!path_target_parse(pathsel, pathsel_count)) > return false; > } > > + if (!path_target_present()) { > + printf("No valid targets found or specified. Try adding -p/-c/-t options > to specify a target.\n"); + printf("Alternatively run 'pdbg -a probe' to > get a list of all valid targets\n"); + return false; > + } > + > return true; > } > > @@ -860,8 +799,5 @@ found_action: > if (rc > 0) > return 0; > > - printf("No valid targets found or specified. Try adding -p/-c/-t options > to specify a target.\n"); - printf("Alternatively run '%s -a probe' to get > a list of all valid targets\n", - basename(argv[0])); > return 1; > }
On Tuesday, 13 November 2018 3:16:22 PM AEDT Alistair Popple wrote: > Looks good, happy to see this code start to disappear. > > > @@ -523,19 +519,12 @@ static bool parse_options(int argc, char *argv[]) > > > > void target_select(struct pdbg_target *target) > > { > > > > - /* We abuse the private data pointer atm to indicate the target is > > - * selected */ > > - pdbg_target_priv_set(target, (void *) 1); > > -} > > - > > -void target_unselect(struct pdbg_target *target) > > -{ > > - pdbg_target_priv_set(target, NULL); > > + path_target_add(target); > > Do you think there's any value in keeping this function rather than just > changing the existing code to call path_target_add() directly? It's not part > of any external API so it seems to just add an unnecessary indirection. Actually better yet might be to just delete these functions and rename the new path_target_add() and path_target_selected() to target_select() and target_selected()? - Alistair > > } > > > > bool target_selected(struct pdbg_target *target) > > { > > > > - return (bool) pdbg_target_priv(target); > > + return path_target_selected(target); > > Ditto. > > - Alistair > > > } > > > > /* Returns the sum of return codes. This can be used to count how many > > > > targets the callback was run on. */ @@ -554,7 +543,6 @@ int > > for_each_child_target(char *class, struct pdbg_target *parent, > > > > index = pdbg_target_index(target); > > assert(index != -1); > > > > - pdbg_target_probe(target); > > > > status = pdbg_target_status(target); > > if (status != PDBG_TARGET_ENABLED) > > > > continue; > > > > @@ -578,7 +566,6 @@ int for_each_target(char *class, int (*cb)(struct > > pdbg_target *, uint32_t, uint6 > > > > index = pdbg_target_index(target); > > assert(index != -1); > > > > - pdbg_target_probe(target); > > > > status = pdbg_target_status(target); > > if (status != PDBG_TARGET_ENABLED) > > > > continue; > > > > @@ -603,8 +590,6 @@ void for_each_target_release(char *class) > > > > static bool target_selection(void) > > { > > > > - struct pdbg_target *fsi, *pib, *chip, *thread; > > - > > > > switch (backend) { > > > > #ifdef TARGET_ARM > > > > case I2C: > > @@ -671,63 +656,17 @@ static bool target_selection(void) > > > > return false; > > > > } > > > > - /* At this point we should have a device-tree loaded. We want > > - * to walk the tree and disabled nodes we don't care about > > - * prior to probing. */ > > - pdbg_for_each_class_target("pib", pib) { > > - int proc_index = pdbg_target_index(pib); > > - > > - if (backend == I2C && device_node) > > - pdbg_target_set_property(pib, "bus", device_node, > > strlen(device_node) + > > > 1); - > > - if (processorsel[proc_index]) { > > - target_select(pib); > > - pdbg_for_each_target("core", pib, chip) { > > - int chip_index = pdbg_target_index(chip); > > - if (pdbg_parent_index(chip, "pib") != proc_index) > > - continue; > > - > > - if (chipsel[proc_index][chip_index]) { > > - target_select(chip); > > - pdbg_for_each_target("thread", chip, thread) { > > - int thread_index = pdbg_target_index(thread); > > - if (threadsel[proc_index][chip_index][thread_index]) > > - target_select(thread); > > - else > > - target_unselect(thread); > > - } > > - } else > > - target_unselect(chip); > > - } > > - > > - /* This is kinda broken as we're overloading what '-c' > > - * means - it's now up to each command to select targets > > - * based on core/chiplet. We really need a better > > - * solution to target selection. */ > > - pdbg_for_each_target("chiplet", pib, chip) { > > - int chip_index = pdbg_target_index(chip); > > - if (chipsel[proc_index][chip_index]) { > > - target_select(chip); > > - } else > > - target_unselect(chip); > > - } > > - } else > > - target_unselect(pib); > > - } > > - > > - pdbg_for_each_class_target("fsi", fsi) { > > - int index = pdbg_target_index(fsi); > > - if (processorsel[index]) > > - target_select(fsi); > > - else > > - target_unselect(fsi); > > - } > > - > > > > if (pathsel_count) { > > > > if (!path_target_parse(pathsel, pathsel_count)) > > > > return false; > > > > } > > > > + if (!path_target_present()) { > > + printf("No valid targets found or specified. Try adding -p/-c/-t > > options > > > to specify a target.\n"); + printf("Alternatively run 'pdbg -a probe' to > > get a list of all valid targets\n"); + return false; > > + } > > + > > > > return true; > > > > } > > > > @@ -860,8 +799,5 @@ found_action: > > if (rc > 0) > > > > return 0; > > > > - printf("No valid targets found or specified. Try adding -p/-c/-t options > > to specify a target.\n"); - printf("Alternatively run '%s -a probe' to get > > a list of all valid targets\n", - basename(argv[0])); > > > > return 1; > > > > }
On Tue, 2018-11-13 at 15:19 +1100, Alistair Popple wrote: > On Tuesday, 13 November 2018 3:16:22 PM AEDT Alistair Popple wrote: > > Looks good, happy to see this code start to disappear. > > > > > @@ -523,19 +519,12 @@ static bool parse_options(int argc, char > > > *argv[]) > > > > > > void target_select(struct pdbg_target *target) > > > { > > > > > > - /* We abuse the private data pointer atm to indicate the target > > > is > > > - * selected */ > > > - pdbg_target_priv_set(target, (void *) 1); > > > -} > > > - > > > -void target_unselect(struct pdbg_target *target) > > > -{ > > > - pdbg_target_priv_set(target, NULL); > > > + path_target_add(target); > > > > Do you think there's any value in keeping this function rather than > > just > > changing the existing code to call path_target_add() directly? It's > > not part > > of any external API so it seems to just add an unnecessary > > indirection. > > Actually better yet might be to just delete these functions and > rename the new > path_target_add() and path_target_selected() to target_select() and > target_selected()? The only reason, I left those functions because they are used in htm.c and pdbgproxy.c. As we convert those commands, we can call the path api directly. Eventually target_*() functions will become unused and then we can drop them. This way less chances of breaking the existing functionality. > > - Alistair > > > > } > > > > > > bool target_selected(struct pdbg_target *target) > > > { > > > > > > - return (bool) pdbg_target_priv(target); > > > + return path_target_selected(target); > > > > Ditto. > > > > - Alistair > > > > > } > > > > > > /* Returns the sum of return codes. This can be used to count > > > how many > > > > > > targets the callback was run on. */ @@ -554,7 +543,6 @@ int > > > for_each_child_target(char *class, struct pdbg_target *parent, > > > > > > index = pdbg_target_index(target); > > > assert(index != -1); > > > > > > - pdbg_target_probe(target); > > > > > > status = pdbg_target_status(target); > > > if (status != PDBG_TARGET_ENABLED) > > > > > > continue; > > > > > > @@ -578,7 +566,6 @@ int for_each_target(char *class, int > > > (*cb)(struct > > > pdbg_target *, uint32_t, uint6 > > > > > > index = pdbg_target_index(target); > > > assert(index != -1); > > > > > > - pdbg_target_probe(target); > > > > > > status = pdbg_target_status(target); > > > if (status != PDBG_TARGET_ENABLED) > > > > > > continue; > > > > > > @@ -603,8 +590,6 @@ void for_each_target_release(char *class) > > > > > > static bool target_selection(void) > > > { > > > > > > - struct pdbg_target *fsi, *pib, *chip, *thread; > > > - > > > > > > switch (backend) { > > > > > > #ifdef TARGET_ARM > > > > > > case I2C: > > > @@ -671,63 +656,17 @@ static bool target_selection(void) > > > > > > return false; > > > > > > } > > > > > > - /* At this point we should have a device-tree loaded. We want > > > - * to walk the tree and disabled nodes we don't care about > > > - * prior to probing. */ > > > - pdbg_for_each_class_target("pib", pib) { > > > - int proc_index = pdbg_target_index(pib); > > > - > > > - if (backend == I2C && device_node) > > > - pdbg_target_set_property(pib, "bus", > > > device_node, > > > > strlen(device_node) + > > > > > 1); - > > > - if (processorsel[proc_index]) { > > > - target_select(pib); > > > - pdbg_for_each_target("core", pib, chip) { > > > - int chip_index = > > > pdbg_target_index(chip); > > > - if (pdbg_parent_index(chip, "pib") != > > > proc_index) > > > - continue; > > > - > > > - if (chipsel[proc_index][chip_index]) { > > > - target_select(chip); > > > - pdbg_for_each_target("thread", > > > chip, thread) { > > > - int thread_index = > > > pdbg_target_index(thread); > > > - if > > > (threadsel[proc_index][chip_index][thread_index]) > > > - target_select(t > > > hread); > > > - else > > > - target_unselect > > > (thread); > > > - } > > > - } else > > > - target_unselect(chip); > > > - } > > > - > > > - /* This is kinda broken as we're overloading > > > what '-c' > > > - * means - it's now up to each command to > > > select targets > > > - * based on core/chiplet. We really need a > > > better > > > - * solution to target selection. */ > > > - pdbg_for_each_target("chiplet", pib, chip) { > > > - int chip_index = > > > pdbg_target_index(chip); > > > - if (chipsel[proc_index][chip_index]) { > > > - target_select(chip); > > > - } else > > > - target_unselect(chip); > > > - } > > > - } else > > > - target_unselect(pib); > > > - } > > > - > > > - pdbg_for_each_class_target("fsi", fsi) { > > > - int index = pdbg_target_index(fsi); > > > - if (processorsel[index]) > > > - target_select(fsi); > > > - else > > > - target_unselect(fsi); > > > - } > > > - > > > > > > if (pathsel_count) { > > > > > > if (!path_target_parse(pathsel, pathsel_count)) > > > > > > return false; > > > > > > } > > > > > > + if (!path_target_present()) { > > > + printf("No valid targets found or specified. Try adding > > > -p/-c/-t > > > > options > > > > > to specify a target.\n"); + printf("Alternatively > > > run 'pdbg -a probe' > to > > > get a list of all valid targets\n"); + return false; > > > + } > > > + > > > > > > return true; > > > > > > } > > > > > > @@ -860,8 +799,5 @@ found_action: > > > if (rc > 0) > > > > > > return 0; > > > > > > - printf("No valid targets found or specified. Try adding -p/-c/- > > > t options > > > to specify a target.\n"); - printf("Alternatively run '%s > > > -a probe' to > get > > > a list of all valid targets\n", - basename(argv[0])); > > > > > > return 1; > > > > > > } > > Amitay.
diff --git a/src/main.c b/src/main.c index c72cb73..1a7d610 100644 --- a/src/main.c +++ b/src/main.c @@ -75,10 +75,6 @@ static int i2c_addr = 0x50; #define MAX_LINUX_CPUS (MAX_PROCESSORS * MAX_CHIPS * MAX_THREADS) -static int **processorsel[MAX_PROCESSORS]; -static int *chipsel[MAX_PROCESSORS][MAX_CHIPS]; -static int threadsel[MAX_PROCESSORS][MAX_CHIPS][MAX_THREADS]; - #define MAX_PATH_ARGS 16 static const char *pathsel[MAX_PATH_ARGS]; @@ -523,19 +519,12 @@ static bool parse_options(int argc, char *argv[]) void target_select(struct pdbg_target *target) { - /* We abuse the private data pointer atm to indicate the target is - * selected */ - pdbg_target_priv_set(target, (void *) 1); -} - -void target_unselect(struct pdbg_target *target) -{ - pdbg_target_priv_set(target, NULL); + path_target_add(target); } bool target_selected(struct pdbg_target *target) { - return (bool) pdbg_target_priv(target); + return path_target_selected(target); } /* Returns the sum of return codes. This can be used to count how many targets the callback was run on. */ @@ -554,7 +543,6 @@ int for_each_child_target(char *class, struct pdbg_target *parent, index = pdbg_target_index(target); assert(index != -1); - pdbg_target_probe(target); status = pdbg_target_status(target); if (status != PDBG_TARGET_ENABLED) continue; @@ -578,7 +566,6 @@ int for_each_target(char *class, int (*cb)(struct pdbg_target *, uint32_t, uint6 index = pdbg_target_index(target); assert(index != -1); - pdbg_target_probe(target); status = pdbg_target_status(target); if (status != PDBG_TARGET_ENABLED) continue; @@ -603,8 +590,6 @@ void for_each_target_release(char *class) static bool target_selection(void) { - struct pdbg_target *fsi, *pib, *chip, *thread; - switch (backend) { #ifdef TARGET_ARM case I2C: @@ -671,63 +656,17 @@ static bool target_selection(void) return false; } - /* At this point we should have a device-tree loaded. We want - * to walk the tree and disabled nodes we don't care about - * prior to probing. */ - pdbg_for_each_class_target("pib", pib) { - int proc_index = pdbg_target_index(pib); - - if (backend == I2C && device_node) - pdbg_target_set_property(pib, "bus", device_node, strlen(device_node) + 1); - - if (processorsel[proc_index]) { - target_select(pib); - pdbg_for_each_target("core", pib, chip) { - int chip_index = pdbg_target_index(chip); - if (pdbg_parent_index(chip, "pib") != proc_index) - continue; - - if (chipsel[proc_index][chip_index]) { - target_select(chip); - pdbg_for_each_target("thread", chip, thread) { - int thread_index = pdbg_target_index(thread); - if (threadsel[proc_index][chip_index][thread_index]) - target_select(thread); - else - target_unselect(thread); - } - } else - target_unselect(chip); - } - - /* This is kinda broken as we're overloading what '-c' - * means - it's now up to each command to select targets - * based on core/chiplet. We really need a better - * solution to target selection. */ - pdbg_for_each_target("chiplet", pib, chip) { - int chip_index = pdbg_target_index(chip); - if (chipsel[proc_index][chip_index]) { - target_select(chip); - } else - target_unselect(chip); - } - } else - target_unselect(pib); - } - - pdbg_for_each_class_target("fsi", fsi) { - int index = pdbg_target_index(fsi); - if (processorsel[index]) - target_select(fsi); - else - target_unselect(fsi); - } - if (pathsel_count) { if (!path_target_parse(pathsel, pathsel_count)) return false; } + if (!path_target_present()) { + printf("No valid targets found or specified. Try adding -p/-c/-t options to specify a target.\n"); + printf("Alternatively run 'pdbg -a probe' to get a list of all valid targets\n"); + return false; + } + return true; } @@ -860,8 +799,5 @@ found_action: if (rc > 0) return 0; - printf("No valid targets found or specified. Try adding -p/-c/-t options to specify a target.\n"); - printf("Alternatively run '%s -a probe' to get a list of all valid targets\n", - basename(argv[0])); return 1; }
Drop the old target selection code. Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- src/main.c | 80 ++++++------------------------------------------------ 1 file changed, 8 insertions(+), 72 deletions(-)