| Message ID | 1433260778-26497-7-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> +static int nest_pmu_create(struct device_node *dev, int pmu_index) > +{ > + struct ppc64_nest_ima_events **p8_events_arr; > + struct ppc64_nest_ima_events *p8_events; > + struct property *pp; > + char *buf; > + const __be32 *lval; > + u32 val; > + int len, idx = 0; > + struct nest_pmu *pmu_ptr; > + const char *start, *end; > + > + if (!dev) > + return -EINVAL; > + > + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); > + if (!pmu_ptr) > + return -ENOMEM; > + > + /* Needed for hotplug/migration */ > + per_nestpmu_arr[pmu_index] = pmu_ptr; > + > + p8_events_arr = kzalloc((sizeof(struct ppc64_nest_ima_events) * 64), > + GFP_KERNEL); > + if (!p8_events_arr) > + return -ENOMEM; > + p8_events = (struct ppc64_nest_ima_events *)p8_events_arr; I think you're trying to get the first element of the array here: why not just `p8_events = p8_events_arr[0];`? > + > + /* > + * Loop through each property > + */ > + for_each_property_of_node(dev, pp) { > + start = pp->name; > + end = start + strlen(start); > + len = strlen(start); > + > + if (!strcmp(pp->name, "name")) { > + if (!pp->value || > + (strnlen(pp->value, pp->length) >= pp->length)) > + return -EINVAL; > + > + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + sprintf(buf, "Nest_%s", (char *)pp->value); > + pmu_ptr->pmu.name = (char *)buf; > + pmu_ptr->attr_groups[1] = &p8_nest_format_group; > + pmu_ptr->attr_groups[2] = &cpumask_nest_pmu_attr_group; > + } > + > + /* Skip these, we dont need it */ > + if (!strcmp(pp->name, "name") || > + !strcmp(pp->name, "phandle") || > + !strcmp(pp->name, "device_type") || > + !strcmp(pp->name, "linux,phandle")) > + continue; > + > + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + if (strncmp(pp->name, "unit.", 5) == 0) { > + start += 5; > + len = strlen(start); > + strncpy(buf, start, strlen(start)); You've just saved strlen(start), you could just use len. This also applies in the next case below. > + p8_events->ev_name = buf; > + > + if (!pp->value || > + (strnlen(pp->value, pp->length) >= pp->length)) > + return -EINVAL; The strnlen will never be greater than pp->length, so the only case this will hit is if strnlen(pp->value, pp->length) == pp->length. This also applies again below. > + > + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + strncpy(buf, (const char *)pp->value, pp->length); > + p8_events->ev_value = buf; > + idx++; > + p8_events++; > + > + } else if (strncmp(pp->name, "scale.", 6) == 0) { > + start += 6; > + len = strlen(start); > + strncpy(buf, start, strlen(start)); > + p8_events->ev_name = buf; > + > + if (!pp->value || > + (strnlen(pp->value, pp->length) >= pp->length)) > + return -EINVAL; > + > + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + strncpy(buf, (const char *)pp->value, pp->length); > + p8_events->ev_value = buf; > + idx++; > + p8_events++; > + > + } else { > + strncpy(buf, start, len); This is the only case where you actually use the orignal version of len. This makes me think you could drop the variable entirely and just use strlen(start) in all cases. I also don't see where `end` is used anywhere in this function: could that be dropped? > + p8_events->ev_name = buf; > + lval = of_get_property(dev, pp->name, NULL); > + val = (uint32_t)be32_to_cpup(lval); > + > + /* > + * Use DT property value as the event > + */ I'm not sure if this is my mailer, but it looks like lines 2 and 3 of that comment need to be indented to line up under the * in the first line. > + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + sprintf(buf,"event=0x%x", val); > + p8_events->ev_value = buf; > + p8_events++; > + idx++; > + } > + } > + > + return 0; > +} > + > @@ -288,7 +427,7 @@ static int __init nest_pmu_init(void) > cpumask_chip(); > > /* > - * Detect the Nest PMU feature > + * Detect the Nest PMU feature and register the pmus > */ > if (nest_ima_detect_parse()) > return 0; As the changed comment indicates, this function changes the behaviour of nest_ima_detect_parse. Given that it's a new function introduced by this patch series, maybe it should also change names.
On Wednesday 03 June 2015 06:16 AM, Daniel Axtens wrote: >> +static int nest_pmu_create(struct device_node *dev, int pmu_index) >> +{ >> + struct ppc64_nest_ima_events **p8_events_arr; >> + struct ppc64_nest_ima_events *p8_events; >> + struct property *pp; >> + char *buf; >> + const __be32 *lval; >> + u32 val; >> + int len, idx = 0; >> + struct nest_pmu *pmu_ptr; >> + const char *start, *end; >> + >> + if (!dev) >> + return -EINVAL; >> + >> + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); >> + if (!pmu_ptr) >> + return -ENOMEM; >> + >> + /* Needed for hotplug/migration */ >> + per_nestpmu_arr[pmu_index] = pmu_ptr; >> + >> + p8_events_arr = kzalloc((sizeof(struct ppc64_nest_ima_events) * 64), >> + GFP_KERNEL); >> + if (!p8_events_arr) >> + return -ENOMEM; >> + p8_events = (struct ppc64_nest_ima_events *)p8_events_arr; > I think you're trying to get the first element of the array here: why > not just `p8_events = p8_events_arr[0];`? Yes. Will change it. >> + >> + /* >> + * Loop through each property >> + */ >> + for_each_property_of_node(dev, pp) { >> + start = pp->name; >> + end = start + strlen(start); >> + len = strlen(start); >> + >> + if (!strcmp(pp->name, "name")) { >> + if (!pp->value || >> + (strnlen(pp->value, pp->length) >= pp->length)) >> + return -EINVAL; >> + >> + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + sprintf(buf, "Nest_%s", (char *)pp->value); >> + pmu_ptr->pmu.name = (char *)buf; >> + pmu_ptr->attr_groups[1] = &p8_nest_format_group; >> + pmu_ptr->attr_groups[2] = &cpumask_nest_pmu_attr_group; >> + } >> + >> + /* Skip these, we dont need it */ >> + if (!strcmp(pp->name, "name") || >> + !strcmp(pp->name, "phandle") || >> + !strcmp(pp->name, "device_type") || >> + !strcmp(pp->name, "linux,phandle")) >> + continue; >> + >> + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + if (strncmp(pp->name, "unit.", 5) == 0) { >> + start += 5; >> + len = strlen(start); >> + strncpy(buf, start, strlen(start)); > You've just saved strlen(start), you could just use len. This also > applies in the next case below. Yes. That is true. >> + p8_events->ev_name = buf; >> + >> + if (!pp->value || >> + (strnlen(pp->value, pp->length) >= pp->length)) >> + return -EINVAL; > The strnlen will never be greater than pp->length, so the only case this > will hit is if strnlen(pp->value, pp->length) == pp->length. This also > applies again below. True will change it. > >> + >> + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + strncpy(buf, (const char *)pp->value, pp->length); >> + p8_events->ev_value = buf; >> + idx++; >> + p8_events++; >> + >> + } else if (strncmp(pp->name, "scale.", 6) == 0) { >> + start += 6; >> + len = strlen(start); >> + strncpy(buf, start, strlen(start)); >> + p8_events->ev_name = buf; >> + >> + if (!pp->value || >> + (strnlen(pp->value, pp->length) >= pp->length)) >> + return -EINVAL; >> + >> + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + strncpy(buf, (const char *)pp->value, pp->length); >> + p8_events->ev_value = buf; >> + idx++; >> + p8_events++; >> + >> + } else { >> + strncpy(buf, start, len); > This is the only case where you actually use the orignal version of len. > This makes me think you could drop the variable entirely and just use > strlen(start) in all cases. I also don't see where `end` is used > anywhere in this function: could that be dropped? Correct. I guess we can drop both len and end. I used "end" for my prints during debug. >> + p8_events->ev_name = buf; >> + lval = of_get_property(dev, pp->name, NULL); >> + val = (uint32_t)be32_to_cpup(lval); >> + >> + /* >> + * Use DT property value as the event >> + */ > I'm not sure if this is my mailer, but it looks like lines 2 and 3 of > that comment need to be indented to line up under the * in the first > line. No, it is not your mail :). Will fix the indentation. >> + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> + >> + sprintf(buf,"event=0x%x", val); >> + p8_events->ev_value = buf; >> + p8_events++; >> + idx++; >> + } >> + } >> + >> + return 0; >> +} >> + >> @@ -288,7 +427,7 @@ static int __init nest_pmu_init(void) >> cpumask_chip(); >> >> /* >> - * Detect the Nest PMU feature >> + * Detect the Nest PMU feature and register the pmus >> */ >> if (nest_ima_detect_parse()) >> return 0; > As the changed comment indicates, this function changes the behaviour of > nest_ima_detect_parse. Given that it's a new function introduced by this > patch series, maybe it should also change names. > Ok will fix it. Thanks for the review. Maddy
diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c index c979e57..514a0be 100644 --- a/arch/powerpc/perf/nest-pmu.c +++ b/arch/powerpc/perf/nest-pmu.c @@ -12,6 +12,7 @@ #include "nest-pmu.h" static struct perchip_nest_info p8_perchip_nest_info[P8_MAX_CHIP]; +static struct nest_pmu *per_nestpmu_arr[P8_MAX_NEST_PMUS]; static cpumask_t cpu_mask_nest_pmu; static ssize_t cpumask_nest_pmu_get_attr(struct device *dev, @@ -243,6 +244,129 @@ static int update_pmu_ops(struct nest_pmu *pmu) return 0; } +static int nest_pmu_create(struct device_node *dev, int pmu_index) +{ + struct ppc64_nest_ima_events **p8_events_arr; + struct ppc64_nest_ima_events *p8_events; + struct property *pp; + char *buf; + const __be32 *lval; + u32 val; + int len, idx = 0; + struct nest_pmu *pmu_ptr; + const char *start, *end; + + if (!dev) + return -EINVAL; + + pmu_ptr = kzalloc(sizeof(struct nest_pmu), GFP_KERNEL); + if (!pmu_ptr) + return -ENOMEM; + + /* Needed for hotplug/migration */ + per_nestpmu_arr[pmu_index] = pmu_ptr; + + p8_events_arr = kzalloc((sizeof(struct ppc64_nest_ima_events) * 64), + GFP_KERNEL); + if (!p8_events_arr) + return -ENOMEM; + p8_events = (struct ppc64_nest_ima_events *)p8_events_arr; + + /* + * Loop through each property + */ + for_each_property_of_node(dev, pp) { + start = pp->name; + end = start + strlen(start); + len = strlen(start); + + if (!strcmp(pp->name, "name")) { + if (!pp->value || + (strnlen(pp->value, pp->length) >= pp->length)) + return -EINVAL; + + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + sprintf(buf, "Nest_%s", (char *)pp->value); + pmu_ptr->pmu.name = (char *)buf; + pmu_ptr->attr_groups[1] = &p8_nest_format_group; + pmu_ptr->attr_groups[2] = &cpumask_nest_pmu_attr_group; + } + + /* Skip these, we dont need it */ + if (!strcmp(pp->name, "name") || + !strcmp(pp->name, "phandle") || + !strcmp(pp->name, "device_type") || + !strcmp(pp->name, "linux,phandle")) + continue; + + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + if (strncmp(pp->name, "unit.", 5) == 0) { + start += 5; + len = strlen(start); + strncpy(buf, start, strlen(start)); + p8_events->ev_name = buf; + + if (!pp->value || + (strnlen(pp->value, pp->length) >= pp->length)) + return -EINVAL; + + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + strncpy(buf, (const char *)pp->value, pp->length); + p8_events->ev_value = buf; + idx++; + p8_events++; + + } else if (strncmp(pp->name, "scale.", 6) == 0) { + start += 6; + len = strlen(start); + strncpy(buf, start, strlen(start)); + p8_events->ev_name = buf; + + if (!pp->value || + (strnlen(pp->value, pp->length) >= pp->length)) + return -EINVAL; + + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + strncpy(buf, (const char *)pp->value, pp->length); + p8_events->ev_value = buf; + idx++; + p8_events++; + + } else { + strncpy(buf, start, len); + p8_events->ev_name = buf; + lval = of_get_property(dev, pp->name, NULL); + val = (uint32_t)be32_to_cpup(lval); + + /* + * Use DT property value as the event + */ + buf = kzalloc(MAX_PMU_NAME_LEN, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + sprintf(buf,"event=0x%x", val); + p8_events->ev_value = buf; + p8_events++; + idx++; + } + } + + return 0; +} + static int nest_ima_detect_parse(void) { const __be32 *gcid; @@ -270,6 +394,21 @@ static int nest_ima_detect_parse(void) rc = 0; } + /* + * At this point if nest-ima not found in DT, return. + */ + if (rc) + return rc; + + /* + * Look for Nest IMA units supported here. + */ + idx = 0; /* Reuse for nest pmu counts */ + for_each_node_by_type(dev, "nest-ima-unit") { + nest_pmu_create(dev, idx); + idx++; + } + return rc; } @@ -288,7 +427,7 @@ static int __init nest_pmu_init(void) cpumask_chip(); /* - * Detect the Nest PMU feature + * Detect the Nest PMU feature and register the pmus */ if (nest_ima_detect_parse()) return 0;
Patch adds a device-tree parser function to detect each Nest PMU and will traverse through the folder to find the supported events and corresponding unit and scale files, if any. Event file will contain the offset in HOMER region to get the counter data for a given event. Kernel DT parser looks for scale/unit in the file name and pass on the file as an event attr for perf tool to use in the post processing. For scale and unit, DT will have file name starting with "scale.<event name>.scale" and "unit.<event name>.unit". Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com> Cc: Stephane Eranian <eranian@google.com> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- arch/powerpc/perf/nest-pmu.c | 141 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 1 deletion(-)