Hi Masami,
On Wed, 2018-12-19 at 21:22 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> On Tue, 18 Dec 2018 14:33:22 -0600
> Tom Zanussi <zanu...@kernel.org> wrote:
>
> > From: Tom Zanussi <tom.zanu...@linux.intel.com>
> >
> > Since all the variable reference hist_fields are collected into
> > hist_data->var_refs[] array, there's no need to go through all the
> > fields looking for them, or in separate arrays like
> > synth_var_refs[],
> > which will be going away soon anyway.
> >
> > This also allows us to get rid of some unnecessary code and
> > functions
> > currently used for the same purpose.
>
> I just have a nitpick.
>
> >
> > Signed-off-by: Tom Zanussi <tom.zanu...@linux.intel.com>
> > ---
> > kernel/trace/trace_events_hist.c | 57 +++++-----------------------
> > ------------
> > 1 file changed, 7 insertions(+), 50 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index 25d06b3ae1f6..ee839c52bd3f 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field
> > *hist_field,
> > {
> > struct hist_field *found = NULL;
> >
> > - if (hist_field && hist_field->flags &
> > HIST_FIELD_FL_VAR_REF) {
> > - if (hist_field->var.idx == var_idx &&
> > - hist_field->var.hist_data == var_data) {
> > - found = hist_field;
> > - }
> > - }
> > -
> > - return found;
> > -}
> > -
> > -static struct hist_field *
> > -check_field_for_var_refs(struct hist_trigger_data *hist_data,
> > - struct hist_field *hist_field,
> > - struct hist_trigger_data *var_data,
> > - unsigned int var_idx,
> > - unsigned int level)
> > -{
> > - struct hist_field *found = NULL;
> > - unsigned int i;
> > -
> > - if (level > 3)
> > - return found;
> > -
> > - if (!hist_field)
> > - return found;
> > + WARN_ON(!(hist_field && hist_field->flags &
> > HIST_FIELD_FL_VAR_REF));
> >
> > - found = check_field_for_var_ref(hist_field, var_data,
> > var_idx);
> > - if (found)
> > - return found;
> > -
> > - for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
> > - struct hist_field *operand;
> > -
> > - operand = hist_field->operands[i];
> > - found = check_field_for_var_refs(hist_data,
> > operand, var_data,
> > - var_idx, level +
> > 1);
> > - if (found)
> > - return found;
> > - }
> > + if (hist_field && hist_field->var.idx == var_idx &&
> > + hist_field->var.hist_data == var_data)
> > + found = hist_field;
> >
> > return found;
>
> It seems we don't need "found" var here. Just return hist_field or
> NULL.
>
OK, will change these and resubmit shortly.
Thanks,
Tom
