Re: [PATCH v1 6/9]powerpc/powernv: dt parser function for nest pmu and its events
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
+
Re: [PATCH v1 6/9]powerpc/powernv: dt parser function for nest pmu and its events
> +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 =
