On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: > Patch adds common event attribute function and Nest pmu registration call. > > Cc: Michael Ellerman <[email protected]> > Cc: Benjamin Herrenschmidt <[email protected]> > Cc: Paul Mackerras <[email protected]> > Cc: Sukadev Bhattiprolu <[email protected]> > Cc: Anshuman Khandual <[email protected]> > Cc: Stephane Eranian <[email protected]> > Signed-off-by: Madhavan Srinivasan <[email protected]> > --- > arch/powerpc/perf/nest-pmu.c | 52 > ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c > index 514a0be..dd84fd7 100644 > --- a/arch/powerpc/perf/nest-pmu.c > +++ b/arch/powerpc/perf/nest-pmu.c > @@ -244,6 +244,49 @@ static int update_pmu_ops(struct nest_pmu *pmu) > return 0; > } > > +/* > + * Populate event name and string in attribute > + */ > +struct attribute *dev_str_attr(char *name, char *str) > +{ > + struct perf_pmu_events_attr *attr; > + > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > + > + attr->event_str = (const char *)str; Erk. Two things: - Is str const or not? If you're treating it as const here, should you pass that through the function signature? - Who is responsible for the memory behind it? It looks like a caller can't construct str dynamically, pass it to this function and then free it, because that will invalidate attr->event_str. Is this documented?
> + attr->attr.attr.name = name;
> + attr->attr.attr.mode = 0444;
> + attr->attr.show = perf_event_sysfs_show;
> +
> + return &attr->attr.attr;
If you're returning the address of attr->attr.attr, then:
- why don't you just deal directly with struct attribute * in the
function? Why an entire struct perf_pmu_events_attr *?
- with the function as written, if you return just &attr->attr.attr,
don't attr->event_str and attr->attr.show get lost?
> +}
> +
> +int update_events_in_group(
> + struct ppc64_nest_ima_events *p8_events, int idx,
> + struct nest_pmu *pmu)
> +{
> + struct attribute_group *attr_group;
> + struct attribute **attrs;
> + int i;
> +
> + attr_group = kzalloc(((sizeof(struct attribute *) * (idx + 1)) +
> + sizeof(*attr_group)), GFP_KERNEL);
> + if (!attr_group)
> + return -ENOMEM;
> +
> + attrs = (struct attribute **)(attr_group + 1);
> + attr_group->name = "events";
> + attr_group->attrs = attrs;
> +
> + for (i=0; i< idx; i++, p8_events++)
> + attrs[i] = dev_str_attr((char *)p8_events->ev_name,
> + (char *)p8_events->ev_value);
> +
> + pmu->attr_groups[0] = attr_group;
> + return 0;
> +}
I'm very confused by what this function is trying to do. Could you add
some comments? I'm particularly confused by the relationship between
attrs and attr_group.
> +
> +
> static int nest_pmu_create(struct device_node *dev, int pmu_index)
> {
> struct ppc64_nest_ima_events **p8_events_arr;
> @@ -364,6 +407,15 @@ static int nest_pmu_create(struct device_node *dev, int
> pmu_index)
> }
> }
>
> + update_events_in_group(
> + (struct ppc64_nest_ima_events *)p8_events_arr,
> + idx, pmu_ptr);
> + update_pmu_ops(pmu_ptr);
> +
> + /* Register the pmu */
> + perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> + printk(KERN_INFO "Nest PMU %s Registered\n", pmu_ptr->pmu.name);
> +
> return 0;
> }
>
Regards,
Daniel
signature.asc
Description: This is a digitally signed message part
