[Pdbg] [PATCH v2 08/15] main: Switch to path based target selection

[Amitay Isaacs]

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.
-- 

Real generosity is doing something nice for someone who'll never find it out.
- Frank A. Clark